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/go/analysis: checkers do not (but should) reject Go 1.22 programs #61174

Closed
rsc opened this issue Jul 5, 2023 · 10 comments
Closed

x/tools/go/analysis: checkers do not (but should) reject Go 1.22 programs #61174

rsc opened this issue Jul 5, 2023 · 10 comments
Assignees
@rsc
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Go1.21

Comments

@rsc
Copy link
Contributor

rsc commented Jul 5, 2023 •
edited
Loading

Checkers built on x/tools/go/analysis do not reject Go 1.22 programs but should. Will fix.
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Jul 5, 2023
@rsc rsc added this to the Go1.21 milestone Jul 5, 2023
@rsc rsc self-assigned this Jul 5, 2023
@rsc rsc mentioned this issue Jul 5, 2023
Closed
@rsc rsc changed the title cmd/vet: does not reject Go 1.22 programs go/analysis: checkers do not reject Go 1.22 programs Jul 5, 2023
@rsc rsc changed the title go/analysis: checkers do not reject Go 1.22 programs x/tools/go/analysis: checkers do not reject Go 1.22 programs Jul 5, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 5, 2023
@rsc rsc mentioned this issue Jul 5, 2023
Closed
@rsc rsc changed the title x/tools/go/analysis: checkers do not reject Go 1.22 programs x/tools/go/analysis: checkers do not (but should) reject Go 1.22 programs Jul 5, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/507975 mentions this issue: go/types: record Config.GoVersion for reporting in Package.GoVersion method

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/507880 mentions this issue: go/analysis: pass package's Go version to type checker

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/507879 mentions this issue: go/packages: pass go list-reported Go language version to type checker

@dominikh
Copy link
Member

dominikh commented Jul 5, 2023

This concern affects the combination of go/packages, go/analysis, and go/types: The various GoVersion fields are only populated when a minimum Go version was deduced from go.work or go.mod. But this leaves us with no GoVersion for the standard library, which go list -json doesn't treat as a module, or when using GOPATH mode.

A consequence of that is that tools can't rely exclusively on the GoVersion field to know if they're too old to correctly interpret the code; they also need to figure out the actual Go version and use that when GoVersion isn't set.

Is there something we can do about that?

@rsc
Copy link
Contributor Author

rsc commented Jul 5, 2023 •
edited
Loading

For Go 1.21, I don't think we have a great answer. Luckily it shouldn't be a huge problem either: Nothing should really care about the Go version for the Go 1.21 release. These changes are aimed at setting us up for Go 1.22, specifically the loopvar change. For non-standard-library packages, only code in modules will see the new semantics, so the check can be written so that "no version" = "old loop variable semantics". And any module that says "go 1.22" should be properly rejected by an old tool. That "no version" = "old loopvar semantics" does not handle the standard library though.

I wonder if for Go 1.22 we should try having go list -json report the "std" and "cmd" modules in the Package.Module field. That's too big a change to land this late in the Go 1.21 cycle, but as long as it was in Go 1.22, that would be enough to handle the standard library in any checker that cared because of loopvar.

@dominikh
Copy link
Member

dominikh commented Jul 5, 2023

Having the "std" and "cmd" modules would be great.

Do note that for checkers, language changes may not be the only reason to care about the Go version. Staticcheck has long been using the Go version to guard checks that make recommendations concerning use of the standard library, such as suggesting time.Until, which was added in Go 1.8.

None of this is really important for 1.21 specifically, though, and waiting until 1.22 is fine.

As an aside, all of the GoVersion-related changes, including per-file versions, really benefit Staticcheck. We went from having a -go flag that the user could provide — if they knew about it — to pretending that the go go.mod directive prescribed the minimum version, to having actual, fine-grained minimum versions.

@rsc
Copy link
Contributor Author

rsc commented Jul 5, 2023

Glad to hear it. Thanks for the feedback @dominikh!

@dominikh dominikh mentioned this issue Jul 5, 2023
Closed
gopherbot pushed a commit that referenced this issue Jul 6, 2023
@rsc
go/types: record Config.GoVersion for reporting in Package.GoVersion …
b490bdc 
…method

Clients of go/types, such as analyzers, may need to know which
specific Go version a package is written for. Record that information
in the Package and expose it using the new GoVersion method.

Update parseGoVersion to handle the new Go versions that may
be passed around starting in Go 1.21.0: versions like "go1.21.0"
and "go1.21rc2". This is not strictly necessary today, but it adds some
valuable future-proofing.

While we are here, change NewChecker from panicking on invalid
version to saving an error for returning later from Files.
Go versions are now likely to be coming from a variety of sources,
not just hard-coded in calls to NewChecker, making a panic
inappropriate.

For #61174.
Fixes #61175.

Change-Id: Ibe41fe207c1b6e71064b1fe448ac55776089c541
Reviewed-on: https://go-review.googlesource.com/c/go/+/507975
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 6, 2023
@rsc
go/packages: pass go list-reported Go language version to type checker
f9f582e 
Type checking of a package depends on the Go language version in
effect for that package. We have been not setting it and assuming
"latest" is good enough, but that is likely to become untrue in the
future, and it violates Go 1.21's emphasis on forward compatibility,
namely tools recognizing when they shouldn't be processing newer code.

Pass the Go version along from go/packages to go/types, to allow
go/types to apply the version when type-checking.

This is tested by CL 507880.

For golang/go#61174.

Change-Id: I49353dede9c7c095c2cd0c4a6959f9f6e6a06ec5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/507879
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
@rsc rsc reopened this Jul 12, 2023
@rsc
Copy link
Contributor Author

rsc commented Jul 12, 2023

I haven't brought the code into the main repo yet.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/509099 mentions this issue: all: update vendored dependencies

@rsc
Copy link
Contributor Author

rsc commented Jul 12, 2023

Submitted the revendor.

@rsc rsc closed this as completed Jul 12, 2023
gopherbot pushed a commit that referenced this issue Jul 12, 2023
@rsc
all: update vendored dependencies
6244b19 
Generated by:

	go install golang.org/x/tools/cmd/bundle@latest
	go install golang.org/x/build/cmd/updatestd@latest
	updatestd -goroot=$GOROOT -branch=master

For #36905.
For #55079.

Fixes #61174 (vet checkers understanding Go language version).
Fixes #61200 (slog InfoCtx -> InfoContext etc).

Change-Id: I4f2c86960ce72d6df06e23da1b1297ab3ff2eecf
Reviewed-on: https://go-review.googlesource.com/c/go/+/509099
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@rsc @bradfitz
go/types: record Config.GoVersion for reporting in Package.GoVersion …
e9af5ae 
…method

Clients of go/types, such as analyzers, may need to know which
specific Go version a package is written for. Record that information
in the Package and expose it using the new GoVersion method.

Update parseGoVersion to handle the new Go versions that may
be passed around starting in Go 1.21.0: versions like "go1.21.0"
and "go1.21rc2". This is not strictly necessary today, but it adds some
valuable future-proofing.

While we are here, change NewChecker from panicking on invalid
version to saving an error for returning later from Files.
Go versions are now likely to be coming from a variety of sources,
not just hard-coded in calls to NewChecker, making a panic
inappropriate.

For golang#61174.
Fixes golang#61175.

Change-Id: Ibe41fe207c1b6e71064b1fe448ac55776089c541
Reviewed-on: https://go-review.googlesource.com/c/go/+/507975
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@rsc @bradfitz
all: update vendored dependencies
64f6af6 
Generated by:

	go install golang.org/x/tools/cmd/bundle@latest
	go install golang.org/x/build/cmd/updatestd@latest
	updatestd -goroot=$GOROOT -branch=master

For golang#36905.
For golang#55079.

Fixes golang#61174 (vet checkers understanding Go language version).
Fixes golang#61200 (slog InfoCtx -> InfoContext etc).

Change-Id: I4f2c86960ce72d6df06e23da1b1297ab3ff2eecf
Reviewed-on: https://go-review.googlesource.com/c/go/+/509099
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
awly pushed a commit to tailscale/go that referenced this issue Feb 7, 2024
@rsc @awly
go/types: record Config.GoVersion for reporting in Package.GoVersion …
55ed560 
…method

Clients of go/types, such as analyzers, may need to know which
specific Go version a package is written for. Record that information
in the Package and expose it using the new GoVersion method.

Update parseGoVersion to handle the new Go versions that may
be passed around starting in Go 1.21.0: versions like "go1.21.0"
and "go1.21rc2". This is not strictly necessary today, but it adds some
valuable future-proofing.

While we are here, change NewChecker from panicking on invalid
version to saving an error for returning later from Files.
Go versions are now likely to be coming from a variety of sources,
not just hard-coded in calls to NewChecker, making a panic
inappropriate.

For golang#61174.
Fixes golang#61175.

Change-Id: Ibe41fe207c1b6e71064b1fe448ac55776089c541
Reviewed-on: https://go-review.googlesource.com/c/go/+/507975
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
awly pushed a commit to tailscale/go that referenced this issue Feb 7, 2024
@rsc @awly
all: update vendored dependencies
456388d 
Generated by:

	go install golang.org/x/tools/cmd/bundle@latest
	go install golang.org/x/build/cmd/updatestd@latest
	updatestd -goroot=$GOROOT -branch=master

For golang#36905.
For golang#55079.

Fixes golang#61174 (vet checkers understanding Go language version).
Fixes golang#61200 (slog InfoCtx -> InfoContext etc).

Change-Id: I4f2c86960ce72d6df06e23da1b1297ab3ff2eecf
Reviewed-on: https://go-review.googlesource.com/c/go/+/509099
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
@dominikh dominikh mentioned this issue May 16, 2024
Open
@golang golang locked and limited conversation to collaborators Jul 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rsc rsc

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

No branches or pull requests
3 participants
@dominikh @rsc @gopherbot