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

all: audit x/ repos for gotypesalias settings #69772

Closed
timothy-king opened this issue Oct 3, 2024 · 14 comments
Closed

all: audit x/ repos for gotypesalias settings #69772

timothy-king opened this issue Oct 3, 2024 · 14 comments
Assignees
Milestone

Comments

@timothy-king
Copy link
Contributor

This item is to track auditing and then updating all of the x/ repos for correctly setting the gotypesalias GODEBUG setting in preparation for 1.24. main packages in 1.22 modules (e.g. x/tools/cmd/callgraph) will default to the value of gotypesalias=0. If these packages use go/types and are built with a 1.24 toolchain, they will have gotypesalias=0. Such binaries will not create the *types.Alias value that is required to create an alias with type parameters (#46477). Effectively any such binaries would be unable to properly support 1.24 despite using the 1.24 compiler and standard library.

My proposed plan is to:

  1. Find all of the modules in the x/ repos, find all of the main packages within the modules that are <=1.22, and among these main packages determine which transitively import go/types.
  2. For all of these packages explicitly label (or comment on) their current status explicitly //go:debug gotypesalias=0. (See https://go.dev/cl/617095 for an example.)
  3. Audit each //go:debug gotypesalias=0 for what to do. A good default action is to update these packages to enable GODEBUG=gotypesalias=1 when built with >=1.23 toolchains (see https://go.dev/cl/617636).

cc @aclements , @cherrymui , @findleyr, @griesemer , @adonovan , @dmitshur

@timothy-king timothy-king added this to the Go1.24 milestone Oct 3, 2024
@timothy-king timothy-king self-assigned this Oct 3, 2024
@timothy-king
Copy link
Contributor Author

timothy-king commented Oct 3, 2024

Go repos to audit:

  • golang.org/x/arch
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/benchmarks
    • ./go.mod @ 1.21 (no go/types usage by main packages)
  • blog (no go code)
  • golang.org/x/build
    • ./go.mod @ 1.22.0 (no go/types usage by main packages)
    • ./maintner/cmd/maintserve/go.mod @ 1.20 (no go/types usage by main packages)
  • golang.org/x/crypto
    • ./go.mod @ 1.20 (no go/types usage by main packages)
    • ./blake2b/_asm/AVX2/go.mod @ 1.23 (no go/types usage by main packages)
    • ./blake2b/_asm/standard/go.mod @ 1.23 (no go/types usage by main packages)
    • ./sha3/_asm/go.mod @ 1.22 (no go/types usage by main packages)
    • ./argon2/_asm/go.mod @ 1.23 (no go/types usage by main packages)
    • ./internal/poly1305/_asm/go.mod @ 1.23 (no go/types usage by main packages)
    • ./blake2s/_asm/go.mod @ 1.23 (no go/types usage by main packages)
    • ./salsa20/salsa/_asm/go.mod @ 1.23 (no go/types usage by main packages)
    • ./x509roots/fallback/go.mod @ 1.20 (no go/types usage by main packages)
    • ./chacha20poly1305/_asm/go.mod @ 1.23 (no go/types usage by main packages)
  • golang.org/x/debug (excluding ./internal/gocore/testdata)
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/dl
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/example
    • ./go.mod @ 1.18 (Filed https://go.dev/issue/70696 to complete this. audit is done.)
      • ./gotypes/defsuses (illustrative example only)
      • ./gotypes/doc
      • ./gotypes/hugeparam
      • ./gotypes/implements (illustrative example only)
      • ./gotypes/lookup (illustrative example only)
      • ./gotypes/nilfunc (illustrative example only)
      • ./gotypes/pkginfo (illustrative example only)
      • ./gotypes/skeleton
      • ./gotypes/typeandvalue (illustrative example only)
    • ./helloserver/go.mod @ 1.19 (no go/types usage by main packages)
    • ./ragserver/ragserver/go.mod @ 1.23.0 (no go/types usage by main packages)
    • ./ragserver/ragserver-langchaingo/go.mod @ 1.23.0 (no go/types usage by main packages)
    • ./ragserver/ragserver-genkit/go.mod @ 1.23.0 (no go/types usage by main packages)
    • ./hello/go.mod @ 1.19 (no go/types usage by main packages)
    • ./outyet/go.mod @ 1.19 (no go/types usage by main packages)
  • golang.org/x/exp (excluding ./slog/benchmarks)
    • ./go.mod @ 1.22.0 (Filed https://go.dev/issue/70695 to complete this. Audit is done)
      • cmd/apidiff
      • cmd/gorelease
    • ./sumdb/go.mod @ 1.12 (no go/types usage by main packages)
    • ./shiny/go.mod @ 1.18 (no go/types usage by main packages)
    • ./typeparams/go.mod @ 1.18 [by manual inspection]
      • ./example/findtypeparams
      • ./example/genericmethods
      • ./example/implicit
      • ./example/instantiation
      • ./example/interfaces
      • ./example/methoddecls
      • ./example/predicates
      • ./example/typesets
    • ./jsonrpc2/go.mod @ 1.18 (no go/types usage by main packages)
    • ./errors/go.mod @ no version given (no go/types usage by main packages)
    • ./event/go.mod @ 1.18 (no go/types usage by main packages)
  • github.com/golang/gddo
    • ./go/mod @ 1.13 (no go/types usage by main packages)
  • gofrontend (assessed by @ianlancetaylor )
  • gollvm
    • no go.mod
    • ./tools/capture-fcn-attributes.go [by manual inspection]
    • ./unittests/remaster-tests.go [by manual inspection]
  • govulncheck-action (no go code)
  • grpc-review
    • No go.mod file. Leaving as out of scope.
  • golang.org/x/image
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/lint (repo is frozen. no action needed)
    • ./go.mod @ 1.11
      • golint
  • golang.org/x/mobile
    • ./go.mod @ 1.22.0 (filed https://go.dev/issue/70698. audit is done)
      • cmd/gobind
      • gomobile
    • ./example/ivy/go.mod @ 1.17 (no go/types usage by main packages)
  • golang.org/x/mod
    • ./go.mod @ 1.22.0 (no go/types usage by main packages)
  • golang.org/x/net
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/oauth2
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/oscar
    • ./go.mod @ 1.23 (no go/types usage by main packages)
    • ./internal/gcp/go.mod @ 1.23 (no go/types usage by main packages)
    • ./internal/devtools/go.mod @ 1.23 (no go/types usage by main packages)
    • ./internal/syncdb/go.mod @ 1.23 (no go/types usage by main packages)
    • ./internal/gaby/go.mod @ 1.23 (no go/types usage by main packages)
  • golang.org/x/perf
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/pkgsite (excluding ./internal/fetch/testdata)
    • ./go.mod @ 1.23 (no go/types usage by main packages)
  • golang.org/x/pkgsite-metrics (excluding ./internal/testdata and ./internal/worker/testdata)
    • ./go.mod @ 1.22.0 (no go/types usage by main packages)
  • golang.org/x/playground
    • ./go.mod @ 1.22.0 [by manual inspection]
      • cache.go [see notes in comment below]
  • proposal (no go code)
  • google.golang.org/protobuf
    • ./go.mod @ 1.21
      • cmd/protoc-gen-go [by manual inspection]
      • internal/cmd/generate-protos [by manual inspection]
  • golang.org/x/review
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • scratch (out of scope)
  • sublime-build (no go code)
  • sublime-config (no go code)
  • golang.org/x/sync
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/sys
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • talks (no go code)
  • golang.org/x/telemetry
    • ./go.mod @ 1.22.0 (no go/types usage by main packages)
    • ./config/go.mod @ 1.21 (no go/types usage by main packages)
    • ./godev/go.mod @ 1.22.0 (no go/types usage by main packages)
  • golang.org/x/term
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/text (filed https://go.dev/issue/70697. audit is done)
    • ./go.mod @ 1.18
      • cmd/gotext
  • golang.org/x/time
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • golang.org/x/tools
    • go.mod @ 1.22.0
      • cmd/bundle
      • cmd/callgraph
      • cmd/deadcode
      • cmd/eg
      • cmd/godex
      • cmd/godoc
      • cmd/goimports
      • cmd/gomvpkg
      • cmd/gotype
      • cmd/ssadump
      • cmd/stringer
      • go/analysis/passes/defers/cmd/defers
      • go/analysis/passes/fieldalignment/cmd/fieldalignment
      • go/analysis/passes/findcall/cmd/findcall
      • go/analysis/passes/httpmux/cmd/httpmux
      • go/analysis/passes/ifaceassert/cmd/ifaceassert
      • go/analysis/passes/lostcancel/cmd/lostcancel
      • go/analysis/passes/nilness/cmd/nilness
      • go/analysis/passes/shadow/cmd/shadow
      • go/analysis/passes/stringintconv/cmd/stringintconv
      • go/analysis/passes/unmarshal/cmd/unmarshal
      • go/analysis/passes/unusedresult/cmd/unusedresult
      • go/packages/gopackages
      • go/packages/internal/nodecount
      • go/types/internal/play
    • ./gopls/go.mod @ 1.23.1
    • ./gopls/doc/assets/go.mod @ go 1.19 (no go code)
  • golang.org/x/tour @ 1.18
    • ./go.mod @ 1.18 (no go/types usage by main packages)
  • vgo
    • ./go.mod @ 1.18 (out of scope. requires patching)
  • vscode-go (not including ./extension/test/testdata)
    • ./go.mod @ 1.21 (no go/types usage by main packages)
    • ./extension/go.mod @ 1.21 (no go/types usage by main packages)
    • ./docs/go.mod @ 1.21 (no go code)
  • golang.org/x/vuln (not including ./cmd/govulncheck/testdata)
    • ./go.mod @ 1.22.0
      • x/vuln/cmd/govulncheck
  • golang.org/x/vulndb
    • ./go.mod @ 1.22.0 [updated to 1.23]
      • x/vulndb/cmd/issue
      • x/vulndb/cmd/vulnreport
      • x/vulndb/cmd/worker
  • golang.org/x/website
    • ./go.mod @ 1.22.0 [see comment below]
      • x/website/cmd/golangorg [by manual inspection]
      • x/website/tour [by manual inspection]
  • wiki (no go code)
  • golang.org/x/xerrors
    • ./gomod @ 1.18 (no go/types usage by main packages)

@dmitshur
Copy link
Contributor

dmitshur commented Oct 3, 2024

Gaby didn't include it in #69772 (comment), but issue #67791 is also related here. If there are tests in x/ repos that would fail when switching to the eventual new default of gotypesalias=1, that builder would report it.

@timothy-king
Copy link
Contributor Author

Excluding gofrontend, there are 60 matches and out of these 25 are in x/tools. x/{tools,vuln,vulndb} are fairly easy to address. The rest give me some pause though. The right thing to do here might just be to update go/types to avoid this.

@findleyr
Copy link
Member

findleyr commented Oct 8, 2024

The rest give me some pause though. The right thing to do here might just be to update go/types to avoid this.

Can you say more? You mean back porting additional logic to 1.22.x?
IMO it looks pretty straightforward to apply the same mechanical fix in https://go.dev/cl/618676 to each binary target (ideally in a single CL or handful of CLs).

@timothy-king
Copy link
Contributor Author

timothy-king commented Oct 8, 2024

Can you say more?

Sure. 1.24 could materialize *types.Alias values when # type params > 0 and the GOEXPERIMENT is on. Basically ignore the GODEBUG flag for this case. This is backwards compatible with pre 1.24 code. Users of go/types with go.mod files at 1.22 may still need to update their tools to support *types.Aliases coming from 1.24 code, but they would not need to apply the a change like https://go.dev/cl/618676. They would not see an error from go/types. A proposal would be needed to change the godebug wording. The advantage is changes like this are unnecessary (in particular for users are not us).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/619395 mentions this issue: all: set gotypesalias=1 when using >=1.23 toolchain

gopherbot pushed a commit to golang/tools that referenced this issue Oct 10, 2024
Set gotypesalias=1 when using >=1.23 toolchain on all of
the main packages in x/tools that use go/types. Does not include
packages that are ignored due to build tags.

This effectively upgrades commit https://go.dev/cl/617095.

For golang/go#69772

Change-Id: I434c280b928ad21e1fd9c7f880e1324c14741dc3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/619395
Reviewed-by: Robert Findley <rfindley@google.com>
Commit-Queue: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/627715 mentions this issue: cmd/govulncheck: set gotypesalias=1 when using >=1.23 toolchain

gopherbot pushed a commit to golang/vuln that referenced this issue Nov 13, 2024
Set gotypesalias=1 when using >=1.23 toolchain on all of
the main packages in x/tools that use go/types.

This effectively upgrades commit https://go.dev/cl/617095.

For golang/go#69772

Change-Id: I9f3e64d348f6bffc75321a08145fde07fb4024a6
Reviewed-on: https://go-review.googlesource.com/c/vuln/+/627715
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
@timothy-king
Copy link
Contributor Author

golang.org/x/playground uses go/types only via golang.org/x/tools/imports. My manual inspection golang.org/x/tools/imports is not sensitive to this setting.

@timothy-king
Copy link
Contributor Author

golang.org/x/website uses go/types only via golang.org/x/tools/imports and runtime.test.

@timothy-king
Copy link
Contributor Author

For golang.org/x/exp/cmd/{apidiff,gorelease} the dependency on go/types is through golang.org/x/exp/apidiff.

golang.org/x/exp/cmd/apidiff tests fail when gotypesalias=1 is enabled.

So it looks like golang.org/x/exp/apidiff needs to be updated for types.Alias.

@timothy-king
Copy link
Contributor Author

golang.org/x/text takes underlying types in message/pipeline/extract.go. So this likely needs to be updated to support 1.24 or an expert on the code needs to do a thorough audit.

@timothy-king
Copy link
Contributor Author

The main packages in golang.org/x/exp/typeparams are examples for illustration purposes only. These do not take input. They are thus not a concern.

@timothy-king
Copy link
Contributor Author

Issues have been filed for the remaining investigations/work. Audit of the packages is done. Nothing remaining seems like a blocker for 1.24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants