test: errorcheck support for intraline errors

[mdempsky]

Now that the compiler supports reporting column position for errors, we need a mechanism in test/run.go for testing this.

I propose we use /* ERROR "foo" */ placed just before where we expect the error to be emitted, except to allow for white space in between.

E.g., for #19576, the test would be

package p

func f() int { /* ERROR "missing return" */ }

Rationale:

• run.go currently only looks for // ERROR annotations, which can only occur at the end of the line, so there's no risk of collision using /* ERROR */ for column-precise error matching.

• Not explicitly having to write column numbers into the errors makes them easier to maintain, just like how we don't currently have to worry about line numbers.

• We emit errors at the start of tokens, so putting the error expectation before that lets us unambiguously match it to an expected column. Putting afterwards would require more complex logic for figuring out how many characters to rewind past.

• Allowing whitespace after the error comment allows the code to be gofmt'd.

/cc @griesemer @ianlancetaylor @bradfitz

[ianlancetaylor]

My main comment is that I would prefer that we not go heavily into testing column values. gccgo uses the same testsuite, and it's inevitable that there will be some disagreements about column position, and that will force the use of GC_ERROR and GCCGO_ERROR, which makes the tests that much less readable.

I think your suggestion sounds good for cases where we do explicitly want to test column values.

[griesemer]

The go/types test suite uses this mechanism, except that (for a marginally simpler implementation) it requires the ERROR comments to be after the offending token. @mdempsky 's suggestion is better (and I wanted to change this for go/types in the past) because as is in go/types, the ERROR comment often ends up in the middle of some construct, for instance a selector expression:

... x /* ERROR "x has no field or method f" */ .f

One way to make column testing not destroy the readability of code is to use an extra comment and leave the ERROR comments where they are. That would also make it easier to convert existing tests:

... /*1*/ x.f  // ERROR "x has no field or method f"

The number in /*1*/ could be the index of the respective ERROR comment at the end of the line. If there's no index comment, we just check the line.

[randall77]

You could also do something like
// ERROR TOKEN 5 "x has no field or method f"
Which would require that the error occur at the same column as the 5th token if you tokenize that line. Hopefully all errors occur at a token, right?
It's not quite testing columns, but it is probably a lot easier to count tokens than columns and nicer than interstitial comments.

[griesemer]

Errors are positioned at tokens because the compiler only sees tokens and their positions. But using a comment in place where the error happens is much more maintainable: We don't want to re-arrange all token numbers just because we changed the test code a little bit.

[mdempsky]

@ianlancetaylor Agreed, I think for most tests line position information is adequate.

@griesemer Using /*1*/ seems reasonable too. Perhaps less intrusive to implement too. Also has the benefit for gccgo that we could include multiple /*1*/ markers and accept the error at any of them, or explicitly annotate them like /*GC:1*/ or /*GCCGO:1*/.

@randall77 Yeah, errors are always at the start of a token. But if we're going to require test authors to explicitly record position information, I'd lean towards just writing the column number directly.

[odeke-em]

Just a drive-by post, @mdempsky sent me here. The regress test from https://go-review.googlesource.com/c/go/+/76150 could use a cleanup once we've implemented this feature.

And for context, we need to be able to test that running the repro for #21317

package main

import "fmt"

func main() {
        n, err := fmt.Print(1)
}

gives

./main.go:6:9: n declared and not used
./main.go:6:12: err declared and not used

and that we can test for the presence of those column numbers.

[quasilyte]

The /* ERROR "msg" */ format can make line harder to read (subjectively):

/* ERROR "n declared and not used" */ n, /* ERROR "err declared and not used" */ err := fmt.Println(1)

Numbered comments make things better:

/*1*/ n, /*2*/ err := fmt.Println(1) // ERROR "n declared and not used" "err declared and not used"

But after you run gofmt, it will look like this:

/*1*/ n /*2*/, err := fmt.Println(1) // ERROR "n declared and not used" "err declared and not used"

I find it somewhat confusing.

If indexed comments are placed after the token, there is no such problem:

n /*1*/, err /*2*/ := fmt.Println(1) // ERROR "n declared and not used" "err declared and not used"

By the way, test for issue21317 could be written without any changes to errorcheck:

// errorcheck

package main

import "fmt"

func main() {
	n, // ERROR "n declared and not used"
		err := // ERROR "err declared and not used"
		fmt.Println(1)
}

[griesemer]

@quasilyte It is already the case that we do not gofmt the test directory (at least as a whole) since some tests rely on the specific layout of the code. I don't expect that to change.