Fix assignment analysis of ref fields #75340
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
force-pushed
the
75315-RefFieldNeverAssigned
branch
from
9727861
to
500e713
Compare
| _sourceAssembly.NoteFieldAccess(field, read: value == null || WriteConsideredUse(fieldAccess.FieldSymbol.Type, value), write: true); | ||
| _sourceAssembly.NoteFieldAccess(field, | ||
| read: value == null || WriteConsideredUse(fieldAccess.FieldSymbol.Type, value), | ||
| write: field.RefKind == RefKind.None || isRef); |
There was a problem hiding this comment.
Wondering about how this interacts with the following logic which performs the same check on the receiver of the field access. e.g. to say that when a struct field is written, the containing struct is also written.
Would it be the most robust to isRef = false; just after n = fieldAccess.ReceiverOpt;? Or, perhaps to make some assertion about the ref kind within the MayRequireTracking branch. Basically, this would express that we don't expect the receiver of a ref field access, to be itself a ref field access, at any level of nesting.
I'm conflicted as to whether any change is needed at all here. Curious what you think.
There was a problem hiding this comment.
Good catch, I think isRef = false should be there. Even though it would be an error scenario today to have a ref field to a ref struct, it could be allowed in the future, and the warnings can be observed even today. And if we are ref-assigning (i.e., isRef = true) we don't want to propagate that to the receiver (we are not ref assigning the receiver).
| { | ||
| // TODO: localize these strings | ||
| if (isRef) return "(null reference)"; |
There was a problem hiding this comment.
I wish we had a name for this which didn't get shredded when attempting to search online for it.
| @@ -14109,6 +14109,9 @@ ref struct RS | |||
| """, | |||
| targetFramework: TargetFramework.NetCoreApp); | |||
| comp.VerifyEmitDiagnostics( | |||
| // (3,13): warning CS0649: Field 'RS.ri' is never assigned to, and will always have its default value (null reference) | |||
There was a problem hiding this comment.
The wording of the diagnostic feels a little confusing. Perhaps it would make sense to introduce a new message for ref fields which indicates the need to ref-assign.
There was a problem hiding this comment.
Sounds good. Does this wording seem fine?
Field '{0}' is never ref-assigned to, and will always have its default value (null reference)
There was a problem hiding this comment.
Btw, technically that will be a new warning that can be reported for existing code - even where it wasn't previously reported due to the bug being fixed - but also where it was previously reported it will now be under a different error code. So should it be part of c# 14 warning wave?
Alternatively we could keep the current wording, that's why I added the (null reference) for the ref field case so it's clearer.
Or maybe just change the existing message to have the assigned/ref-assigned as another parameter, so the error code remains the same. (But still it could be reported for new locations, so maybe a separate error code in a warning wave would be better?)
| @@ -346,7 +346,7 @@ protected override ImmutableArray<PendingBranch> Scan(ref bool badRegion) | |||
| // All primary constructor parameters are definitely assigned outside of the primary constructor | |||
| foreach (var parameter in primaryConstructor.Parameters) | |||
| { | |||
| NoteWrite(parameter, value: null, read: true); | |||
| NoteWrite(parameter, value: null, read: true, isRef: false); | |||
There was a problem hiding this comment.
Does this behave correctly when a primary constructor parameter is by-ref? For example, do ref parameters in primary constructors behave consistently with by-value parameters in the dataflow APIs?
There was a problem hiding this comment.
I don't think this should affect dataflow APIs in any way, since the isRef is only passed down to _sourceAssembly.NoteFieldAccess which isn't part of dataflow but is only used to report the "unused field" diagnostics IIUC. Moreover, it's only used if the symbol is a field but here it's a parameter, so that code can't be reached.
That being said, we might use the parameter elsewhere in the future, so it seems better to pass isRef: parameter.RefKind != RefKind.None here and in other similar places.
| if ((object)_sourceAssembly != null && node.MemberSymbol != null && node.MemberSymbol.Kind == SymbolKind.Field) | ||
| { | ||
| _sourceAssembly.NoteFieldAccess((FieldSymbol)node.MemberSymbol.OriginalDefinition, read: false, write: true); | ||
| NoteWrite(iterationVariable, null, read: true, isRef: false); |
There was a problem hiding this comment.
Does this behave correctly (e.g. is there any behavior change in the dataflow APIs) for a case like the following?
void M(Span<int> span) {
foreach (ref var x in span) { }
}There was a problem hiding this comment.
Similar response as above, there should be no behavior change, since iterationVariable is a LocalSymbol, hence isRef has no effect on it.
Fixes #75315.
A field cannot be assigned-to via a readonly reference, but previously we considered any reference to be a write and hence didn't emit the "field is never assigned" warning.
Similarly we didn't distinguish between ref assignments and value assignments of ref fields - now we consider only ref assignments to be writes to ref fields.
This also improves the warning to say that any never-assigned ref field will have its default value set to "null reference".