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

cmd/go: always pass a GoVersion to cmd/vet #65612

Open
timothy-king opened this issue Feb 8, 2024 · 10 comments
Open

cmd/go: always pass a GoVersion to cmd/vet #65612

timothy-king opened this issue Feb 8, 2024 · 10 comments
Assignees
@samthanawalla
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Thinking
Milestone
Backlog

Comments

@timothy-king
Copy link
Contributor

Reported by @alexaandru in #63888 (comment)

$ cat go.mod
module example.com/m

go 1.22.0
$ cat main.go
package main

func main() {
	for i := range 10 {
		go func() { println(i) }()
	}
}

No output (as expected) when running as a package:

$ go1.22.0 vet example.com/m

False positive when running on a file:

$ go1.22.0 vet main.go
# command-line-arguments
# [command-line-arguments]
./main.go:5:23: loop variable i captured by func literal

go1.22.0 vet -x -json example.com/m gives the GoVersion as "GoVersion": "go1.22.0",. go1.22.0 vet -x -json main.go gives the GoVersion as "GoVersion": "",.

@bcmills
@timothy-king timothy-king mentioned this issue Feb 8, 2024
Closed
@bcmills
Copy link
Contributor

bcmills commented Feb 8, 2024

We set the GoVersion for cmd/vet based on the module containing the package to be built:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/exec.go;l=1168-1174;drc=f81e4986733bc18ec2bef16549534b9029756444
In the absence of that parameter I would expect cmd/vet to default to the most recent Go version, but maybe that's not the case?

At any rate, that does seem to match what we do when we invoke the compiler: we also omit the flag in that case.
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/work/gc.go;l=71-79;drc=b4e7d630bc6fbf654a20a4bebda94a8150811bea

This is kind of an unfortunate consequence of the property that files given on the command line are not considered to be part of a module (see https://go.dev/cl/339170).

That also has some implications for, say, importing internal packages, which we might not have fully taken into account. This may need more thought.

(CC @matloob @samthanawalla)

@bcmills bcmills added GoCommand cmd/go Thinking NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 8, 2024
@bcmills bcmills added this to the Backlog milestone Feb 8, 2024
@bcmills bcmills added the modules label Feb 8, 2024
@timothy-king
Copy link
Contributor Author

With the range scope change, there are some tools (x/tools/go/ssa, nilness, staticcheck) that need a decision about what the GoVersion of a file is. ssa and vet were defaulting to unknown meant the earliest possible version, e.g. 1. That seemed like a backwards compatible direction at the time. The vet bug report was an inconsistency with the command line file name and the module. The loopclosure.Analyzer can be fixed to interpret "" as an 'unknown' version and can suppress these. ssa needs to choose a semantics though. Guessing the toolchain version for a file would produce inconsistent results between runs. (More likely to guess more up to date?)

This may need more thought.

+1 Feel free to pull me into the conversation as needed.

@changkun
Copy link
Member

I was trying to remove some of the x := x parts from my existing code.

It turns out that golangci-lint invokes go vet, and remains to report loop variable i captured by func literal with Go 1.22, and some weird _ = x needs to stay in the code.

Any suggestions we can get rid of this in 1.22.0 release?

@timothy-king
Copy link
Contributor Author

@changkun I don't think this is directly related to this issue, and I will need to ask several follow-up questions. Please open a new issue and cc me.

@bcmills
Copy link
Contributor

bcmills commented Feb 14, 2024

Some interesting thought-experiments to consider:

  • If I run go run /tmp/foo.go, there isn't a “real” module corresponding to that file at all.
  • If go run ./nested/foo.go, and ./nested is a different (nested) module (or outside of my workspace), it may be the case that that module isn't in the build list at all (in which case it would be weird to report that module as containing the file), and it may also be the case that the go or toolchain directive in that module's go.mod file is actually higher than the toolchain that we're currently running.

In discussing with @rsc, @matloob, and @samthanawalla, we're thinking maybe the thing to do for now is to use the language version from either the go.work file or the main module's go.mod file.

On the other hand, there is currently a special case for resolving imports of internal paths, which works only if the directory containing the files package is within a main module. Maybe we can do something similar for the Go version.
(https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/load/pkg.go;l=1539-1548;drc=36b14a78b58924a8aea22c8949c3b8a4b7045d8b)

@timothy-king
Copy link
Contributor Author

In discussing with @rsc, @matloob, and @samthanawalla, we're thinking maybe the thing to do for now is to use the language version from either the go.work file or the main module's go.mod file.

Any discussion for when you were thinking this should be done? Part of 1.22.1 ? If it is after, maybe we should update cmd/vet to use the same logic in the interim? Having vet differ from run seems like a bad state of affairs.

@rsc
Copy link
Contributor

rsc commented Feb 14, 2024

Does vet differ from the compiler here? Presumably the go command passes the same version to both. If it does differ, why does it differ?

@bcmills
Copy link
Contributor

bcmills commented Feb 15, 2024

I think the compiler defaults to the highest go version rather than an older one.

@gopherbot
Copy link

Change https://go.dev/cl/567435 mentions this issue: cmd/go: set the GoVersion for files listed on the commandline

@samthanawalla samthanawalla added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Feb 27, 2024
@samthanawalla samthanawalla self-assigned this Feb 27, 2024
@timothy-king timothy-king mentioned this issue Feb 28, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/567635 mentions this issue: internal/versions: updates the meaning of FileVersions.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 28, 2024
@timothy-king
internal/versions: updates the meaning of FileVersions.
7f348c7 
For Go >=1.22, FileVersions returns an unknown [invalid] Future version
when the file versions would be invalid. This better matches
go/types.

For Go <=1.21, FileVersions returns either the runtime.Version()
or the Future version.

Adds AtLeast and Before for doing comparisons with Future.

Updates golang/go#65612
Fixes golang/go#66007

Change-Id: I93ff1681b0f9117765614a20d82642749597307c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/567635
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@bcmills bcmills mentioned this issue Mar 4, 2024
Open
@rsc rsc changed the title cmd/go: vet is passed an incorrect GoVersion when files are listed on the commandline cmd/go: always pass a GoVersion to cmd/vet Apr 5, 2024
@pgporada pgporada mentioned this issue Apr 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@samthanawalla samthanawalla

Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Thinking
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rsc @timothy-king @bcmills @changkun @gopherbot @samthanawalla