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

encoding/binary: Read and Write have asymmetric value for unexported struct fields #7482

Closed
bgilmore opened this issue Mar 6, 2014 · 6 comments
Closed

encoding/binary: Read and Write have asymmetric value for unexported struct fields #7482

bgilmore opened this issue Mar 6, 2014 · 6 comments
Labels
FrozenDueToAge

Comments

@bgilmore
Copy link

bgilmore commented Mar 6, 2014 

What does 'go version' print?

  go version go1.2 linux/amd64


What steps reproduce the problem?

  Use binary.Write to marshal a struct with an unexported field, then attempt to unmarshal the resulting bytes using binary.Read
  
  http://play.golang.org/p/YelgiZZFKD


What happened?

  The reflect module panics when attempting to set the unexported field value, e.g:

  "panic: reflect: reflect.Value.SetInt using value obtained using unexported field"


What should have happened instead?

  The documentation clearly states unnamed fields are written out zeroed, but no verbiage exists for unexported fields.

  Some potential ways forward:

    * unexported fields could be written zeroed and skipped during reading (treat as unnamed fields).
    * unexported fields could be skipped during reading and writing.
    * continue to panic, but add explicit documentation about unexported field handling.


See also:

  Blank field handling added in https://golang.org/cl/6750053
@minux
Copy link
Member

minux commented Mar 6, 2014

Comment 1:

i think ideally Write should just skip unexported fields, however, that is an API change.
so at this stage, we actually have only two options:
1. skip unexported fields during read (and document that),
2. panic as usual, document it.
I think option 1 is the way to go.
What do you think?

@bgilmore
Copy link
Author

bgilmore commented Mar 6, 2014

Comment 2:

rsc objected to not-panicking in during Read in message #3 on the linked change, and we
don't want to change the output of Write, so I think this may be best addressed as a doc
fix.
That being said, I agree that Write should be skipping (or zeroing) unexported fields.
Requiring developers to implement BinaryMarshaler and BinaryUnmarshaler to get symmetric
behavior is unfortunate.

@ianlancetaylor
Copy link
Contributor

Comment 3:

Owner changed to @ianlancetaylor.

Status changed to Started.

@gopherbot
Copy link

Comment 4:

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

@ianlancetaylor
Copy link
Contributor

Comment 5:

This issue was closed by revision c00804c.

Status changed to Fixed.

@ianlancetaylor
Copy link
Contributor

Comment 6:

Issue #8315 has been merged into this issue.

@bgilmore bgilmore added the fixed label Jul 2, 2014
@ianlancetaylor ianlancetaylor mentioned this issue Dec 8, 2014
Closed
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bgilmore @minux @ianlancetaylor @gopherbot