-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix ref safe context of assignments to be their RHS #80633
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
Conversation
src/Compilers/CSharp/Test/Emit3/Symbols/UserDefinedCompoundAssignmentOperatorsTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Symbols/UserDefinedCompoundAssignmentOperatorsTests.cs
Outdated
Show resolved
Hide resolved
|
Briefly glanced over changes (commit 2), not a complete review pass #Closed |
|
@AlekseyTs @dotnet/roslyn-compiler for reviews, thanks |
AlekseyTs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 4), but I recommend getting a sign-off from ref-safety experts.
|
@dotnet/roslyn-compiler for reviews, thanks |
src/Compilers/CSharp/Test/Semantic/Semantics/RefLocalsAndReturnsTests.cs
Show resolved
Hide resolved
RikkiGibson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree that the change being made here is correct, in the sense that it doesn't open any ref safety holes, and removes ref safety errors which were "unnecessary".
It also looks like this decision is a reversal of ref safety changes made in #24904, which were themselves a reversal of a prior state where the ref-safe-context of an assignment was the RHS.
In my view this emphasizes that this area needs to be well-specified. Basically I am OK with letting this PR in as it is now, but, let's make sure that before too long we get the behavior we actually want recorded in the standard. I would be happy to work together on this.
Interesting find, thanks. Note that there are already some places where RHS is used, it's just not very consistent. For example:
The standard has this section, but it doesn't seem to answer what the safe-context should be for assignment operators I think:
|
Fixes #79054.
Closes #73549.