Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: errorcheck support for intraline errors #19577

Open
mdempsky opened this issue Mar 16, 2017 · 8 comments

Comments

@mdempsky
Copy link
Member

commented Mar 16, 2017

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

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 16, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2017

@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

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

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.

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Dec 7, 2017

quasilyte added a commit to quasilyte/go-contributing-ru that referenced this issue Apr 1, 2018
tasks: add several new tracked tasks
New tasks include:
golang/go#19675 cmd/vet: report uses of -0 in float32/64 context
golang/go#19683 cmd/compile: eliminate usages of global lineno
golang/go#19670 x/tools/go/ssa: make opaqueType less annoying to use
golang/go#19636 encoding/base64: decoding is slow
golang/go#23471 x/perf/cmd/benchstat: tips or quickstart for newcomers
golang/go#19577 test: errorcheck support for intraline errors
golang/go#19490 cmd/vet: reduce the amount of false positives for -shadow mode
golang/go#19042 cmd/internal/obj: optimize wrapper method prologue for branch prediction
golang/go#19013 cmd/compile: add tool for understanding/debugging SSA rules
@quasilyte

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.