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

go/types: wrong line numbers in errors for constant overflows involving iota #42991

Closed
griesemer opened this issue Dec 4, 2020 · 5 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@griesemer
Copy link
Contributor

go test -run Check$ -files $GOROOT/test/fixedbugs/issue8183.go --errlist

produces

    check_test.go:267: .../issue8183.go:12:12: cannot convert iota + 253 (untyped int constant 256) to byte
    check_test.go:267: .../issue8183.go:19:11: invalid array length 1 - iota (untyped int constant -1)
    check_test.go:267: .../issue8183.go:19:11: invalid array length 1 - iota (untyped int constant -2)

yet the correct line numbers are 15, 21, and 22.

@griesemer griesemer added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 4, 2020
@griesemer griesemer added this to the Go1.16 milestone Dec 4, 2020
@griesemer
Copy link
Contributor Author

griesemer commented Dec 4, 2020

go/types doesn't make a copy of the syntax trees for "copied" constant expressions in grouped constant declarations; it evaluates them with the correct iota value but at their original position. As a result we get the wrong line numbers in the result. I'm amazed this has not come up in the last few years of go/types use.

One possibility might be to remember a line correction delta when we evaluate an expression containing an iota. The line correction delta is the iota value.

Follow-up: @findleyr I think I've got a fairly simple fix for this. Need to rethink/test/clean up etc. but hopefully should be ready sometime tomorrow or over the weekend.

@findleyr
Copy link
Contributor

findleyr commented Dec 4, 2020

Thanks for investigating this @griesemer. I'm not sure I understand your point about using the iota value to compute correction delta, since there need not be any correlation between iota value and line number, for example https://play.golang.org/p/FQfRsDMMv8v.

@griesemer
Copy link
Contributor Author

@findleyr Yes, you're correct - I've realized that last night as well. Turns out we just need to remember the position of the actual constant being initialized; in fact we need that position anyway to be able to position the error correctly. In short: Whenever we evaluate a constant initialization expression that is "inherited" from (shared with) other constants, we remember the position of the constant identifier. If there's an error in the initialization expression, we use the position of the constant identifier (this is stored in the current Checker context) for the error location (and we can still point to the original expression as well).

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/275517 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: correct error position for inherited const init expression

gopherbot pushed a commit that referenced this issue Dec 7, 2020
…for inherited const init expression

Enabled fixedbugs/issue8183.go for run.go with new typechecker
now that issue is fixed.

Fixes #42992.
Updates #42991.

Change-Id: I23451999983b740d5f37ce3fa75ee756daf1a44f
Reviewed-on: https://go-review.googlesource.com/c/go/+/275517
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/276172 mentions this issue: go/types: correct error position for inherited const init expressions

@golang golang locked and limited conversation to collaborators Dec 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants