cmd/go: add (or make explicit) tests for ambiguous imports in module mode

[jeanbza]

What version of Go are you using (go version)?

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/deklerk/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/deklerk/workspace/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lk/zs4m7sv12mq2vzk_wfn2tfvm00h16k/T/go-build259032920=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

• Create repo foo with:
  • A foo/go.mod
  • A foo/somefile.go containing const Version = "foo=0.0.1"
  • A directory foo/bar/ with a foo/bar/somefile.go containing const Version = "foo=0.0.1"
• Commit, tag v0.0.1 (parent encompasses directory), push both

...

• In some other library, depend on foo with require foo v0.0.1
• Add fmt.Println(foo.Version) // expect "foo=0.0.1"
• Add fmt.Println(bar.Version) // expect "foo=0.0.1"

...

• Back in repo foo, make a foo/bar its own submodule:
  • Create foo/bar/go.mod
  • Update foo/bar/somefile.go to now have const Version = "bar=1.0.0"
  • Commit, tag bar/v1.0.0, tag v0.0.2 (new version of the parent because a submodule was carved out), and push all

...

• In your other library, add a require bar v1.0.0 dependency (keeping the require foo v0.0.1 dependency!)
• Now you require "bar" two ways: via require bar v1.0.0 and via require foo v0.0.1, in which foo/bar was still part of the module
• fmt.Println(bar.Version) results in bar=1.0.0

EDIT: actually, you get the following error (see @myitcv excellent repro below):

```
unknown import path "github.com/jadekler/module-testing/pkg_b": ambiguous import: found github.com/jadekler/module-testing/pkg_b in multiple modules:
```

What did you expect to see?

I'm not sure. Maybe this is WAI? I vaguely expected to see an error. I suspect there are some hidden subtleties here that could end up breaking people. Maybe since bar has to always be backwards compatible, this is OK?

Anyways, feel free to close if this is unsurprising. I thought it was subtle and maybe worth looking at, but y'all let me know.

EDIT: Ignore this, since my original post was flawed. In fact you cannot do this, which seems to imply carving out submodules after-the-fact is unsupported.

What did you see instead?

Go modules seems to silently replace foo/bar/ in the require foo v0.0.1 with the bar v1.0.0 version.

EDIT: Ignore this, since my original post was flawed. In fact you cannot do this, which seems to imply carving out submodules after-the-fact is unsupported.

[agnivade]

I don't see anything obviously wrong here. It was always part of the design to be able to import to different versions of the same package. A submodule is also just another package.

/cc @myitcv for further comments.

[myitcv]

cc @bcmills too 😄

@jadekler - can you provide a bit more detail? Because I get an error as expected (the import is ambiguous):

...
$ go mod edit -require=github.com/myitcvscratch/foo/bar@v1.0.0
$ cat go.mod
module mod

require (
        github.com/myitcvscratch/foo v0.0.1
        github.com/myitcvscratch/foo/bar v1.0.0
)
$ go run .
go: finding github.com/myitcvscratch/foo/bar v1.0.0
go: downloading github.com/myitcvscratch/foo/bar v1.0.0
main.go:6:2: unknown import path "github.com/myitcvscratch/foo/bar": ambiguous import: found github.com/myitcvscratch/foo/bar in multiple modules:
        github.com/myitcvscratch/foo v0.0.1 (/home/gopher/pkg/mod/github.com/myitcvscratch/foo@v0.0.1/bar)
        github.com/myitcvscratch/foo/bar v1.0.0 (/home/gopher/pkg/mod/github.com/myitcvscratch/foo/bar@v1.0.0)

Full repro

$ cd /tmp
$ mkdir foo usefoobar
$ cd foo
$ git init
Initialized empty Git repository in /tmp/foo/.git/
$ git remote add origin https://github.com/myitcvscratch/foo
$ go mod init
go: creating new go.mod: module github.com/myitcvscratch/foo
$ mkdir bar
$ cat <foo.go
package foo
const Version = "foo v0.0.1"

EOD

$ cat <bar/bar.go

package bar
const Version = "foo v0.0.1"

EOD

$ git add -A

$ git commit -q -am 'Initial commit'

$ git push -q

remote:

remote: Create a pull request for 'master' on GitHub by visiting:

remote:      https://github.com/myitcvscratch/foo/pull/new/master

remote:

$ git tag v0.0.1

$ git push -q origin v0.0.1

$ cd /tmp/usefoobar

$ go mod init mod

go: creating new go.mod: module mod

$ cat <main.go

package main
import (

"fmt"

"github.com/myitcvscratch/foo"

"github.com/myitcvscratch/foo/bar"

)
func main() {

fmt.Printf("foo: %v\n", foo.Version)

fmt.Printf("bar: %v\n", bar.Version)

}

EOD

$ go run .

go: finding github.com/myitcvscratch/foo/bar latest

go: finding github.com/myitcvscratch/foo v0.0.1

go: downloading github.com/myitcvscratch/foo v0.0.1

foo: foo v0.0.1

bar: foo v0.0.1

$ cd /tmp/foo/bar

$ go mod init github.com/myitcvscratch/foo/bar

go: creating new go.mod: module github.com/myitcvscratch/foo/bar

$ cat <bar.go

package bar
const Version = "var v1.0.0"

EOD

$ cd ..

$ cat <foo.go

package foo
const Version = "foo v0.0.2"

EOD

$ git add -A

$ git commit -q -am 'Second commit'

$ git tag v0.0.2

$ git tag bar/v1.0.0

$ git push -q origin v0.0.2

$ git push -q origin bar/v1.0.0

$ cd /tmp/usefoobar

$ go mod edit -require=github.com/myitcvscratch/foo/bar@v1.0.0

$ cat go.mod

module mod
require (

github.com/myitcvscratch/foo v0.0.1

github.com/myitcvscratch/foo/bar v1.0.0

)

$ go run .

go: finding github.com/myitcvscratch/foo/bar v1.0.0

go: downloading github.com/myitcvscratch/foo/bar v1.0.0

main.go:6:2: unknown import path "github.com/myitcvscratch/foo/bar": ambiguous import: found github.com/myitcvscratch/foo/bar in multiple modules:

github.com/myitcvscratch/foo v0.0.1 (/home/gopher/pkg/mod/github.com/myitcvscratch/foo@v0.0.1/bar)

github.com/myitcvscratch/foo/bar v1.0.0 (/home/gopher/pkg/mod/github.com/myitcvscratch/foo/bar@v1.0.0)

But if I then require v0.0.2 of foo all works as expected:

$ go mod edit -require=github.com/myitcvscratch/foo@v0.0.2
$ cat go.mod
module mod

require (
        github.com/myitcvscratch/foo v0.0.2
        github.com/myitcvscratch/foo/bar v1.0.0
)
$ go run .
go: finding github.com/myitcvscratch/foo v0.0.2
go: downloading github.com/myitcvscratch/foo v0.0.2
foo: foo v0.0.2
bar: var v1.0.0

In case it's useful, I use the very hacky egrunner to quickly pull together these repros

[jeanbza]

Ahhh. Thank you @myitcv, in my repro I had incorrectly depended upon too late of a version. When I bump it back down to v0.0.1, I see the same failure. Thanks for the confirmation.

Before I make changes to this issue, let me ask y'all this: this result seems to indicate that it is subtly unsafe to carve out a submodule after-the-fact if anyone depends on your package. Do you agree? In which case I'd like to suggest that the documentation makes this clear.

cc @hyangah @bradfitz re: x/tools, x/oauth2 (maybe y'all have already discovered and discussed this)

[myitcv]

What do you mean by "unsafe", exactly?

I think the situation we've reproduced here is unfortunate (and could perhaps be better flagged/resolved), but I don't think it's unsafe in so far as we end up with a bad build.

Come and join the party over in #27858 (comment) (linking to @bcmills's thoughts on this specifically) for more discussion on this!

[jeanbza]

Unsafe as in, users' programs may break in the "ambiguous import" manner seen above. That is, you probably need to do major bumping when you carve out a submodule, instead of just a minor bump.

Subtle as in, this is hard to see as a library maintainer without setting up a fairly convoluted check.

SG, will catch up on that discussion and see if I have anything of worth to contribute heh. :)

[myitcv]

Unsafe as in, users' programs may break in the "ambiguous import" manner seen above. That is, you probably need to do major bumping when you carve out a submodule, instead of just a minor bump.

Got it. Just one small nit, the program itself doesn't break, the build does. And the build only breaks in response to us doing something, i.e. go mod edit -require=github.com/myitcvscratch/foo/bar@v1.0.0 (or equivalent). It's conceivable that in such a situation we can do something to try and automatically resolve the situation, thereby side-stepping the error altogether, and hence it's no longer a subtle problem. But that's just a knee-jerk response. @bcmills has given this much more thought.

[jeanbza]

Turns out you can do this in a really funny way. Per #27858 (comment), you can make the carved-out submodule require the first version of the parent module that does not contain its contents. Although, I don't think you can do this atomically, since the "carve out commit" must be tagged parent=vNEXT as well as child=vNEXT, and in that same commit you need to have the require statement for parent=vNEXT.

[bcmills]

You can do it atomically (by tagging the parent and child vNEXT in the same commit), but you can't test it before you push (because you can't currently resolve unpublished module versions). 😕

[jeanbza]

Ohhh right. I had incorrectly thought there wouldn't be a sha until you push. But, that's obviously wrong.

[bcmills]

in my repro I had incorrectly depended upon too late of a version.

I had thought we had a test that confirmed the ambiguous import error, but git grep ambiguous isn't turning anything up. Since this does seem to be working as intended, I'll retitle the bug to be about making sure it keeps working.

[hyangah]

You can do it atomically (by tagging the parent and child vNEXT in the same commit), but you can't test it before you push (because you can't currently resolve unpublished module versions).

In addition to this, I observed we can't compute the correct go.sum entries. It doesn't cause build failure but verification is skipped.

[jeanbza]

@hyangah Maybe my git noob-ness, but: does the commit sha change when you push? In #28806 (comment) I had naively thought the local sha would be OK to manually use.

edit: Whoops, ignore this. I just had the realization that go.sum is not comprised of shas.

[bcmills]

we can't compute the correct go.sum entries.

That's true, although omitting a go.sum entry is fairly benign. (Certainly not ideal, but not actively harmful either.)

[gopherbot]

Change https://golang.org/cl/255046 mentions this issue: cmd/go/internal/modload: avoid a network fetch when querying a valid semantic version