cmd/go: check for conflicting vN/go.mod files during development

[bcmills]

Forked from discussion on https://golang.org/cl/116836.

@rsc says:

The module is defined by a go.mod and the files in that directory and below, not by files above. If x/v2/go.mod exists and is OK and says it is x/v2, it should not matter much what x/go.mod says, only that it doesn't also claim to be x/v2. When you are working in the x/v2/foo directory, vgo walks up parent directories to find x/v2/go.mod and stops there. It doesn't sanity check all possible higher parents. So everything seems fine. Nothing looks at x/go.mod. Then you commit, you tag v2.1.3, and you push it, and THEN you find out that vgo cares about some mistake in your x/go.mod that is completely irrelevant to using x/v2 (like maybe x/go.mod says require blah/v4 v5.0.0 - it doesn't matter to deciding the code we want is in x/v2). Clearly we do have to check that x/go.mod does not say module x/v2 but beyond that, vgo should be as robust as possible to problems in irrelevant files.

That seems inconsistent to me. If x/go.mod is irrelevant to x/v2/go.mod (and vice-versa) during development, then it should be irrelevant after commit too. But it cannot be irrelevant after commit, because vgo's “major subdirectory” convention makes tag resolution ambiguous.

Consider a sequence of events where the structure builds in the opposite direction: suppose you're working in repo root rsc.io/x/, and you're migrating from the “directory” convention to the “branch” convention. You start by copying everything from x/v2/ to x/, including the go.mod file. Then you run vgo test all, and everything passes. Now you tag and push the commit, and it turns out that you broke vgo get rsc.io/x@v2: it wasn't ambiguous when you were testing locally, but now it is.

If we want to avoid that kind of failure mode, then we need one of two things: either we need to detect the conflicting v2 module during the normal build/test cycle, or we need to ignore the conflict during tag resolution (i.e., by arbitrarily preferring either the outer or the inner go.mod file).

An example illustrating the undetected problem:

/tmp/v2v2$ find .
.
./y
./go.mod
./x_test.go
./v2
./v2/go.mod
./v2/x_test.go

/tmp/v2v2$ cat go.mod
module x/v2

/tmp/v2v2$ cat v2/go.mod
module x/v2

/tmp/v2v2$ vgo test all
ok      x/v2    (cached)
ok      x/v2/v2 (cached)

(CC: @myitcv)

[myitcv]

The module is defined by a go.mod and the files in that directory and below, not by files above.

This corresponds with how I think about modules. Because after all, directories (and their subdirectories) containing modules can appear anywhere: the only things that cares that they are properly nested is vgo list from a parent directory (this could ensure consistency, not sure whether it currently does) or vgo itself when resolving modules (ultimately from a VCS), per Russ' comments.

That said:

... then it should be irrelevant after commit too

Did you mean "after push/release" here?

Because I think the rest of your argument gets at an issue I was thinking about earlier, namely having some sort of tooling to verify offline, pre-push (release) that your modules are nested, tagged etc as you expect. For custom import paths this would need to be combined with some sort of injection of <meta> tags (because again, you want to test everything offline before the big "push").

Quite how this overlaps (or not) with the proposed "release" tooling I'm not sure.

Equally I'm not sure how well this maps to (company) setups where there is a more involved release process.

[myitcv]

@bcmills just to pick up on the point I made above:

the only things that cares that they are properly nested is vgo list from a parent directory

Just to make explicit the "disconnect" I was alluding to:

$ mkdir pkga
$ cd pkga
$ cat <<EOD >pkga.go
package pkga // import "example.com/pkga"
EOD
$ vgo mod -init
vgo: creating new go.mod: module example.com/pkga
$ mkdir pkgb
$ cd pkgb
$ cat <<EOD >pkgb.go
package pkgb // import "banana.com/pkgb"
EOD
$ vgo mod -init
vgo: creating new go.mod: module banana.com/pkgb
$ vgo list .
banana.com/pkgb
$ cd ..
$ vgo list ./...
example.com/pkga
example.com/pkga/pkgb

But I'll note here that we are looking at packages, not modules.

Another interesting case:

$ mkdir pkga
$ cd pkga
$ vgo mod -init -module banana.com/pkga
vgo: creating new go.mod: module banana.com/pkga
$ cat <<EOD >pkga.go
package pkga // import "example.com/pkga"
EOD
$ vgo test
testing: warning: no tests to run
PASS
?       banana.com/pkga 0.001s [no test files]
$ vgo list .
banana.com/pkga
$ vgo mod -json
{
        "Module": {
                "Path": "banana.com/pkga",
                "Version": ""
        },
        "Require": null,
        "Exclude": null,
        "Replace": null
}

Both examples demonstrate there is (currently) no import path checking for packages "within" a module (else both would have failed).

Not sure whether this is related or not (or whether it's "broken") - just adding another data point.

[rsc]

The go command today does NOT go poking around for version control system metadata in the directories where it finds local source code (except legacy "go get", which is going away), which would be required to detect this problem. I strongly believe it should continue to be ignorant of version control system metadata.

I could see an argument for walking up the file system to look for a parent go.mod that conflicts with the current one, but what if you just want to try something out and do:

mkdir _test
cp -a * _test
cd _test
go build, etc

We shouldn't disallow this. Or even if you are moving to a subdirectory development mode and do

mkdir v2
cp -a *v2
cd v2

to play around and see how that would work: should you have to delete all the parent files just to try something out?

The error messages for when this does happen are now dramatically clearer than they were. Let's close this until we find evidence that they're not good enough on their own.

Another good place for a check like this would be the eventual "go release", but that doesn't exist yet. I created #26420 and listed this check there.