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

go/types: uses $GOROOT/pkg/$GOOS_$GOARCH/unsafe.a incorrectly #22112

Closed
rsc opened this issue Oct 2, 2017 · 8 comments
Closed

go/types: uses $GOROOT/pkg/$GOOS_$GOARCH/unsafe.a incorrectly #22112

rsc opened this issue Oct 2, 2017 · 8 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Oct 2, 2017

The Go compiler treats import "unsafe" as special - even if there is a vendor/unsafe directory, or a compiled unsafe.a package file, it doesn't use any of those - import "unsafe" always means the real unsafe, no matter what.

go/types appears not to follow this rule. I accidentally created a $GOROOT/pkg/darwin_amd64/unsafe.a, and that causes go/types to fail to typecheck just about any code using unsafe, because it no longer uses the real unsafe.

Obviously I will fix my problem and stop generating unsafe.a, but go/types should still refuse to treat import "unsafe" as anything other than the real unsafe.

/cc @adonovan @griesemer

@rsc rsc added this to the Go1.10 milestone Oct 2, 2017
@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Oct 2, 2017

@rsc which package did you use to import the data into go/types?

I believe that's who's responsible for ensuring that unsafe is always the same.

(Not 100% sure about that but I've been poking around in various importers the last few days and noticed code like "if importing unsafe then return types.Unsafe")

Loading

@rsc
Copy link
Contributor Author

@rsc rsc commented Oct 2, 2017

Reproduction:

touch $(go env GOROOT)/pkg/$(go env GOOS)_$(go env GOARCH)/unsafe.a
go test -short go/types

It fails quite loudly. The first few lines of output are:

--- FAIL: TestBuiltinSignatures (0.00s)
	builtins_test.go:141: var s []int; _ = append(s): 1:19: could not import unsafe (/Users/rsc/go/pkg/darwin_amd64/unsafe.a: can't find export data (EOF))
	builtins_test.go:141: var s []int; _ = append(s, 0): 1:19: could not import unsafe (/Users/rsc/go/pkg/darwin_amd64/unsafe.a: can't find export data (EOF))

and so on.

Loading

@griesemer griesemer self-assigned this Oct 3, 2017
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 3, 2017

go/types just uses whatever importer is provided. In this case, it's going to be the default importer which is the gc importer. Since the unsafe.a file exists, it tries to import that.

It's certainly easy to not call any importer at all for "unsafe".

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 3, 2017

Change https://golang.org/cl/67634 mentions this issue: go/types: import "unsafe" always means package unsafe

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Oct 3, 2017

The Go vendor spec explicitly says that
https://golang.org/s/go15vendor:

These rules...apply to standard library packages. If someone wants to vendor (and therefore hide the standard library version of) “math” or even “unsafe”, they can.

It sounds like a bug in gc or 'go build' to me, not go/types.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 3, 2017

ping @rsc

Loading

@rsc
Copy link
Contributor Author

@rsc rsc commented Oct 4, 2017

Sigh. Obviously my memory is going. OK. Well, the importer is going to get rewritten as part of package management and workspace abstraction issues, so that will probably fix this anyway. So I guess nothing to do now.

Loading

@rsc rsc closed this Oct 4, 2017
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 4, 2017

FWIW, it doesn't make a lot of sense to me to vendor package unsafe - I'd be fine to disallow this and perhaps even state it in the spec that importing unsafe means only one thing and cannot be changed.

Loading

@golang golang locked and limited conversation to collaborators Oct 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants