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.

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

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

Open
13 tasks done
findleyr opened this issue Jan 25, 2024 · 14 comments
Open
13 tasks done

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

findleyr opened this issue Jan 25, 2024 · 14 comments
Assignees
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

Comments

@findleyr
Copy link
Contributor

findleyr commented Jan 25, 2024

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:

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.

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.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 5, 2024
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
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
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
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
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
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
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
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
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
...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 to golang/tools that referenced this issue Feb 22, 2024
This change (part of the types.Alias audit) defines two
helper functions for dealing with pointer types:

1. internal/typeparams.MustDeref (formerly go/ssa.mustDeref)
   is the type equivalent of a LOAD instruction.
2. internal/typesinternal.Unpointer strips off a
   Pointer constructor (possibly wrapped in an Alias).

The golang.Deref function, which recursively strips off
Pointer and Named constructors, is a meaningless
operation. It has been replaced in all cases by
Unpointer + Unalias.

There are far too many functions called 'deref' with
subtle variations in their behavior. I plan to
standardize x/tools on few common idioms.
(There is more to do.)

Updates golang/go#65294

Change-Id: I502bab95e8d954715784b7e35ec801f4be4bc959
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565476
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 28, 2024
This change is the result of an audit of all type assertions
and type switches whose operand is a types.Type. (These were
enumerated with an analyzer tool.)

If the operand is already the result of a call to Underlying
or Unalias, there is nothing to do, but in other cases,
explicit Unalias operations were added in order to preserve
the existing behavior when go/types starts creating explicit
Alias types.

This change does not address any desired behavior changes
required for the ideal handling of aliases; they will wait
for a followup. In a number of places I have added comments
matching "TODO.*alias".

It may be prudent to split this change by top-level directory,
both for ease of review, and of later bisection if needed.

During the audit, there appeared to be a recurring need
for the following operators:
- (*types.Func).Signature (golang/go#65772);
- Deref(Type): it's easy to forget to strip off the
  Alias constructor;
- ReceiverName (CL 565075), for destructuring receiver
  types such as T and *T, in which up to two Aliases
  might be present.

Updates golang/go#65294

Change-Id: I5180b9bae1c9191807026b8e0dc6f15ed4953b9a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565035
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/567842 mentions this issue: refactor/rename: fix renaming of aliases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

5 participants