go/ast: formally deprecate Object #52463

mvdan · 2022-04-21T12:50:37Z

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.

cc @findleyr @griesemer @josharian @dominikh

The text was updated successfully, but these errors were encountered:

mvdan · 2022-04-21T12:56:15Z

One potential question is what to do with existing users of ast.Object. For example, cmd/gofmt uses 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 simplify s[:len(s)] into s[:]. I don't think we're necessarily in a rush to replace both of those bits of code.

timothy-king · 2022-04-22T21:56:37Z

Two others examples of existing users include x/tools/go/{ssa,cfg}. Both use the *ast.Objects to build control flow internally.

dominikh · 2022-04-22T22:16:18Z

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 · 2022-04-22T23:05:19Z

Change https://go.dev/cl/401875 mentions this issue: cmd/gofmt: use SkipObjectResolution with -s as well

mvdan · 2022-04-22T23:05:46Z

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 · 2022-04-25T00:16:45Z

Took a look at go/cfg. It does not depend on go/types yet. This would need a types.Info for the same solution to apply. The obvious fix is to pass a *types.Info which is a breaking change.

gopherbot · 2022-04-25T02:50:22Z

Change https://go.dev/cl/402034 mentions this issue: ssa: switch lblocks to types.Object

gopherbot · 2022-04-25T03:40:50Z

Change https://go.dev/cl/402054 mentions this issue: nilness: add unit test for generic instantiation.

gopherbot · 2022-04-25T04:10:53Z

Change https://go.dev/cl/402055 mentions this issue: ssautil: Add unit tests that set ssa.InstantiateGenerics

findleyr · 2022-04-25T21:09:29Z

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 SkipObjectResolution parser 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.

// ResolveFiles syntactically resolves identifiers in the package defined by files, recording their referenced declarations
// in the given decls map, if non-nil. The resulting error will wrap a scanner.ErrorList reporting errors encountered
// during resolution.
func ResolveFiles(files []*ast.File, decls map[*ast.Ident]ast.Node) error

CC @adonovan, who has been thinking about this problem recently.

Switches lblocks from *ast.Object to types.Object. Removes a user from the infrequently used *ast.Object. Updates golang/go#52463 Change-Id: I1a21ab55b7136f4891f6aa2f76459880ace389c9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/402034 Reviewed-by: Dominik Honnef <dominik@honnef.co> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Tim King <taking@google.com> Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>

Updates golang/go#52463 Updates golang/go#48525 Change-Id: I07cac2c9ad8d08a96cd14d4c729b14cf224d7f2e Reviewed-on: https://go-review.googlesource.com/c/tools/+/402055 Run-TryBot: Tim King <taking@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Tim King <taking@google.com> Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>

Adds a unit test for generic instantiation to nilness. Currently the expected behavior is to ignore the instantiation as it is not added to builssa's SrcFuncs result. Updates golang/go#52463 Updates golang/go#48525 Change-Id: I7b214aae88c8aa26605abb5019591178f76a7cbb Reviewed-on: https://go-review.googlesource.com/c/tools/+/402054 Run-TryBot: Tim King <taking@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Tim King <taking@google.com> Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com> Reviewed-by: Robert Findley <rfindley@google.com>

gopherbot · 2022-04-26T04:44:25Z

Change https://go.dev/cl/402276 mentions this issue: pointer: Adds unit tests for pointer with type parameters.

timothy-king · 2022-04-26T05:28:42Z

Please ignore messages for ssautil, nilness and cl/402276. These were attached to the wrong github issue.

mvdan · 2022-05-03T10:10:23Z

@findleyr that plan of action sounds good to me.

I have hesitated to use the SkipObjectResolution parser mode more widely, because I am wary of creating a world where we have different color syntax trees.

While that's a valid concern, I think it's more of an issue for general-purpose APIs like go/packages, where we don't know what the go/ast will be used for. The changes I have been making are much more limited, such as in cmd/gofmt or go/format, where we know for certain that the AST objects are unused and that will not change over time.

rsc · 2022-05-04T18:06:44Z

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

findleyr · 2022-05-18T17:31:29Z

We discussed this today in the external tools call. There seemed to be consensus that ast.Object should be deprecated, so I think it would be reasonable to proceed with this proposal even if we do not yet have a replacement API for syntactic object resolution. We will of course preserve backward compatibility, so people can continue to use ast.Object even if it is deprecated, but deprecation will make the current situation less confusing to new users.

rsc · 2022-05-18T18:00:14Z

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The "simplify" feature used go/ast's object tracking in only one place - to replace s[a:len(s)] with s[a:]. Using go/ast.Object did allow us to not simplify code like: len := func(s []int) int { ... } s = s[a:len(s)] The existing code already noted the limitation with that approach, such as "len" being redeclared in a different file in the same package. Since go/ast's object tracking is file-based and very basic, it wouldn't work with edge cases like those. The reasoning is that redeclaring len and abusing it that way is extremely unlikely, and hasn't been a problem in about a decade now. I reason that the same applies to len being redeclared in the same file, so we should be able to safely remove the use of go/ast.Object here. Per https://go.dev/cl/401454, this makes "gofmt -s" about 5% faster. If we ever wanted to truly get rid of false positive simplifications, I imagine we'd want to reimplement the feature under go/analysis, which is able to fully typecheck packages and suggest edits. That seems unnecessary at this point, but we can always course correct in the presumably unlikely scenario that users start reporting bugs. See #46485. For #52463. Change-Id: I77fc97adceafde8f0fe6887ace83ae325bfa7416 Reviewed-on: https://go-review.googlesource.com/c/go/+/401875 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Robert Griesemer <gri@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>

rsc · 2022-05-25T18:04:33Z

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

We don't use go/ast.Objects, as we use go/types instead. Avoiding tihs work saves a bit of CPU and memory allocs. name old time/op new time/op delta Build-16 10.2s ± 1% 10.2s ± 1% ~ (p=0.937 n=6+6) name old bin-B new bin-B delta Build-16 5.47M ± 0% 5.47M ± 0% ~ (all equal) name old cached-time/op new cached-time/op delta Build-16 328ms ±14% 321ms ± 6% ~ (p=0.589 n=6+6) name old mallocs/op new mallocs/op delta Build-16 34.8M ± 0% 34.0M ± 0% -2.26% (p=0.010 n=6+4) name old sys-time/op new sys-time/op delta Build-16 5.89s ± 3% 5.89s ± 3% ~ (p=0.937 n=6+6) See golang/go#52463.

We don't use go/ast.Objects. Avoiding this work saves a bit of CPU and memory allocs. See golang/go#52463.

We don't use go/ast.Objects, as we use go/types instead. Avoiding this work saves a bit of CPU and memory allocs. name old time/op new time/op delta Build-16 10.2s ± 1% 10.2s ± 1% ~ (p=0.937 n=6+6) name old bin-B new bin-B delta Build-16 5.47M ± 0% 5.47M ± 0% ~ (all equal) name old cached-time/op new cached-time/op delta Build-16 328ms ±14% 321ms ± 6% ~ (p=0.589 n=6+6) name old mallocs/op new mallocs/op delta Build-16 34.8M ± 0% 34.0M ± 0% -2.26% (p=0.010 n=6+4) name old sys-time/op new sys-time/op delta Build-16 5.89s ± 3% 5.89s ± 3% ~ (p=0.937 n=6+6) See golang/go#52463.

We don't use go/ast.Objects. Avoiding this work saves a bit of CPU and memory allocs. See golang/go#52463.

We don't use go/ast.Objects, as we use go/types instead. Avoiding this work saves a bit of CPU and memory allocs. name old time/op new time/op delta Build-16 10.2s ± 1% 10.2s ± 1% ~ (p=0.937 n=6+6) name old bin-B new bin-B delta Build-16 5.47M ± 0% 5.47M ± 0% ~ (all equal) name old cached-time/op new cached-time/op delta Build-16 328ms ±14% 321ms ± 6% ~ (p=0.589 n=6+6) name old mallocs/op new mallocs/op delta Build-16 34.8M ± 0% 34.0M ± 0% -2.26% (p=0.010 n=6+4) name old sys-time/op new sys-time/op delta Build-16 5.89s ± 3% 5.89s ± 3% ~ (p=0.937 n=6+6) See golang/go#52463.

gopherbot · 2023-06-21T17:53:50Z

Change https://go.dev/cl/504915 mentions this issue: go/ast: deprecate Object

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>

mvdan added Documentation Proposal Tools This label describes issues relating to any tools in the x/tools repository. labels Apr 21, 2022

gopherbot added this to the Proposal milestone Apr 21, 2022

ianlancetaylor added this to Incoming in Proposals (old) Apr 21, 2022

rsc moved this from Incoming to Active in Proposals (old) May 4, 2022

rsc moved this from Active to Likely Accept in Proposals (old) May 18, 2022

rsc added the Proposal-FinalCommentPeriod label May 18, 2022

rsc moved this from Likely Accept to Accepted in Proposals (old) May 25, 2022

rsc changed the title ~~proposal: go/ast: formally deprecate Object~~ go/ast: formally deprecate Object May 25, 2022

rsc added the Proposal-Accepted label May 25, 2022

rsc modified the milestones: Proposal, Backlog May 25, 2022

mvdan added a commit to mvdan/gofumpt that referenced this issue Jun 10, 2022

use go/parser.SkipObjectResolution

4c40b87

We don't use go/ast.Objects. Avoiding this work saves a bit of CPU and memory allocs. See golang/go#52463.

mvdan added a commit to mvdan/gofumpt that referenced this issue Jun 10, 2022

use go/parser.SkipObjectResolution

2a5d4bc

We don't use go/ast.Objects. Avoiding this work saves a bit of CPU and memory allocs. See golang/go#52463.

findleyr mentioned this issue Jun 24, 2022

go/ast,go/parser: Decl field of type parameter identifier is nil for methods (but not for functions) #50956

Open

mgnsk mentioned this issue Jul 29, 2022

unused-parameter marks unused parameter as used due to identical field in composite literal struct mgechev/revive#721

Closed

adonovan mentioned this issue Apr 23, 2023

go/ast: clarify when Ident.Obj is nil and how to find the ident's Obj #48141

Closed

rsc removed the Proposal-FinalCommentPeriod label Jul 26, 2023

adonovan closed this as completed Aug 7, 2023

adonovan mentioned this issue Mar 13, 2024

go/doc: NewFromFiles panics on ast.Files that lack ast.Objects due to parser.SkipObjectResolution #66290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go/ast: formally deprecate Object #52463

go/ast: formally deprecate Object #52463

mvdan commented Apr 21, 2022

mvdan commented Apr 21, 2022

timothy-king commented Apr 22, 2022

dominikh commented Apr 22, 2022

gopherbot commented Apr 22, 2022

mvdan commented Apr 22, 2022

timothy-king commented Apr 25, 2022

gopherbot commented Apr 25, 2022

gopherbot commented Apr 25, 2022

gopherbot commented Apr 25, 2022

findleyr commented Apr 25, 2022

gopherbot commented Apr 26, 2022

timothy-king commented Apr 26, 2022

mvdan commented May 3, 2022

rsc commented May 4, 2022

findleyr commented May 18, 2022

rsc commented May 18, 2022

rsc commented May 25, 2022

gopherbot commented Jun 21, 2023

go/ast: formally deprecate Object #52463

go/ast: formally deprecate Object #52463

Comments

mvdan commented Apr 21, 2022

mvdan commented Apr 21, 2022

timothy-king commented Apr 22, 2022

dominikh commented Apr 22, 2022

gopherbot commented Apr 22, 2022

mvdan commented Apr 22, 2022

timothy-king commented Apr 25, 2022

gopherbot commented Apr 25, 2022

gopherbot commented Apr 25, 2022

gopherbot commented Apr 25, 2022

findleyr commented Apr 25, 2022

gopherbot commented Apr 26, 2022

timothy-king commented Apr 26, 2022

mvdan commented May 3, 2022

rsc commented May 4, 2022

findleyr commented May 18, 2022

rsc commented May 18, 2022

rsc commented May 25, 2022

gopherbot commented Jun 21, 2023