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

reflect: Struct.PkgPath may be empty even if Name is unexported #21122

Closed
dsnet opened this issue Jul 21, 2017 · 6 comments
Closed

reflect: Struct.PkgPath may be empty even if Name is unexported #21122

dsnet opened this issue Jul 21, 2017 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@dsnet
Copy link
Member

dsnet commented Jul 21, 2017 •
edited

Since Go1.0, the following:

type x int
type S1 struct{ *x }
type S2 struct{ x }
fmt.Printf("%+v\n", reflect.TypeOf(S1{}).Field(0))
fmt.Printf("%+v\n", reflect.TypeOf(S2{}).Field(0))

Has printed the following:

{Name:x PkgPath: Type:*main.x Tag: Offset:0 Index:[0] Anonymous:true}
{Name:x PkgPath:main Type:main.x Tag: Offset:0 Index:[0] Anonymous:true}

Notice that StructField.PkgPath for an embedded unexported pointer type is empty, while it is populated for an unexported non-pointer type. This is surprising since the documentation for StructField.PkgPath says the following:

[PkgPath] is empty for upper case (exported) field names.

Although not explicitly stated, I would expect it to be non-empty for unexported field names.

We should either:

  • Document the special-case that PkgPath may be empty for anonymous fields of pointer types.
  • Fix StructField.PkgPath to be populated for embedded pointer types as well. However, this would change the behavior of what has been happening since Go1.0.

Given that the current behavior has persisted for 9 releases of Go, perhaps we should just document this? However, this difference in behavior has led to bugs (see #21121).

\cc @ianlancetaylor @rsc @crawshaw
@dsnet dsnet mentioned this issue Jul 21, 2017
Closed
@dsnet dsnet added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 21, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 21, 2017
@dsnet
Copy link
Member Author

dsnet commented Jul 21, 2017

Some evidence in support of changing the behavior. The following all make the assumption that PkgPath=="" if and only if the field is exported.

In the standard library, I did not find a single case where the current behavior is desired.

I glanced through a bunch of usages of this inside Google and it seems pervasive that people expect this property to be true.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jul 22, 2017
@dsnet
encoding/json: ignore embedded fields of pointers to unexported non-s…
b817359 
…tructs

https://golang.org/cl/33773 fixes the JSON marshaler to avoid serializing
embedded fields on unexported types of non-struct types. However, Go allows
embedding pointer to types, so the check for whether the field is a non-struct
type must first dereference the pointer to get at the underlying type.

Furthermore, due to a edge-case in the behavior of StructField.PkgPath not
being a reliable indicator of whether the field is unexported (see #21122),
we use our own logic to determine whether the field is exported or not.

The logic in this CL may be simplified depending on what happens in #21122.

Fixes #21121
Updates #21122

Change-Id: I8dfd1cdfac8a87950df294a566fb96dfd04fd749
Reviewed-on: https://go-review.googlesource.com/50711
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@abhinav abhinav mentioned this issue Jul 26, 2017
Merged
@gopherbot
Copy link

Change https://golang.org/cl/53643 mentions this issue: reflect: populate StructField.PkgPath on unexported embedded pointer types

@dsnet
Copy link
Member Author

dsnet commented Aug 8, 2017 •
edited

I ran my change through global testing (at Google) to detect regressions, and only 1 target failed as a result of this change.

The failure was in cloud.google.com/go/internal/fields, however I believe this is due to a different bug in reflect that is related (#21353).

Given that fixing this issue also fixes another bug in reflect, I have more confidence now that fixing this is the right action.

@dsnet dsnet mentioned this issue Aug 8, 2017
Closed
@dsnet dsnet added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 14, 2017
@dsnet dsnet 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

@gopherbot
Copy link

Change https://golang.org/cl/65550 mentions this issue: encoding/json: cleanup detection of unexported embedded fields

gopherbot pushed a commit that referenced this issue Sep 23, 2017
@dsnet
encoding/json: cleanup detection of unexported embedded fields
6872a8e 
CL 60410 fixes the compiler such that reflect.StructField.PkgPath
is non-empty if and only if the field is unexported.
Given that property, we can cleanup the logic in the json encoder
to avoid parsing the field name to detect export properties.

Updates #21122

Change-Id: Ic01b9c4ca76386774846b742b0c1b9b948f53e7c
Reviewed-on: https://go-review.googlesource.com/65550
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@mdempsky mdempsky mentioned this issue Sep 26, 2017
Closed
@golang golang locked and limited conversation to collaborators Sep 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
3 participants
@bradfitz @dsnet @gopherbot