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/compile: _ (blank) struct fields lose package info #15514

Closed
griesemer opened this issue May 3, 2016 · 4 comments
Closed

cmd/compile: _ (blank) struct fields lose package info #15514

griesemer opened this issue May 3, 2016 · 4 comments

Comments

@griesemer
Copy link
Contributor

This is a bug with the old (and currently also the new) export format. Example by mdempsky:

$ cat a/a.go
package a
type A struct { _ int32 }

$ cat b.go
package b
import "./a"
var x a.A = struct{ _ int32 }{}

Compiling b succeeds even though the (anonymous) struct in b has a _ (blank) field from a different package (b) then the struct x (package a). But per the spec, such an assignment is invalid because the blank fields are different (i.e., not unique): they are not exported yet from different packages.

We should probably just fix this with the new export format.

@griesemer griesemer self-assigned this May 3, 2016
@griesemer griesemer added this to the Go1.7 milestone May 3, 2016
@robpike robpike changed the title cmd/compile: _ (blank) struct fields loose package info cmd/compile: _ (blank) struct fields lose package info May 3, 2016
@griesemer
Copy link
Contributor Author

This is a bug that's been around for a long time - no need to fix for 1.7. Should address this in new export format only after the textual export format is gone to avoid "mixed-mode" errors. Leaving for 1.8.

@griesemer griesemer modified the milestones: Go1.8Early, Go1.7 May 9, 2016
@griesemer
Copy link
Contributor Author

I cannot reproduce this anymore with Go1.6 (I don't think it was present in 1.6, the error case was probably flawed due to a buggy exporter/importer at that time).

This bug also doesn't exist in Go1.7 - at least given the above testcase. In fact, it happens to have been fixed inadvertently by https://go-review.googlesource.com/#/c/22714/6 ; specifically: https://go-review.googlesource.com/#/c/22714/6/src/cmd/compile/internal/gc/bimport.go, line 521:

The exporter doesn't export a package for the (unexported) _ field. The importer uses the builtin package for the _ field. Since any local struct type using a _ field will use the local package for _ field, an anonymous struct with a _ field and a structurally equal imported struct with _ field will have different packages for that field, leading to the expected error.

But the "optimization" of using the builtin package for imported _ fields is still incorrect because it's possible to export unnamed structs with _ fields which should also not be assignment compatible. Here's a test case:

$ cat a.go
package a
type A struct { _ int32 }

$ cat b.go
package b
func B() (_ struct { _ int32 }) { return }

$ cat c.go
package c
import "./a"
import "./b"
var _ a.A = b.B() // <<< assignment should fail

@griesemer
Copy link
Contributor Author

Here's another test case:

$ cat a.go
package a
func F() (_ struct { _ int32 }) { return }

$ cat b.go
package b
func F() (_ struct { _ int32 }) { return }

$ cat c.go
package c
import "./a"
import "./b"
var aa = a.F()
var bb = b.F()
var _ = aa == bb // <<< comparison should fail

go/types reports an error in this case when type-checking c.go:

$ gotype c.go 
c.go:6:9: cannot compare aa == bb (mismatched types struct{_ int32} and struct{_ int32})

which is correct. go/types can report an error because its importer assigns the package from which the struct is imported to the struct's _ field. (That's not correct in general either, because a struct may have been imported transitively and then gets the last package rather than the original one. But fixing this bug for cmd/compile in the export data will also fix this case for go/types).

gopherbot pushed a commit that referenced this issue Aug 23, 2016
Towards a fix for #15514.

Change-Id: I62073e9fdcfe5ddda9b0a47fe8554b524191a77c
Reviewed-on: https://go-review.googlesource.com/27638
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 23, 2016
Code from https://go-review.googlesource.com/#/c/27639/ .

Remain backward-compatible with possibly installed packages
that remain in Go1.6 format.

Change-Id: If424e7a881c81bbfcacf38c0946542793c406abd
Reviewed-on: https://go-review.googlesource.com/27640
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
zchee added a commit to zchee/gocode that referenced this issue Aug 24, 2016
- Original commit message
  Code from https://go-review.googlesource.com/#/c/27639/ .
  Remain backward-compatible with possibly installed packages
  that remain in Go1.6 format.

- backport of golang/tools@b2560d1

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@gopherbot
Copy link

CL https://golang.org/cl/27912 mentions this issue.

gopherbot pushed a commit that referenced this issue Aug 26, 2016
The Go1.7 export format didn't encode the field package for
blank struct fields (#15514). Re-introduce support for that
format so we can read it w/o error.

For #16881.

Change-Id: Ib131d41aac56dbf970aab15ae7e75ef3944b412d
Reviewed-on: https://go-review.googlesource.com/27912
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Aug 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants