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

Jump to bottom

cmd/compile: wrong logic for pkg derivation of unnamed type #21696

Closed
rsc opened this issue Aug 30, 2017 · 5 comments
Closed

cmd/compile: wrong logic for pkg derivation of unnamed type #21696

rsc opened this issue Aug 30, 2017 · 5 comments
Labels
FrozenDueToAge release-blocker
Milestone
Go1.10

Comments

@rsc
Copy link
Contributor

rsc commented Aug 30, 2017

Happened to be working in cmd/compile/internal/gc/reflect.go and saw:

		// Unnamed type. Grab the package from the first field, if any.
		for _, f := range t.Fields().Slice() {
			if f.Embedded != 0 {
				continue
			}
			pkg = f.Sym.Pkg
			break
		}

I don't believe this is completely correct. There is no guarantee that the first non-embedded field will have a package. The commit message refers to

struct { i int }

which the code handles correctly, but I think the code still mishandles

struct { X int; i int }

because it will look at X, which has no Pkg, instead of at i, which does. It should probably continue the loop until it finds something with a pkg.

/cc @crawshaw @griesemer @mdempsky
@rsc rsc added this to the Go1.10 milestone Aug 30, 2017
@mdempsky
Copy link
Member

From what I can tell, the package we select there is just for a space optimization. Picking the wrong package means we waste space in the reflect type information tables (and thus should still be fixed), but I don't think it affects correctness at least.

@rsc
Copy link
Contributor Author

rsc commented Aug 30, 2017

@mdempsky the test in CL 27791, which introduced this code, is testing semantics observable via reflect and mentions #16616, so I don't think this is only about space usage.

@ianlancetaylor
Copy link
Contributor

It's not obvious to me that anything is wrong here. The f.Sym.Pkg field here is a compiler internal data structure. It's not the same as the reflect data. The fact that exported fields have no package in the reflect data is handled later, in dnameField.

@mdempsky
Copy link
Member

@rsc fixedbugs/issue16616.go still passes after removing that loop though.

@mdempsky mdempsky mentioned this issue Aug 30, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/60410 mentions this issue: cmd/compile: fix and improve struct field reflect information

@golang golang locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
4 participants
@mdempsky @rsc @ianlancetaylor @gopherbot