Repository navigation
go/ast: formally deprecate Object #52463
Description
https://pkg.go.dev/go/ast is the package declaring the syntax tree types for parsed Go code, where https://pkg.go.dev/go/parser produces them.
However, note that it also has a rather rudimentary form of typechecking, as it attempts to keep track of objects and scopes. However, this doesn't really work in practice, because go/ast and go/parser do not implement a proper Go typechecker; that's https://pkg.go.dev/go/types.
I find this to be very unfortunate in terms of confusing users, especially those new to Go tools and static analysis who are not aware of this historical gotcha. For example, I was just helping someone on Slack who was finding that objects were sometimes not resolved correctly, and the reason was the use of ast.Object rather than types.Object. See their code for context.
For some more context, we added a mode bit to go/parser to skip object resolution in #46485, bringing significant speedups at basically no cost for many programs, since they made no use of this partial object resolution.
We can't outright remove types and fields like ast.Object for backwards compatibility concerns, but we could certainly warn users against the use of ast.Object. For instance:
package ast
// [...]
//
// Deprecated: go/ast does not implement a full Go type checker; use go/types.Object instead.
type Object [...]
// [...]
//
// Deprecated: go/ast does not implement a full Go type checker; use go/types.Scope instead.
type Scope [...]
It is true that, potentially, using the partial object resolution in go/ast is fine for some use cases. Perhaps they only deal with very simple Go code that doesn't run into any of the limitations against go/types. I think it should be fine for those (hopefully very rare) cases to consciously ignore the deprecation notice.
Activity
mvdan commentedon Apr 21, 2022
One potential question is what to do with existing users of
ast.Object. For example,cmd/gofmtuses it for its rewrite feature (which arguably is deprecated in its own right, as it's rather limited and hasn't been developed in many years) and for its simplify feature, where it's only used to simplifys[:len(s)]intos[:]. I don't think we're necessarily in a rush to replace both of those bits of code.timothy-king commentedon Apr 22, 2022
Two others examples of existing users include
x/tools/go/{ssa,cfg}. Both use the *ast.Objects to build control flow internally.dominikh commentedon Apr 22, 2022
I've switched from ast.Object to types.Object in my fork of go/ssa and it was a trivial change. I can't speak to go/cfg, but I reckon it'll be roughly the same.
gopherbot commentedon Apr 22, 2022
Change https://go.dev/cl/401875 mentions this issue:
cmd/gofmt: use SkipObjectResolution with -s as wellmvdan commentedon Apr 22, 2022
The CL above removes the lone use in
gofmt -s, which was arguably unnecessary to begin with. Or rather, it only prevented simple forms of false positives, and the assumption was that there shouldn't be any false positives anyway.timothy-king commentedon Apr 25, 2022
Took a look at
go/cfg. It does not depend ongo/typesyet. This would need atypes.Infofor the same solution to apply. The obvious fix is to pass a*types.Infowhich is a breaking change.gopherbot commentedon Apr 25, 2022
Change https://go.dev/cl/402034 mentions this issue:
ssa: switch lblocks to types.Objectgopherbot commentedon Apr 25, 2022
Change https://go.dev/cl/402054 mentions this issue:
nilness: add unit test for generic instantiation.gopherbot commentedon Apr 25, 2022
Change https://go.dev/cl/402055 mentions this issue:
ssautil: Add unit tests that set ssa.InstantiateGenericsfindleyr commentedon Apr 25, 2022
This came up in #50956 as well. With the status-quo, users are likely to be confused about whether to use ast.Object, and then even more confused when they encounter a limitation of ast.Object. With that said, there are currently several valid uses of syntactic object resolution, such as in cmd/godoc, that we probably won't ever update to use go/types. This is of course OK even if we deprecate the API.
I have hesitated to use the
SkipObjectResolutionparser mode more widely, because I am wary of creating a world where we have different color syntax trees. Deprecating ast.Object would be a step toward skipping object resolution in more contexts. However I think we should consider first providing an API to do syntactic object resolution without relying on ast.Ident.Obj. This would allow existing valid usage of ast.Object to migrate off the API without the machinery of go/types.For example, something equivalent to the following would be trivial to implement based on the current factoring of go/parser, though the API requires some serious thought.
CC @adonovan, who has been thinking about this problem recently.
ssa: switch lblocks to types.Object
14 remaining items
use go/parser.SkipObjectResolution
use go/parser.SkipObjectResolution
use go/parser.SkipObjectResolution
use go/parser.SkipObjectResolution
use go/parser.SkipObjectResolution
gopherbot commentedon Jun 21, 2023
Change https://go.dev/cl/504915 mentions this issue:
go/ast: deprecate Objectgo/ast: deprecate Object