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: check for import path collision in go build #26227

Open
UsernameIsAlreadyTaken6 opened this issue Jul 5, 2018 · 11 comments
Open

cmd/go: check for import path collision in go build #26227

UsernameIsAlreadyTaken6 opened this issue Jul 5, 2018 · 11 comments
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@UsernameIsAlreadyTaken6
Copy link

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

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

Proposal: Raise a warning or an error at compile time if the import path of a local package matches the import path of package from the standard library.
This is prevents a fairly niche problem, namely collisions, but in practice the compile error that they produce remind of a declaration/syntax/code organization error so it's pretty hard to guess what went wrong.

@gopherbot gopherbot added this to the Proposal milestone Jul 5, 2018
@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

The Go compiler has no warnings, so that's not an option.

I guess a compiler error would be an option, but I'm not sure if that's possible to do as part of Go1. For example, what if a new standard library package is added? Should those packages break in people's GOPATHs all of a sudden?

There's also the question of whether this should also apply to internal packages within the standard library.

A perhaps more generic solution would be to encourage (or force) users to always use domains as the first element of their import paths, like foo.com/bar instead of foo/bar. However, that seems even less likely to happen, as I presume it would break lots of existing packages.

@UsernameIsAlreadyTaken6
Copy link
Author

UsernameIsAlreadyTaken6 commented Jul 5, 2018

When the collision happened I've got compile-time errors such as:

undefined: pkgname.XXXXXX".

Maybe a good compromise would be to check if "pkgname" matches the name of a standard library package when these errors happen, and if that the case, print a final error "No XXXXXX provided by standard library package pkgname"?

@mvdan
Copy link
Member

mvdan commented Jul 5, 2018

Perhaps so. I'm not sure how smart should the compiler errors get. /cc @mdempsky @randall77 @josharian

@josharian
Copy link
Contributor

If we did this it would be in cmd/go instead of cmd/compile. But this is a legitimate thing to do, so I don’t think we should make it an error. Also, I have personally used vendored versions of modified stdlib packages to incorporate fixes and performance improvements that were not upstream yet.

@mdempsky
Copy link
Contributor

mdempsky commented Jul 5, 2018

As best I can tell, the issue is that @UsernameIsAlreadyTaken6 created a $GOPATH/src/foo/bar package; but then when they tried to use it, $GOROOT/src/foo/bar was used instead?

In practice, this doesn't happen because user import paths are rooted in a domain name, which contains a period, whereas stdlib packages don't contain periods. Maybe we should formalize that practice better with a vet or lint warning?

@UsernameIsAlreadyTaken6
Copy link
Author

I thought naming my package "context" would be nice (I'm a newbie).. turns out it was not very nice.
I didn't use a domain name because it's a local project at the moment and deliberately adding a dummy domain would have been weird, but.. that's your call really.

@rsc
Copy link
Contributor

rsc commented Jul 9, 2018

If you tried to run "go install" in your context dir you get a good error:

$ cd $GOPATH/src
$ mkdir context
$ cd context
$ echo package context >c.go
$ go install
go install: no install location for /Users/rsc/src/context: hidden by /Users/rsc/go/src/context
$ 

(/Users/rsc/go is my $GOROOT.)

Maybe we should break "go build" too? I don't think we should make all builds trying to import the standard context break just because $GOPATH/src/context exists. And soon GOPATH will be gone anyway.

@bradfitz
Copy link
Contributor

Seems like making go build and go install consistent in their error messages makes sense.

@bradfitz bradfitz changed the title proposal: check for import path collision cmd/go: check for import path collision in go build Jul 16, 2018
@bradfitz bradfitz modified the milestones: Proposal, Unplanned Jul 16, 2018
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 16, 2018
@mknyszek
Copy link
Contributor

This came up again as the lack of an error leaks into downstream tooling and makes for a subpar user experience. See #53825. That was closed as a duplicate, but we may want to consider raising the priority of resolving this. Unclear who should do it, though.

@joerdav
Copy link

joerdav commented Oct 17, 2022

This could manifest in some odd ways. Luckily, the bug I raised #56269 was an issue someone encountered when learning rather than in a production bit of code.

I know module naming says you should name it as to not cause overlap, but if you don't follow this the the following could occur:

  • Company A creates a Go module but isn't interested in sharing it so just calls it floats, and they build it out with unit tests.
  • Go releases a new std lib package called floats to provide some useful utilities for floating point numbers.
  • Company A upgrade the version of Go.
  • Company A make a change that would have failed their tests, however go test ran the std library floats package instead.

Although it is a contrived example and very unlikely, does this mean each new package in the std library is breaking backwards compatibility?

@mvdan
Copy link
Member

mvdan commented Oct 17, 2022

does this mean each new package in the std library is breaking backwards compatibility?

No, given #32819. As you correctly point out, without reserving dot-less import paths for the standard library, adding any new standard library package could potentially break users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

10 participants