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/ast,go/parser: Decl field of type parameter identifier is nil for methods (but not for functions) #50956

Open
aarzilli opened this issue Feb 1, 2022 · 7 comments
Labels
NeedsFix
Milestone

Comments

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented Feb 1, 2022

$ go version
go version devel go1.18-902dc38212 Tue Feb 1 16:52:46 2022 +0000 linux/amd64

See: https://gotipplay.golang.org/p/QhA7oB1-h54
For F, a generic funciton, the identifier T has a Decl field referring to the field in the type parameter list of F.
But for M, a method for a generic type, the identifier T` has a nil Decl field instead.

Is this intentional? Seems inconsistent.

cc @findleyr

@findleyr
Copy link
Contributor

@findleyr findleyr commented Feb 1, 2022

Thank you. No, this is not intentional. See also #45221, which may have missed methods.

Will try to fix for 1.18 (but not marking as a release blocker as we are trying to be very selective about release blockers at this point.

CC @griesemer

@findleyr findleyr added this to the Go1.18 milestone Feb 1, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 1, 2022

Change https://golang.org/cl/382247 mentions this issue: go/parser: resolve receiver type parameters

@findleyr
Copy link
Contributor

@findleyr findleyr commented Feb 2, 2022

I recalled incorrectly: as commented here we didn't resolve receiver type parameters because we weren't sure how to declare them: the docmentation for ast.Object.Decl does not (currently) allow for *ast.Ident's (the parameter expression) to be a declaration.

In general, syntactic object resolution is unreliable (it can be incorrect), slow, and of questionable value -- tools that need object resolution should probably use go/types. Also, if we make the API change to allow ast.Object.Decl to hold identifiers, we risk breaking working code.

@griesemer and I discussed this today and think we should consider this for 1.19, but probably shouldn't do anything for 1.18 at this point.

One thing that occurred to me, however, is that we could/should avoid incorrect object resolution involving type parameters. For example:

package p

type T struct{}
type P struct{}

func (T[P]) m(p P){}

In this example, because we don't declare the type parameter P, I think we'll resolve the struct type declaration for the P used in the signature of m. @griesemer what do you think about fixing this by declaring type parameters but not recording them when we resolve?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 2, 2022

@findleyr Fixing the incorrect resolution for 1.18 seems fine, if it is easy (which I think it might be). But if it's anything more than that, it's ok to defer at this point. We're veryclose to the release and object resolution is not a feature that we recommend using in the first place. We won't fix everything for 1.18 and this is certainly low priority.

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Feb 3, 2022

syntactic object resolution is unreliable (it can be incorrect), slow, and of questionable value -- tools that need object resolution should probably use go/types.

object resolution is not a feature that we recommend using in the first place

Shouldn't this be documented somewhere? go/ast doesn't say anything about this and go/parser just documents the existence of SkipObjectResolution.

@toothrot toothrot added the NeedsFix label Feb 4, 2022
@findleyr
Copy link
Contributor

@findleyr findleyr commented Feb 4, 2022

Shouldn't this be documented somewhere? go/ast doesn't say anything about this and go/parser just documents the existence of SkipObjectResolution.

We should document this, however I think maybe we overstated a bit: I saw your CL and godoc is one place where syntactic object resolution makes sense and should generally work. I am not sure that it is 100% obvious that changing godoc to use go/types is a good change, as the performance implications are significant and imprecision is acceptable.

Unfortunately, at this point it is really too late to add a new Decl type to the documentation go/ast.Object, and this is possible to work-around (albeit awkwardly), so I will avoid the incorrect resolution in the associated CL, and move this to 1.19. We will address this in 1.19. Sorry.

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Feb 4, 2022

We will address this in 1.19. Sorry.

No, I think it's a good decision.

@findleyr findleyr removed this from the Go1.18 milestone Feb 7, 2022
@findleyr findleyr added this to the Go1.19 milestone Feb 7, 2022
gopherbot pushed a commit that referenced this issue Feb 7, 2022
Declare receiver type parameters in the function scope, but don't
resolve them (for now), as ast.Object.Decl is not documented to hold
*ast.Idents. This avoids incorrect resolution of identifiers to names
outside the function scope.

Also make tracing and error reporting more consistent.

For #50956

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

No branches or pull requests

5 participants