Fix modification of readonly locals through ref fields #74255
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
|
||
| ref struct R(ref V v) | ||
| { | ||
| public ref V V = ref v; |
There was a problem hiding this comment.
The submitted repro has these as readonly ref fields. Let's add some more tests with different variants of readonlyness sprinkled throughout. It would also be good to test variables that come from ref foreach loops.
| @@ -60,6 +60,7 @@ private static bool NeedsByValueFieldAccess(BoundExpression? receiver, FieldSymb | |||
| { | |||
| if (fieldSymbol.IsStatic || | |||
| !fieldSymbol.ContainingType.IsValueType || | |||
| fieldSymbol.RefKind is RefKind.Ref || | |||
There was a problem hiding this comment.
In L76 we are effectively checking RefKind != RefKind.None on the localSymbol, should we also do that here for the fieldSymbol? Is it possible we need this when we have a ref-readonly field whose referent value is changing "during" the expression?
There was a problem hiding this comment.
If we have a ref readonly field, there are binding errors if we try to change its referent value, so I think we cannot observe the difference (the IsByValue computed here is used by Binder.HasHome in codegen to determine whether the field should be stored in a temp but we never get there if we have binding errors). But it makes sense to me to check what you suggest.
| public void MutatingThroughRefFields_01( | ||
| [CombinatorialValues("ref", "")] string eRef, | ||
| [CombinatorialValues("readonly", "")] string vReadonly) | ||
| { |
There was a problem hiding this comment.
Consider adding a smoke test for LINQ iteration variables and using variables. They have the same "can't assign" semantics as foreach iteration variables and hence are possibly subject to the same issue.
There was a problem hiding this comment.
There are tests for using variables added in this PR in CodeGenUsingStatementTests.cs.
LINQ iteration variables aren't affected by this bug (the iteration variable is not a LocalSymbol so the BoundFieldAccess has always IsByValue set to false), but I will add similar tests.
|
@jjonescz when will this be publically available can I test this somehow? |
|
It will be in the .NET 9.0 SDK and VS 17.12 |
If you want to test sooner you can install this package: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet-tools/NuGet/Microsoft.Net.Compilers.Toolset |
Not all bugs meet the bar. In this case it seems to me better not to backport it since it's a behavioral (i.e., silent) breaking change. |
Imho it is not really a breaking change rather than a bug fix of sth that should actually work. It's quite unfortunate because many users will skip .Net 9 because it is not an LTS would be really nice to see this back in .NET 8 but I guess I have to wait till .Net 10 then which is the next LTS. |
Fixes #73741.