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.

Sign up for GitHub

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: keep RO flags unchanged in Value.Addr #32787

Closed
wants to merge 3 commits into from
Closed

reflect: keep RO flags unchanged in Value.Addr #32787

wants to merge 3 commits into from

Conversation

kezhuw
Copy link
Contributor

@kezhuw kezhuw commented Jun 26, 2019 •
edited
Loading

Currently, Value.Addr collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr.Elem read only.

This commit fix this by keeping all bits of flagRO from origin
value in Value.Addr. This should be safe due to following reasons:

  • Result of Value.Addr is not CanSet because of it is not CanAddr
    but not flagRO.
  • Addr.Elem get same flagRO as origin, so it should behave same as
    origin in CanSet.

Fixes #32772.

@kezhuw
reflect: error if Value.Addr().Elem.CanSet() diff from Value.CanSet()
48e8569
@kezhuw
reflect: inherit all bits of flagRO in Value.Addr()
808aaea 
Currently, Value.Addr() collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr().Elem() read only.

This commit fix this by inheriting all bits of flagRO from origin
value in Value.Addr(). This should be safe due to following reasons:
* Result of Value.Addr() is not CanSet() because of it is not CanAddr()
  but not flagRO.
* Addr().Elem() get same flagRO as origin, so it should behave same as
  origin in CanSet().

Fixes golang#32772.
@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jun 26, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: 808aaea) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/183937 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 1:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@kezhuw kezhuw changed the title Fix lost flagEmbedRO in reflect.Value.Addr().Elem() reflect: keep RO flags unchanged in Value.Addr Jun 27, 2019
@gopherbot
Copy link
Contributor

Message from Kezhu Wang:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Kezhu Wang:

Patch Set 2:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 2:

Thanks, will review in the 1.14 release cycle.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot gopherbot force-pushed the master branch 2 times, most recently from a1b0af9 to 93a79bb Compare October 1, 2019 22:09
@gopherbot gopherbot force-pushed the master branch 6 times, most recently from 4a7ed1f to 0f992b9 Compare November 5, 2019 02:56
@gopherbot
Copy link
Contributor

Message from Ingo Oeser:

Patch Set 2:

@ian do you have time to review this for 1.15?

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 2:

@mdempsky this CL undoes one of the changes in https://golang.org/cl/66331. Any opinions on this? Thanks.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Matthew Dempsky:

Patch Set 2:

@mdempsky this CL undoes one of the changes in https://golang.org/cl/66331. Any opinions on this? Thanks.

Package reflect's model is: to access promoted, exported fields without restriction, you need to access them through an unbroken sequence of Field() and Elem() calls.

I don't immediately see any issues with extending that to allow interspersing .Addr().Elem() sequences, but I also don't see any rationale to make that change?

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Kezhu Wang:

Patch Set 2:

Patch Set 2:

@mdempsky this CL undoes one of the changes in https://golang.org/cl/66331. Any opinions on this? Thanks.

Package reflect's model is: to access promoted, exported fields without restriction, you need to access them through an unbroken sequence of Field() and Elem() calls.

I don't immediately see any issues with extending that to allow interspersing .Addr().Elem() sequences, but I also don't see any rationale to make that change?

From my side, the rationale is intuition that an addressable reflect.Value and its .Addr().Elem() should express same public behaviors, eg. calling .Addr().Elem() on it should have no side effects, no matter whether that reflect.Value is embedded or not.

Currently, exported fields from embedded struct field are CanSet(), after .Addr().Elem(), they are not CanSet(), and thus panic when SetXyz(). You can see https://play.golang.org/p/mKRa1-3j2He.

I found this when upgrading from 1.5 to 1.11, I think this may be an unaware breaking change. If this is a desired change, may be we should document it in package or function level as it is somewhat counterintuitive.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Matthew Dempsky:

Patch Set 2: Code-Review-2

I'm going to mark -2 for now. I don't foresee any technical issue with the CL, but I think we should discuss the idea first, and that discussion is better handled on the issue tracker.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Matthew Dempsky:

Patch Set 2: -Code-Review

(1 comment)

Sorry for the delays. Per discussion on #32772, we can move forward with this.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 78e280e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/183937 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Kezhu Wang:

Patch Set 2:

Patch Set 2: -Code-Review

(1 comment)

Sorry for the delays. Per discussion on #32772, we can move forward with this.

Done.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Matthew Dempsky:

Patch Set 3: Run-TryBot+1 Code-Review+2

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=8a36a3d7

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 3: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/183937.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request May 4, 2020
@kezhuw @mdempsky
reflect: keep RO flags unchanged in Value.Addr
4c003f6 
Currently, Value.Addr collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr.Elem read only.

This commit fix this by keeping all bits of flagRO from origin
value in Value.Addr. This should be safe due to following reasons:
* Result of Value.Addr is not CanSet because of it is not CanAddr
   but not flagRO.
* Addr.Elem get same flagRO as origin, so it should behave same as
   origin in CanSet.

Fixes #32772.

Change-Id: I79e086628c0fb6569a50ce63f3b95916f997eda1
GitHub-Last-Rev: 78e280e
GitHub-Pull-Request: #32787
Reviewed-on: https://go-review.googlesource.com/c/go/+/183937
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/183937 has been merged.

@gopherbot gopherbot closed this May 4, 2020
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this pull request May 21, 2020
@kezhuw @xujianhai666
reflect: keep RO flags unchanged in Value.Addr
3127708 
Currently, Value.Addr collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr.Elem read only.

This commit fix this by keeping all bits of flagRO from origin
value in Value.Addr. This should be safe due to following reasons:
* Result of Value.Addr is not CanSet because of it is not CanAddr
   but not flagRO.
* Addr.Elem get same flagRO as origin, so it should behave same as
   origin in CanSet.

Fixes golang#32772.

Change-Id: I79e086628c0fb6569a50ce63f3b95916f997eda1
GitHub-Last-Rev: 78e280e
GitHub-Pull-Request: golang#32787
Reviewed-on: https://go-review.googlesource.com/c/go/+/183937
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

reflect: Value.Addr().Elem() loses flagEmbedRO in origin value
3 participants
@kezhuw @gopherbot @googlebot