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: go list reports a mangled ImportPath for a relative path that is not a valid Go package #36008

Open
nmiyake opened this issue Dec 6, 2019 · 14 comments · May be fixed by #36244
Open

cmd/go: go list reports a mangled ImportPath for a relative path that is not a valid Go package #36008

nmiyake opened this issue Dec 6, 2019 · 14 comments · May be fixed by #36244

Comments

@nmiyake
Copy link

@nmiyake nmiyake commented Dec 6, 2019

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

Using gotip as of 12/5/2019 because this workflow fails with an error using Go 1.13.4:

$ gotip version
go version devel +eeb319a Thu Dec 5 22:16:02 2019 +0000 darwin/amd64

What did you do?

$ mkdir ~/foo && cd $_
$ echo "module github.com/nmiyake/foo" > go.mod
$ gotip list -e -f {{.ImportPath}} .

What did you expect to see?

github.com/nmiyake/foo

What did you see instead?

_/Users/nmiyake/foo

When running with Go 1.13.4, see:

build .: cannot find module for path .

This is likely #35414, which has been fixed on tip.

Further information/context

When a Go file is added to the directory, the output is as expected:

$ echo "package main" > main.go
$ gotip list -e -f {{.ImportPath}} .
github.com/nmiyake/foo

The behavior previously worked as I would expect in GOPATH mode:

$ mkdir $GOPATH/src/github.com/nmiyake/foo && cd $_
$ gotip list -e -f {{.ImportPath}} .
github.com/nmiyake/foo

If the list command can find the go.mod file and determine the import path relative to that, it would be helpful if it returned the import path if Go files existed in the directory rather than the _ import path.

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Dec 6, 2019

@dmitshur dmitshur added this to the Backlog milestone Dec 6, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

This is closely related to #33157. We cannot in general assume that an empty directory within a module corresponds to that package import path.

For example, if there were a github.com/nmiyake module in the dependency graph (in addition to the github.com/nmiyake/foo main module), then it would be possible for that module to provide package github.com/nmiyake/foo — so reporting that path for the empty directory would be misleading (since the package that actually exists would be in some other directory entirely).

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

Note that the change in behavior from go1.13.4 to gotip is almost entirely in the error reporting: the error is now reported for the package instead of failing the entire command. That is an intentional result of CL 206902.

$ gotip version
go version devel +0915a19a Fri Dec 6 05:12:15 2019 +0000 linux/amd64

$ mkdir foo && cd foo

foo$ echo "module github.com/nmiyake/foo" > go.mod

foo$ go1.13.5 list .
build .: cannot find module for path .

foo$ go1.13.5 list -e -f '{{.ImportPath}}: {{.Error}}' .
build .: cannot find module for path .

foo$ gotip list .
can't load package: package .: no Go files in /tmp/tmp.2ibCnzx27T/foo

foo$ gotip list -e -f '{{.ImportPath}}: {{.Error}}' .
_/tmp/tmp.2ibCnzx27T/foo: package .: no Go files in /tmp/tmp.2ibCnzx27T/foo
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

It is admittedly a bit strange that we emit a mangled ImportPath instead of leaving it at the user-provided ., though.

@bcmills bcmills changed the title cmd/go: go list returns incorrect import path for module without Go files cmd/go: go list reports a mangled ImportPath for a relative path that is not a valid Go package Dec 6, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

This needs a fix, but the fix that is needed is not to report a module-relative path. I think we should preserve the original argument passed by the user (or ., if no argument is provided) as the reported ImportPath, rather than mangling the relative import path as would be done in GOPATH mode.

That is, the output should be more like:

foo$ go list -e -f '{{.ImportPath}}: {{.Error}}' .
.: package .: no Go files in /tmp/tmp.2ibCnzx27T/foo
@nmiyake

This comment has been minimized.

Copy link
Author

@nmiyake nmiyake commented Dec 6, 2019

@bcmills for clarification, can you explain why the situation is different if the directory just happens to contain any *.go file? Doesn't the possibility of another module providing that path still exist? That's the primary reason I'm confused about this behavior (and why I expected the import path with no Go files in the directory to match the case where there are Go files there).

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 6, 2019

If the directory does contain a .go source file, and some other active module also provides that package, then the go command should emit an “ambiguous import” error for that package at build time.

@nmiyake

This comment has been minimized.

Copy link
Author

@nmiyake nmiyake commented Dec 6, 2019

I see -- so the thinking is that if the directory doesn't contain a Go file, it isn't a valid package, and reporting it as such could be dangerous/misleading since the build tool wouldn't be able to flag the ambiguous import path (or, in another scenario, the build tool could report a conflict based on a directory that doesn't contain.go files).

Thanks, I believe I understand now. My workflow should be unblocked with Go 1.14, since at that point the overall operation of listing the packages will not fail, and I can filter out non-packages based on the error state being set in the JSON.

@GrigoriyMikhalkin

This comment has been minimized.

Copy link

@GrigoriyMikhalkin GrigoriyMikhalkin commented Dec 21, 2019

Hi, i would like to work on that issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 21, 2019

Change https://golang.org/cl/212358 mentions this issue: cmd/go: return user provided path instead of mangled ImportPath

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jan 2, 2020

I'd argue against changing ImportPath when there's an error. When a package is loaded successfully, the mangled import path (for example _/Users/nmiyake/foo) is the real import path, as seen in stack traces. go list -e -f {{.ImportPath}} ought to be consistent, whether there's an error or not. Inconsistency will confuse golang.org/x/tools/go/packages and tools that depend on it. (cc @matloob @stamblerre to confirm).

It's fine of course to report the original name by which a package was loaded (command line argument or import string) in error messages. I think we're already doing that though in setError in cmd/go/internal/load.Package.load by copying the import stack. This seems correct to me:

$ go list -e -f '{{.ImportPath}}: {{.Error}}' .
_/Users/jayconrod/Code/test: package .: no Go files in /Users/jayconrod/Code/test
@matloob

This comment has been minimized.

Copy link
Contributor

@matloob matloob commented Jan 2, 2020

Yes, we definitely should not return inconsistent ImportPath fields, even when there's an error.

gopls will try to do the best it can loading the packages, and it uses the import path fields as part of building the package graph.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 8, 2020

When a package is loaded successfully, the mangled import path (for example _/Users/nmiyake/foo) is the real import path, as seen in stack traces.

Note that the issue as originally reported is in module mode, not GOPATH mode. If the package were loaded successfully in module mode, then it would have a path that is a suffix of the module path.

The mangled path is only correct in GOPATH mode, but we are not in GOPATH mode in this case, so a mangled path is not appropriate at all.

example.com$ gotip mod init example.com
go: creating new go.mod: module example.com

example.com$ gotip env GOMOD
/tmp/tmp.70NvW5Uu1V/example.com/go.mod

example.com$ gotip list -m
example.com

example.com$ gotip list -e -f {{.ImportPath}} .
_/tmp/tmp.70NvW5Uu1V/example.com
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 8, 2020

To put it another way: there is no “correct” ImportPath to report in this case.

  • We cannot report a module-prefixed path, because that path may (in general) actually be provided by another module whose path is a prefix of the module path + relative path within the module.

  • We should not report a mangled path, because that would not be the actual path if the package were loaded successfully. (The mangled path bears no relationship to the actual package path, nor to the user-supplied argument — it is a complete non sequitur.)

That leaves only one viable option for the import path: whatever path the user requested, even if it is malformed. (Or, I suppose, we could leave the ImportPath field completely blank, but that seems even more confusing than preserving the user-supplied argument.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.