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

Jump to bottom

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

Closed
danscales opened this issue Nov 22, 2021 · 12 comments
Closed

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

danscales opened this issue Nov 22, 2021 · 12 comments
Assignees
@griesemer
Labels
BadErrorMessage Issues related compiler error messages that should be better. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@danscales
Copy link
Contributor

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

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

cc: @findleyr

@findleyr
Copy link
Contributor

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

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

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

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

@griesemer
Copy link
Contributor

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

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

@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@griesemer griesemer modified the milestones: Go1.20, Go1.21 Nov 21, 2022
@griesemer
Copy link
Contributor

Too late for 1.21. Moving forward.

@griesemer griesemer modified the milestones: Go1.21, Go1.22 Jun 6, 2023
@griesemer griesemer added the BadErrorMessage Issues related compiler error messages that should be better. label Dec 7, 2023
@griesemer
Copy link
Contributor

Too late for 1.22. Moving forward.

@griesemer griesemer modified the milestones: Go1.22, Go1.23 Dec 12, 2023
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 12, 2023
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.23.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

Change https://go.dev/cl/560055 mentions this issue: types2: better error when accessing an unexported field or method

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@griesemer @ezz-no
go/types, types2: better errors for non-existing fields or methods
e9b3256 
This CL improves the error messages reported when a field or method
name is used that doesn't exist. It brings the error messges on par
(or better) with the respective errors reported before Go 1.18 (i.e.
before switching to the new type checker):

Make case distinctions based on whether a field/method is exported
and how it is spelled. Factor out that logic into a new function
(lookupError) in a new file (errsupport.go), which is generated for
go/types. Use lookupError when reporting selector lookup errors
and missing struct field keys.

Add a comprehensive set of tests (lookup2.go) and spot tests for
the two cases brought up by the issue at hand.

Adjusted existing tests as needed.

Fixes golang#49736.

Change-Id: I2f439948dcd12f9bd1a258367862d8ff96e32305
Reviewed-on: https://go-review.googlesource.com/c/go/+/560055
Run-TryBot: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
BadErrorMessage Issues related compiler error messages that should be better. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.23
Development

No branches or pull requests
5 participants
@danscales @odeke-em @griesemer @gopherbot @findleyr