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

cmd/compile: better error messages from types2 (Go 1.18) for unexported fields/methods #49736

Open
danscales opened this issue Nov 22, 2021 · 8 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@danscales
Copy link
Contributor

danscales commented Nov 22, 2021

The error messages from types2 for Go 1.18, I think, as less helpful than the Go 1.17 (-G=0) messages in the case of unexported fields and members. This is shown for the two existing bug test cases in go/test/fixedbugs, issue18419.go and issue31053.go.

We have disabled these tests in run.go because of different error message output for Go 1.17 and Go.18, and the difference is because Go 1.18 is not helpfully indicating the problem is due to an unexported field/method, but is instead indicating the field/method is unknown or doesn't exist.
(I'm going through a bunch of the other disabled tests, which are mostly due to largely equivalent error messages, just slight different words or a few extra, related errors. One area that I will work on fixing is missing errors because of improper compiler directives (//go:...), which are mostly ignored by types2.)

You can enable the test for each of these issues by commenting out the specified issue in types2Failures array in test/run.go.

For issue 18419, the Go 1.17 error message is:

./test.go:12:3: e.member undefined (cannot refer to unexported field or method member)

whereas for Go 1.18 it is:

./test.go:12:4: e.member undefined (type *other.Exported has no field or method member, but does have Member)

For issue 31053, there are many error messages that match, but the two relevant ones that differ are for Go 1.17:

./p.go:13:3: cannot refer to unexported field 'doneChan' in struct literal of type f1.Foo
./p.go:20:3: cannot refer to unexported field 'hook' in struct literal of type f1.Foo

and for Go 1.18:

./p.go:13:3: unknown field 'doneChan' in struct literal of type f1.Foo
./p.go:20:3: unknown field 'hook' in struct literal of type f1.Foo

If it makes sense to fix, I'm sure this is fine to fix either before or after the beta release.

@griesemer @findleyr

@danscales danscales added this to the Go1.18 milestone Nov 22, 2021
@danscales danscales added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Nov 22, 2021
@griesemer griesemer self-assigned this Nov 22, 2021
@griesemer
Copy link
Contributor

griesemer commented Nov 22, 2021

For issue 18419, the error message is longer in 1.18 but provide equal if not more information.
For issue 31053, the two messages that are different should probably be fixed.

@griesemer
Copy link
Contributor

griesemer commented Nov 22, 2021

cc: @findleyr

@findleyr
Copy link
Contributor

findleyr commented Nov 22, 2021

For issue 18419, the error message is longer in 1.18 but provide equal if not more information.

I don't think that's entirely true. Exported has both member and Member. The 1.17 message points out the unexported member, the 1.18 message points out the exported Member. Both are useful, but this from the 1.18 error message seems a little misleading: *other.Exported has no field or method member.

@danscales
Copy link
Contributor Author

danscales commented Nov 22, 2021

Yes, I agree on 18419 - the message is factually incorrect. In both cases, essentially types2 is not noting that the real problem is that the available field/method is not exported, so can't be used. (I guess in issue 18419, you could argue that the more likely error the user is making is not calling the upper-case Member, as opposed to the lower-case member (which is a field), though hard to guess the user's intent.)

@gopherbot
Copy link

gopherbot commented Jan 12, 2022

Change https://golang.org/cl/378177 mentions this issue: cmd/compile/types2, go/types2: report access of unexported field/method

@odeke-em
Copy link
Member

odeke-em commented Jan 13, 2022

I've mailed a fix for this for types2 per CL https://golang.org/cl/378177, please take a look.

@griesemer
Copy link
Contributor

griesemer commented Feb 9, 2022

Moving to 1.19. There's too many parts that may need to be adjusted. Also, what we have now is ok.

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Feb 9, 2022
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Feb 9, 2022
@griesemer
Copy link
Contributor

griesemer commented Jun 24, 2022

Still good to fix, but not urgent. Moving to 1.20.

@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: No status
Development

No branches or pull requests

5 participants