Skip to content
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.

Sign up for GitHub

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

Jump to bottom

x/tools: get tests passing with GODEBUG=gotypesalias=1 #65294

Closed
13 tasks done
findleyr opened this issue Jan 25, 2024 · 30 comments
Closed
13 tasks done

x/tools: get tests passing with GODEBUG=gotypesalias=1 #65294

findleyr opened this issue Jan 25, 2024 · 30 comments
Assignees
@timothy-king @adonovan @findleyr
Labels
NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@findleyr
Copy link
Contributor

findleyr commented Jan 25, 2024 •
edited by adonovan

We need to get x/tools tests passing with GODEBUG=gotypesalias=1 (which enables explicit nodes for type aliases) before we can change the default value of this flag (x/tools test run as a TryBot for the main repo, and in any case we can't break x/tools...)

Therefore, this is somewhat time-sensitive. @adonovan, @timothy-king, and I can collaborate on this. We should divvy up packages.

Possibly incomplete list of packages that fail:

[Edit: the checkboxes above are such a tiny sample of the tip of the iceberg that they paint a misleading picture of the work involved --adonovan]

Note that this change only addresses behavior preservation. Additional changes are needed where explicit aliases (and, later, generic aliases) require either new features or new algorithms. The most obvious candidates are type import and export, and gopls features.

Related:

CC @griesemer @mdempsky
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 25, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 25, 2024
@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 26, 2024
@gopherbot
Copy link

Change https://go.dev/cl/559915 mentions this issue: go/types/typeutil: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559935 mentions this issue: internal/apidiff: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559936 mentions this issue: cmd/deadcode: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559916 mentions this issue: go/analysis/passes/deepequalerrors: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559917 mentions this issue: go/analysis/passes/stringintconv: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559937 mentions this issue: go/callgraph/rta: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/559995 mentions this issue: internal/aliases: Adds an internal alias package.

@findleyr findleyr mentioned this issue Feb 1, 2024
Closed
gopherbot pushed a commit to golang/tools that referenced this issue Feb 5, 2024
@timothy-king
internal/aliases: Adds an internal alias package.
0be034b 
Adds a transitional package for handling types.Alias
until GoVersion>=1.26 for x/tools.

Updates golang/go#65294

Change-Id: I7a58cb9ceb9945529baf14d33543dbebc23af542
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559995
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
@adonovan
internal/apidiff: audit for types.Alias safety
37586e4 
Updates golang/go#65294

Change-Id: I0767c09e277a2225657dcf87e7b41d664c9da1bb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559935
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
@adonovan
cmd/deadcode: audit for types.Alias safety
08393e0 
Updates golang/go#65294

Change-Id: Ica0197fd5d931244079fdf5b8c29f1bfeed5083b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559936
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
@adonovan
go/callgraph/rta: audit for types.Alias safety
a297bfd 
Updates golang/go#65294

Change-Id: I90fe0cc3ab5a6171e112e2eb0e452efb172d9980
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559937
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/562036 mentions this issue: internal/refactor/inline: audit for types.Alias safety

@gopherbot
Copy link

Change https://go.dev/cl/562037 mentions this issue: gopls/doc: audit for types.Alias safety

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
@adonovan
go/types/typeutil: audit for types.Alias safety
0d87589 
Updates golang/go#65294

Change-Id: I921517b9c722d03aaa7c3dc3e0c45364b3a1d53d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559915
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2024
@adonovan
internal/refactor/inline: audit for types.Alias safety
d64ed6a 
Updates golang/go#65294

Change-Id: I14f7d06a0e41799238707b20a88205ae1bfc1ce8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562036
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 7, 2024
@adonovan
gopls/doc: audit for types.Alias safety
76795ef 
Updates golang/go#65294

Change-Id: Ie4a873d5953e495924fc5b6691dc8e43b6917bb0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562037
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 7, 2024
@adonovan
go/analysis/passes/stringintconv: audit for types.Alias safety
7f80389 
Updates golang/go#65294

Change-Id: Idfb583f0d3ad3753b770946cb9b9360625670d0d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559917
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 8, 2024
@adonovan @gopherbot
go/analysis/passes/deepequalerrors: audit for types.Alias safety
314368d 
Updates golang/go#65294

Change-Id: I00543b00c830ff5a4fe442f1bcf6f21ab0b12d97
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559916
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/565035 mentions this issue: x/tools: add explicit Unalias operations

@gopherbot
Copy link

Change https://go.dev/cl/565075 mentions this issue: internal/typesinternal: add ReceiverNamed helper

@gopherbot
Copy link

Change https://go.dev/cl/565476 mentions this issue: gopls: rationalize "deref" helpers

@gopherbot
Copy link

Change https://go.dev/cl/565456 mentions this issue: internal/typesparams: add Deref

gopherbot pushed a commit to golang/tools that referenced this issue Feb 21, 2024
@adonovan @gopherbot
internal/typesinternal: add ReceiverNamed helper
c111c4d 
...and factor numerous places to use it.

(This pattern kept recurring during my types.Alias audit,
golang/go#65294.)

Change-Id: I93228b735f7a8ff70df5c998017437d43742d9f3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565075
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit that referenced this issue Apr 2, 2024
@adonovan @gopherbot
cmd: update vendored x/tools to 904c6ba
3b29222 
This fixes one of the many obstacles to enabling
types.Alias by updating vet.

The 'loopclosure' checker (formerly 'rangeloop') no longer
reports any findings with go1.22, so the test needed work
to ensure that it still runs on files with go1.21 semantics.

Updates #65294

Change-Id: I987ff529d4e165b56b7241563c6003e63bf92bb1
Reviewed-on: https://go-review.googlesource.com/c/go/+/575315
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/575698 mentions this issue: gopls/internal/test: temporarily disable staticcheck with gotypesalias=1

@gopherbot
Copy link

Change https://go.dev/cl/575699 mentions this issue: internal/analysisinternal: ZeroValue support materialized aliases

gopherbot pushed a commit to golang/tools that referenced this issue Apr 2, 2024
@adonovan
gopls/internal/test: temporarily disable staticcheck with gotypesalias=1
85b6527 
staticcheck will need work to accommodate gotypesalias=1.
In the meantime we disable this test in that mode; we plan
to reenable it before the next gopls release.

Updates golang/go#65294

Change-Id: I857edb758f83f180a4d92b7100e99c3401b1d957
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575698
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 2, 2024
@adonovan
internal/gcimporter: support materialized aliases
0a4fc72 
This CL adds support for preserving materialized aliases
while types transit export data (indexed or unified).

(This CL depends on CL 574737 to the compiler and
go/types.)

Updates golang/go#65294
Updates golang/go#64581

Change-Id: I1a0a08357e4f6a480ba6250fbea327922e455873
Reviewed-on: https://go-review.googlesource.com/c/tools/+/574717
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 2, 2024
@adonovan @gopherbot
internal/analysisinternal: ZeroValue: support materialized aliases
222cdde 
My previous audit of this file contained a mistake in the order
in which constructors were tested; it was revealed by testing
with gotypesalias=1.

Updates golang/go#65294

Change-Id: I80f6f0bb24bceb2c69b9919e9c482b780c0d546a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575699
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/576137 mentions this issue: gopls/internal/analysis/fillreturns: skip test if gotypesalias=1

@gopherbot
Copy link

Change https://go.dev/cl/576139 mentions this issue: internal/gcimporter: renable tests of issue50259.go

gopherbot pushed a commit to golang/tools that referenced this issue Apr 3, 2024
@adonovan @gopherbot
gopls/internal/analysis/fillreturns: skip test if gotypesalias=1
c9b0c65 
It changes (improves) the behavior of the test; we will reenable
it once the default has changed.

Updates golang/go#65294

Change-Id: I716da405a9f0c03c303c4c0be8b738dd7c5ebdcd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576137
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Apr 4, 2024
@adonovan @gopherbot
internal/gcimporter: renable tests of issue50259.go
6590f47 
Now that the compiler support for gotypesalias=1 is
fixed (by CL 574737), we can reenable these tests.

Updates golang/go#66550
Updates golang/go#65294

Change-Id: I70b187c561d2eeb3f7a4cc078107b7bcce5ced01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576139
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit that referenced this issue Apr 5, 2024
@adonovan @gopherbot
cmd/compile: export/import materialized aliases
b24ec88 
This CL changes the compiler's type import/export logic
to create and preserve materialized Alias types
when GODEBUG=gotypesaliases=1.

In conjunction with CL 574717, it allows the x/tools
tests to pass with GODEBUG=gotypesaliases=1.

Updates #65294
Updates #64581
Fixes #66550

Change-Id: I70b9279f4e0ae7a1f95ad153c4e6909a878915a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/574737
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/576975 mentions this issue: go/types: fix bug in premature Unalias of alias cycle

gopherbot pushed a commit that referenced this issue Apr 5, 2024
@adonovan @gopherbot
go/types: fix bug in premature Unalias of alias cycle
d186dde 
Unalias memoizes the result of removing Alias constructors.
When Unalias is called too soon on a type in a cycle,
the initial value of the alias, Invalid, gets latched by
the memoization, causing it to appear Invalid forever.

This change disables memoization of Invalid, and adds
a regression test.

Fixes #66704
Updates #65294

Change-Id: I479fe14c88c802504a69f177869f091656489cd4
Reviewed-on: https://go-review.googlesource.com/c/go/+/576975
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/577576 mentions this issue: internal/apidiff: support materialized aliases

gopherbot pushed a commit to golang/tools that referenced this issue Apr 9, 2024
@adonovan
internal/apidiff: support materialized aliases
b4b8186 
(The previous CL 559935 was insufficient.)

Also, improve test output.

Updates golang/go#65294

Change-Id: I05cafadce0dd6f18ff66d2ca462a3eb546c4ca81
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577576
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/577616 mentions this issue: cmd/compile/internal/types2: revert to three-phase alias resolution

@adonovan
Copy link
Member

adonovan commented Apr 9, 2024

Phew.

@adonovan adonovan closed this as completed Apr 9, 2024
gopherbot pushed a commit that referenced this issue Apr 9, 2024
@adonovan
cmd/compile/internal/types2: revert to three-phase alias resolution
a555ac6 
This change reenables the legacy three-phase resolution
(non-alias typenames, aliases, the rest) even when
GODEBUG=gotypesalias=1. Unfortunately the existing test case
for #50259 causes the simpler logic to fail.

Updates #50259
Updates #65294

Change-Id: Ibfaf8146e46760718673a916a9b220a9d678409a
Reviewed-on: https://go-review.googlesource.com/c/go/+/577616
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

Change https://go.dev/cl/578041 mentions this issue: x/tools: make tests agnostic as to whether gotypesalias="" => 0 or 1

gopherbot pushed a commit to golang/tools that referenced this issue Apr 12, 2024
@adonovan @gopherbot
x/tools: make tests agnostic as to whether gotypesalias="" => 0 or 1
dcccb2d 
This is required temporarily as we flip the default.

Updates golang/go#65294

Change-Id: I552e40475cc48b949e2307af347ca98a428c55ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578041
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/578324 mentions this issue: doc: enforce gotypesalias=0 behavior until go1.23

gopherbot pushed a commit to golang/tools that referenced this issue Apr 15, 2024
@adonovan @gopherbot
doc: enforce gotypesalias=0 behavior until go1.23
0c3722a 
Later, we can switch to using aliases, which produces
nicer output.

Updates golang/go#65294

Change-Id: I1d807651cccf52961779cf66e057f859f2bb3a05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578324
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/579015 mentions this issue: cmd/compile/internal/types2: port CL 576975 to types2

gopherbot pushed a commit that referenced this issue Apr 15, 2024
@adonovan @gopherbot
cmd/compile/internal/types2: port CL 576975 to types2
cfbe6cd 
This CL ports to types2 the (passing) test from CL 576975,
which fixed a bug in go/types.

Updates #66704
Updates #65294

Change-Id: Icdf77e39ed177d9f9ecc435d5125f02f2ee4dd0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/579015
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/579075 mentions this issue: go/types, types2: simplify TestUnaliasTooSoonInCycle (cleanup)

gopherbot pushed a commit that referenced this issue Apr 15, 2024
@griesemer @gopherbot
go/types, types2: simplify TestUnaliasTooSoonInCycle (cleanup)
7418d41 
Follow-up on CL 576975 and CL 579015.

Updates #66704
Updates #65294

Change-Id: Ied95386a346be38ccda86d332d09b2089a68c5e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/579075
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/579298 mentions this issue: go/types: renable 2x execution of tests with gotypesalias={0,1}

This was referenced Apr 19, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timothy-king timothy-king

@adonovan adonovan

@findleyr findleyr

Labels
NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@timothy-king @adonovan @gopherbot @cherrymui @findleyr