-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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/types: API changes to support explicit Alias nodes #63223
Comments
I would love this for tools - I have struggled with not being able to distinguish aliases before. The approach with the option sounds good to me. Is |
@mvdan The CL implements this proposal but the proposal needs to be discussed and accepted first. Once we're confident that the CL is ready we may submit it, with the API changes unexported (with a leading |
To be clear, I only mean in this issue body - the type is declared as |
@mvdan I fixed one typo. Not sure if there's others. |
This proposal looks good, and I'm excited for the simplifications this will enable in gopls. I have some questions about the details:
|
If you want more specifics, and fancy getting horrified by my workaround, see: https://github.com/burrowers/garble/blob/82834ace20257dcbb4a9bc9493cce6d69418f4b7/main.go#L1383-L1389 That entire chunk of code could be replaced by this new option and Alias type, as far as I can tell! |
Oh, are we sharing workarounds :)? This entire function could probably be replaced by |
Should there also be a helper function that combines |
Alternative idea: what if Object.Type() and Type.Underlying() continue to behave the same as today, but we add a method So like Then existing alias-unaware code continues working the same, while alias-aware code can always find the Alias type when it wants it, even if the Type value came from alias-unaware code. The only tricky thing would be aliases to Basic and Named. I know within the Go compiler (types1), we assume that defined types always have a unique type instance, so that we can do direct pointer comparisons on them (though we special case So potentially some go/types code would break if we duplicate Basic and Named types so they can store the Alias pointer. We could offer a GODEBUG knob to disable this behavior though. That would allow the problematic alias-unaware code to continue working, while alias-aware code would gracefully degrade to work without Alias information for Named and/or Basic and/or everything. |
@timothy-king can you elaborate on that? Wouldn't |
@mdempsky, that's an interesting idea: we'd still need an Alias node to represent aliases, but it wouldn't occur in
Pragmatically, I think it is a simpler change to introduce an |
It sounds like the exchange between @mdempsky and @rfindley concluded that the "implicit alias info" approach is not the right one – the breakage there seems subtler than the breakage of being explicit. Please correct me if I'm mistaken about that. Discussed with @cherrymui, @adonovan, and @griesemer. Inspired by @mdempsky's mention of GODEBUG, we think it makes sense to use a GODEBUG rather than the EnableAlias bool. The GODEBUG infrastructure provides a transition path after which, in a few years, analysis programs just use the current setup instead of every program in the Go ecosystem having to remember to set EnableAlias = true. Concretely, we would add to go/types:
And then also a new GODEBUG named gotypesalias. gotypesalias=0 means to not generate aliases, gotypesalias=1 means to generate aliases. For Go 1.22, the new API would be added and gotypesalias=0 would remain the default. For Go 1.23, the default would change to gotypesalias=1 (as usual, only in programs that are in a main module with "go 1.23" in the go.mod). This two-step transition means that programs supporting Go 1.21 and Go 1.22 ignore the new API, and programs supporting Go 1.22 and Go 1.23 use the new API, without needing build tags. There is one subtle case to consider about the interaction between go/types and compiler export data. In Go 1.23, if gotypesalias=1 is set, we want programs reading compiler export data to see aliases, same as if they type-checked themselves. That means the compiler must export alias information unconditionally. Either the gcimporter or go/types itself would need to de-alias the types as it reads them in. So the proposal is the new API above and the new GODEBUG gotypesalias defaulting to 0 in Go 1.22. If gotypesalias=1 then the Go 1.22 go/types package would create aliases. @griesemer wasn't sure whether the compiler would be able to create aliases in time for Go 1.22, so perhaps the export data won't have them in Go 1.22, in which case setting gotypesalias=1 but reading compiler export data would find no aliases. That's obviously not ideal but it's also fine, since for the purposes of Go 1.22, gotypesalias=1 can be considered an experimental preview of Go 1.23. |
FWIW, one potential way to implement the de-aliasing would be to make NewAlias return a Type instead of a *Alias, and then when gotypesalias=0, NewAlias(obj, rhs) returns rhs directly. I don't know whether that's a good idea but it seemed worth noting. |
Thanks. This sounds like a good step forward, and GODEBUG avoids the confusing definition for EnableAlias.
I'm unfamiliar with using GODEBUG, but presumably it should be possible to write a wrapper for NewAlias that does this. Therefore we should keep the more specific signature for Overall, the only objection I can think of for this proposal is that an
So, following this logic, I think adding an |
Definitely. I was just observing that if the check happened in NewAlias than any code that "created" aliases would be dealiased properly, so that all the potential creators of aliases (gcimporter, gccgoimporter, and go/types itself) wouldn't have to duplicate the logic. I'm happy not to do that, of course. |
Based on the discussion above, this proposal seems like a likely accept. Some new API is added to go/types to represent aliases.
Because returning Alias information may break existing type switches that do not know to check for the Alias type, this functionality will be controlled by a GODEBUG named “gotypesalias”. When gotypesalias=0, everything behaves as before, and aliases are never generated in the returned types. When gotypesalias=1, aliases are generated and code must expect them. The gotypesalias setting will also control whether aliases are returned by gcimporter and gccgoimporter. When the compilers start recording aliases in the export information, the importers will have to strip those aliases out when gotypeslias=0. For Go 1.22, gotypesalias=0 will be the default, and gotypesalias=1 may or may not be fully implemented. (It will probably produce alias info in go/types type-checking, but it may or may not produce aliases in gcimporter type-checking.) Code that needs to work with Go 1.21 and Go 1.22 can ignore gotypealias and these new types entirely. For Go 1.23, gotypealias=1 will become the default. Code that needs to work with Go 1.22 and Go 1.23 will have the types available for use in type switches, even though they won’t be generated in Go 1.22. This should avoid needing //go:build tags. |
Change https://go.dev/cl/541737 mentions this issue: |
This CL exports the previously unexported Alias type and corresponding functions and methods per issue #63223. Whether Alias types are used or not is controlled by the gotypesalias setting with the GODEBUG environment variable. Setting gotypesalias to "1" enables the Alias types: GODEBUG=gotypesalias=1 By default, gotypesalias is not set. Adjust test cases that enable/disable the use of Alias types to use -gotypesalias=1 or -gotypesalias=0 rather than -alias and -alias=false for consistency and to avoid confusion. For #63223. Change-Id: I51308cad3320981afac97dd8c6f6a416fdb0be55 Reviewed-on: https://go-review.googlesource.com/c/go/+/541737 Run-TryBot: Robert Griesemer <[email protected]> Reviewed-by: Robert Findley <[email protected]> Auto-Submit: Robert Griesemer <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
No change in consensus, so accepted. 🎉 Some new API is added to go/types to represent aliases.
Because returning Alias information may break existing type switches that do not know to check for the Alias type, this functionality will be controlled by a GODEBUG named “gotypesalias”. When gotypesalias=0, everything behaves as before, and aliases are never generated in the returned types. When gotypesalias=1, aliases are generated and code must expect them. The gotypesalias setting will also control whether aliases are returned by gcimporter and gccgoimporter. When the compilers start recording aliases in the export information, the importers will have to strip those aliases out when gotypeslias=0. For Go 1.22, gotypesalias=0 will be the default, and gotypesalias=1 may or may not be fully implemented. (It will probably produce alias info in go/types type-checking, but it may or may not produce aliases in gcimporter type-checking.) Code that needs to work with Go 1.21 and Go 1.22 can ignore gotypealias and these new types entirely. For Go 1.23, gotypealias=1 will become the default. Code that needs to work with Go 1.22 and Go 1.23 will have the types available for use in type switches, even though they won’t be generated in Go 1.22. This should avoid needing //go:build tags. |
Would love to hear what is the thinking for gcimporter in Go 1.22 when the development is a bit further ahead, since this will affect whether |
@mvdan gopls uses yet another fork (based on the index export format), which will need to be updated. Using this in gopls will at least give us some experience. |
Yes, I could have been clearer that that's what I meant. I think the bare minimum for go/packages to support explicit alias nodes in Go 1.22 would be go/types and the export data in the build cache from cmd/go. |
So it won't be usable with go/packages until 1.23 (at least not when using go/package's type checking). Perhaps we can update go/packages to type check from source if |
The go/types and types2 changes have been implemented and submitted. |
Understood, thank you @findleyr. And thanks @griesemer for opening the new issue - following :) |
The fact that we had https://pkg.go.dev/go/types@master#TypeName.IsAlias already, and now we've gained https://pkg.go.dev/go/types@master#Alias, makes me slightly uneasy. Looking at the docs with fresh eyes, I honestly couldn't even say when I should use one API over the other. Should we eventually deprecate the |
I'm sure that the docs can be improved. But IsAlias applies to a TypeName which is an Object. The Alias node represents the Alias type, which is not the same. I don't see a problem here. |
Change https://go.dev/cl/546358 mentions this issue: |
Also, add some missing <code></code> tags. For #63223. Change-Id: I570b82be830b3c124420c5715ab1165ca53725f9 Reviewed-on: https://go-review.googlesource.com/c/go/+/546358 Auto-Submit: Robert Griesemer <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Griesemer <[email protected]> TryBot-Bypass: Robert Griesemer <[email protected]>
Also, add some missing <code></code> tags. For golang#63223. Change-Id: I570b82be830b3c124420c5715ab1165ca53725f9 Reviewed-on: https://go-review.googlesource.com/c/go/+/546358 Auto-Submit: Robert Griesemer <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Robert Griesemer <[email protected]> TryBot-Bypass: Robert Griesemer <[email protected]>
Currently, the go/types.Type for a declared type alias is the same type as the that which it is aliased too. In Go 1.23, when GODEBUG=gotypesalias=1 is enabled by default, it is a go/types.Alias instead, which is a unique type node for aliases that points to the type it is aliased to via go/types.Unalias(). (see golang/go#63223 for more details). errcheck's tests do not currently fail with GODEBUG=gotypesalias=1, but that is because the tests themselves do not exercise the problematic situations, such as the following: ```go type t struct{} func (x t) a() error { /* ... */ } type embedt struct { t } type embedtalias = embedt func foo() { embedtalias.a() } ``` With GODEBUG=gotypesalias=1, errcheck will panic when inspecting `embedtalias.a()` to see if it should be excluded from checking: ``` panic: cannot get Field of a type that is not a struct, got _/home/user/go/src/github.com/kisielk/errcheck/errcheck/testdata/src/a.embedtalias (*types.Alias) goroutine 2962 [running]: github.com/kisielk/errcheck/errcheck.getTypeAtFieldIndex({0x7779c0?, 0xc002eceab0?}, 0x4c?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:96 +0xf9 github.com/kisielk/errcheck/errcheck.walkThroughEmbeddedInterfaces(0x6b83a0?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:53 +0x8e github.com/kisielk/errcheck/errcheck.(*visitor).namesForExcludeCheck(0xc0025eeff0, 0xc002596140?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:364 +0x165 github.com/kisielk/errcheck/errcheck.(*visitor).excludeCall(0xc0025eeff0, 0x4d?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:403 +0x72 github.com/kisielk/errcheck/errcheck.(*visitor).ignoreCall(0xc0025eeff0, 0xc002596140) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:415 +0x2c github.com/kisielk/errcheck/errcheck.(*visitor).Visit(0xc0025eeff0, {0x776d48?, 0xc0066e08a0}) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:564 +0x137 ``` This change uses `types.Unalias` to follow through this extra level of indirection, which fixes the panic and ensures the same behavior with both settings. This change also adds these cases to the test suite, and runs the tests using both settings of gotypesalias. Without the calls to `types.Unalias`, the tests fail.
Currently, the go/types.Type for a declared type alias is the same type as the that which it is aliased too. In Go 1.23, when GODEBUG=gotypesalias=1 is enabled by default, it is a go/types.Alias instead, which is a unique type node for aliases that points to the type it is aliased to via go/types.Unalias(). (see golang/go#63223 for more details). errcheck's tests do not currently fail with GODEBUG=gotypesalias=1, but that is because the tests themselves do not exercise the problematic situations, such as the following: ```go type t struct{} func (x t) a() error { /* ... */ } type embedt struct { t } type embedtalias = embedt func foo() { embedtalias.a() } ``` With GODEBUG=gotypesalias=1, errcheck will panic when inspecting `embedtalias.a()` to see if it should be excluded from checking: ``` panic: cannot get Field of a type that is not a struct, got _/home/user/go/src/github.com/kisielk/errcheck/errcheck/testdata/src/a.embedtalias (*types.Alias) goroutine 2962 [running]: github.com/kisielk/errcheck/errcheck.getTypeAtFieldIndex({0x7779c0?, 0xc002eceab0?}, 0x4c?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:96 +0xf9 github.com/kisielk/errcheck/errcheck.walkThroughEmbeddedInterfaces(0x6b83a0?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:53 +0x8e github.com/kisielk/errcheck/errcheck.(*visitor).namesForExcludeCheck(0xc0025eeff0, 0xc002596140?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:364 +0x165 github.com/kisielk/errcheck/errcheck.(*visitor).excludeCall(0xc0025eeff0, 0x4d?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:403 +0x72 github.com/kisielk/errcheck/errcheck.(*visitor).ignoreCall(0xc0025eeff0, 0xc002596140) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:415 +0x2c github.com/kisielk/errcheck/errcheck.(*visitor).Visit(0xc0025eeff0, {0x776d48?, 0xc0066e08a0}) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:564 +0x137 ``` This change uses `types.Unalias` to follow through this extra level of indirection, which fixes the panic and ensures the same behavior with both settings. This change also adds these cases to the test suite, and runs the tests using both settings of gotypesalias. Without the calls to `types.Unalias`, the tests fail.
Currently, the go/types.Type for a declared type alias is the same type as the that which it is aliased too. In Go 1.23, when GODEBUG=gotypesalias=1 is enabled by default, it is a go/types.Alias instead, which is a unique type node for aliases that points to the type it is aliased to via go/types.Unalias(). (see golang/go#63223 for more details). errcheck's tests do not currently fail with GODEBUG=gotypesalias=1, but that is because the tests themselves do not exercise the problematic situations, such as the following: ```go type t struct{} func (x t) a() error { /* ... */ } type embedt struct { t } type embedtalias = embedt func foo() { embedtalias.a() } ``` With GODEBUG=gotypesalias=1, errcheck will panic when inspecting `embedtalias.a()` to see if it should be excluded from checking: ``` panic: cannot get Field of a type that is not a struct, got _/home/user/go/src/github.com/kisielk/errcheck/errcheck/testdata/src/a.embedtalias (*types.Alias) goroutine 2962 [running]: github.com/kisielk/errcheck/errcheck.getTypeAtFieldIndex({0x7779c0?, 0xc002eceab0?}, 0x4c?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:96 +0xf9 github.com/kisielk/errcheck/errcheck.walkThroughEmbeddedInterfaces(0x6b83a0?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/embedded_walker.go:53 +0x8e github.com/kisielk/errcheck/errcheck.(*visitor).namesForExcludeCheck(0xc0025eeff0, 0xc002596140?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:364 +0x165 github.com/kisielk/errcheck/errcheck.(*visitor).excludeCall(0xc0025eeff0, 0x4d?) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:403 +0x72 github.com/kisielk/errcheck/errcheck.(*visitor).ignoreCall(0xc0025eeff0, 0xc002596140) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:415 +0x2c github.com/kisielk/errcheck/errcheck.(*visitor).Visit(0xc0025eeff0, {0x776d48?, 0xc0066e08a0}) /home/user/go/src/github.com/kisielk/errcheck/errcheck/errcheck.go:564 +0x137 ``` This change uses `types.Unalias` to follow through this extra level of indirection, which fixes the panic and ensures the same behavior with both settings. This change also adds these cases to the test suite, and runs the tests using both settings of gotypesalias. Without the calls to `types.Unalias`, the tests fail.
Background
Currently, when a type alias declaration
is type-checked,
go/types
creates aTypeName
Object
for the type aliasA
but the type of theTypeName
is the type ofsome_type
. WhenA
is used as identifier in source code, the denoted type issome_type
, which if it's a defined type will have a different name thanA
; and if it's not a defined type, it will be a type literal, which is also not calledA
. As a consequence, an error message related to the typeA
will not reportA
butsome_type
or whatever typeA
is a alias for. This is a long-standing issue that we'd like to fix.Furthermore, with proposal #46477 accepted (parameterized type aliases), currently
go/types
doesn't have a way to represent a parameterized alias type.Both these problems can be addressed with an explicit
Alias
type node. This proposal is about introducing such a new type node.Proposal
We introduce a new
go/types
node calledAlias
:An
Alias
node stands for a type alias. AnAlias
node is created with a factory function:When an
Alias
node is encountered,go/types
and related tool clients may need to indirect to the actual type theAlias
node is a type alias for. This can be done through a new functionTools that care about the actual un-aliased type will need to call
Unalias
everywhere in the tools code where there is a type assertion or type switch and the type may be an alias type.This is a non-backward compatible change to
go/types
and it may require pervasive changes to tools, with a potentially long bug tail. For that reason we also propose to introduce a newtypes.Config
flag:Unless
EnableAlias
is set, go/types behaves exactly as before. IfEnableAlias
is set,Alias
nodes appear for type alias declarations, and clients ofgo/types
will need to be aware of that.Discussion
This proposal only introduces the
Alias
node functionality, without support for type parameters. We believe this is a significant change which should be done separately from support for parameterized alias types, ideally for Go 1.22. Once the new API is settled and clients have adjusted, we would then extend theAlias
node with support for type parameters and instantiation, presumably for Go 1.23. As that step doesn't introduce a new type node, we expect it to be easier to adjust clients.Unfortunately we don't see a way around introducing an explicit
Alias
node if we want to support parameterization, and therefore we don't see a way to support parameterized alias types in a backward-compatible way. TheEnableAlias
configuration flag should help ease the transition: tools will continue to work as before if this proposal is accepted. Only when they set the configuration flag will they need to be aware of the new type node.Implementation
An initial implementation can be found here: CL 521956 with unexported API changes.
The compiler doesn't set
EnableAlias
yet. Also, changes to the compiler (calls toUnalias
) and exporters/importers are missing.The text was updated successfully, but these errors were encountered: