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

cmd/go: vet is passed an incorrect GoVersion when files are listed on the commandline #65612

Open
timothy-king opened this issue Feb 8, 2024 · 8 comments
Labels
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

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

@bcmills
Copy link
Member

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
Member

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
Member

bcmills commented Feb 15, 2024

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

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

No branches or pull requests

4 participants