-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: clarify when Ident.Obj is nil and how to find the ident's Obj #48141
Comments
For yet more context and an example:
The question I am having trouble answering is what are the guarantees on the |
(the parser doesn't know whether the use of 'f' is as a struct field or map key). We could try to document the exact rules, but in general there are problems with syntactic object resolution, and it is better to use go/types wherever possible. Would that be possible in the CL in question? See also #45104. While working on that refactoring, I discovered a couple other edge cases where |
I don't think we need something exact. Just enough to know what ast.Ident.Obj == nil implies (or if it is just "optional"). If the answer is package scope, the original CL was already correct. If the answer is really complicated then various cmd/vet checkers should probably steer clear anyways.
I think so. What we ultimately needed that context is the |
Change https://golang.org/cl/347089 mentions this issue: |
I believe this is guaranteed to resolve to the
We need the function declaration AST only, that's why I can't use I think we just need to document that |
Change https://golang.org/cl/347530 mentions this issue: |
Left a comment on your CL; we can say something about what it means for Yes, you should be able to rely on identifiers being resolved to declarations within the function scope. |
Re-opening as we're discussing the documentation (on the CL above), and it still needs some improvement. |
Change https://golang.org/cl/347592 mentions this issue: |
This reverts commit 52aef05. Reason for revert: After discussion on CL 347530, it is not clear this is an improvement to the documentation. Updates #48141 Change-Id: I5f3d9995c5f5666b92602c4b8ec393673baa73fc Reviewed-on: https://go-review.googlesource.com/c/go/+/347592 Trust: Cherry Mui <cherryyz@google.com> Trust: Robert Findley <rfindley@google.com> Reviewed-by: Robert Findley <rfindley@google.com>
For function declared in other files, the identifier denoted function will be nil, cause the analysis panics. To fix this, we just skip that identifier for now, until golang/go#48141 is resolved. Fixes golang/go#48124 Change-Id: I87876505ee5964639ed3d1772d541c00d091ceb6 Reviewed-on: https://go-review.googlesource.com/c/tools/+/347089 Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Tim King <taking@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
See also #52463 |
Change https://go.dev/cl/504915 mentions this issue: |
The following declarations related to syntactic object resolution are now deprecated: - Ident.Obj - Object - Scope - File.{Scope,Unresolved} - Importer - Package, NewPackage New programs should use the type checker instead. Updates #52463 Updates #48141 Change-Id: I82b315f49b1341c11ae20dcbf81106084bd2ba86 Reviewed-on: https://go-review.googlesource.com/c/go/+/504915 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Findley <rfindley@google.com> Run-TryBot: Alan Donovan <adonovan@google.com>
@adonovan, hi! Question about
How to receive AST declaration from |
The If you're know you're looking for a package-level declaration (e.g. a global var, or a named function), you can iterate over File.Decls. Or you can use astutil.PathEnclosingInterval to find all enclosing nodes. (Internally, it walks down the tree.) It's more work, there's no denying it. P.S. I've long been meaning to add a faster version of PathEnclosingInterval that isn't as clever about whitespace and internal tokens (see the comments on that function to know what I'm talking about) as most callers don't need it, and it makes it slower. |
The nilness was documented by https://golang.org/cl/347592. The deprecation documentation in https://go.dev/cl/504915 included the advice to use go/types. Marking as complete. |
The following declarations related to syntactic object resolution are now deprecated: - Ident.Obj - Object - Scope - File.{Scope,Unresolved} - Importer - Package, NewPackage New programs should use the type checker instead. Updates golang/go#52463 Updates golang/go#48141 Change-Id: I82b315f49b1341c11ae20dcbf81106084bd2ba86 Reviewed-on: https://go-review.googlesource.com/c/go/+/504915 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Findley <rfindley@google.com> Run-TryBot: Alan Donovan <adonovan@google.com>
The go/ast.Ident is declared as:
Unfortunately, the doc is not much helpful, since when there's no description/example about when
Ident.Obj
is nil, and how to resolve it.It's better if we add more documentation to describe this situation.
cc @griesemer @findleyr @mdempsky @timothy-king
For more context, I end up filing this issue after getting the review from @timothy-king in CL 347089. The root cause of that issue is the function declaration of an ident and the ident itself are in two separated files of the same package. Thus the
Ident.Obj
is nil, and I'm looking through all thepass.Files
to lookup the ident's Object.The text was updated successfully, but these errors were encountered: