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: wrong logic for pkg derivation of unnamed type #21696

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

Comments

Projects
None yet
4 participants
@rsc
Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Aug 30, 2017

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

@gopherbot gopherbot closed this in d349fa2 Sep 5, 2017

@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.
You can’t perform that action at this time.