proposal: go/types: add Scope.Node convenience getter #70645
Comments
|
We have the |
It is true, one can, but it is never available when you want it, and adding a complete pass over the AST when all you need is a cheap lookup is not the right thing to do.
If the Scopes map is populated, the Info already pins every ast.File in the package. Nothing would change. |
adonovan
changed the title
|
If we do this, perhaps do it only if |
In general I don't like "colors" of data structures that have more or less stuff filled in, and I was going to say that we have never encountered a real situation in which we wanted types.Packages produced from syntax to outlive that syntax, but @findleyr reminded me that gopls has just recently started doing this for the first time. So, I think we do need to have the invariant you propose. So: // Node returns the syntax node with which this scope is associated, and
// only if the Info.Scopes mapping was populated during type-checking of syntax.
// Invariant: s == Info.Scopes[n] <=> s.Node() == n.
func (*Scope) Node() ast.Node |
|
The suggestion in #70645 (comment) seems very similar to x/tools/go/ast/astutil.Inspector + x/tools/go/analysis/passes/inspector to me. Alan would a utility library + an analyzer cover your use cases well? When have you needed the getter? |
No. The library code would be trivial--invert the map--but it's the wrong algorithm to use at the place where you need it. This is orthogonal to the analysis framework.
Good question. The need has arisen often in refactoring tools (inline, extract, rename, etc), and in fact it came up again just this afternoon: see https://go.dev/633720, which could have been simplified as per the comment added in this CL if the feature had existed. However... that CL reminded me that there is always an annoying edge case, namely that the Scope for a FuncDecl or FuncLit is associated with the FuncType, and this would render the Scope to Node mapping less useful because there's no way to get from the FuncType to its parent Func{Decl,Lit}. So I think this may be less useful than I thought. I move to retract. |
Background:
types.Scoperepresents a block of the lexical environment, mapping names to symbols; Scopes form a tree. In refactoring tools there is a common and referring need to know, for a given symbol, what kind of block it is declared in, but unfortunately this information can currently only be obtained by a roundabout means (of inverting the Info.Scopes mapping).Given that only go/types constructs Scopes--the public
NewScopefunction is never used--and that Scopes are only used as an annotation on syntax trees (the importer doesn't construct scopes for package-level functions, for example), there is no substantial obstacle or cost to the type-checker recording theast.Nodein the Scope and returning it through a public accessor.In particular, this would not cause type information to prolong the lifetime of syntax nodes any more than necessary, since Scopes are currently only created when type-checking syntax.
A public
(*Scope).SetNodemethod would be symmetric, but give that NewScope is almost never used, it does not seem necessary.To be clear, this method is strictly a convenience as you can compute the inverse of the Info.Scopes mapping, but it is rarely convenient to do that: we have hundreds of functions that accept parameters such as (*types.Package, *types.Info, ast.Node) and plumb them down many levels, and either you would have to add a new parameter for this inverted scope mapping, or recompute it each time you need it.
Proposal: We propose to add the following method:
@findleyr @griesemer @timothy-king
The text was updated successfully, but these errors were encountered: