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

Fix assignment analysis of ref fields #75340

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Fix assignment analysis of ref fields #75340

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
500e713
Fix assignment analysis of ref fields
jjonescz Oct 3, 2024
253e28e
Avoid propagating ref assignment
jjonescz Oct 4, 2024
d759195
Set `isRef` to more future-proof values in more places
jjonescz Oct 4, 2024
3005947
Use separate warning
jjonescz Oct 8, 2024
db62221
Document `isRef` parameter
jjonescz Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/compilers/CSharp/Warnversion Warning Waves.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ In a typical project, this setting is controlled by the `AnalysisLevel` property
which determines the `WarningLevel` property (passed to the `Csc` task).
For more information on `AnalysisLevel`, see https://devblogs.microsoft.com/dotnet/automatically-find-latent-bugs-in-your-code-with-net-5/

## Warning level 10

The compiler shipped with .NET 10 (the C# 14 compiler) contains the following warnings which are reported only under `/warn:10` or higher.

| Warning ID | Description |
|------------|-------------|
| CS9265 | [Field is never ref-assigned to, and will always have its default value (null reference)](https://github.com/dotnet/roslyn/issues/75315) |

## Warning level 8

The compiler shipped with .NET 8 (the C# 12 compiler) contains the following warnings which are reported only under `/warn:8` or higher.
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1916,6 +1916,12 @@ If such a class is used as a base class and if the deriving class defines a dest
<data name="WRN_UnassignedInternalField_Title" xml:space="preserve">
<value>Field is never assigned to, and will always have its default value</value>
</data>
<data name="WRN_UnassignedInternalRefField" xml:space="preserve">
<value>Field '{0}' is never ref-assigned to, and will always have its default value (null reference)</value>
</data>
<data name="WRN_UnassignedInternalRefField_Title" xml:space="preserve">
<value>Field is never ref-assigned to, and will always have its default value (null reference)</value>
</data>
<data name="ERR_CStyleArray" xml:space="preserve">
<value>Bad array declarator: To declare a managed array the rank specifier precedes the variable's identifier. To declare a fixed size buffer field, use the fixed keyword before the field type.</value>
</data>
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2346,6 +2346,7 @@ internal enum ErrorCode
ERR_PartialPropertyDuplicateInitializer = 9263,

WRN_UninitializedNonNullableBackingField = 9264,
WRN_UnassignedInternalRefField = 9265,

// Note: you will need to do the following after adding errors:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ internal static int GetWarningLevel(ErrorCode code)
// docs/compilers/CSharp/Warnversion Warning Waves.md
switch (code)
{
case ErrorCode.WRN_UnassignedInternalRefField:
// Warning level 10 is exclusively for warnings introduced in the compiler
// shipped with dotnet 10 (C# 14) and that can be reported for pre-existing code.
return 10;
case ErrorCode.WRN_AddressOfInAsync:
case ErrorCode.WRN_ByValArraySizeConstRequired:
// Warning level 8 is exclusively for warnings introduced in the compiler
Expand Down Expand Up @@ -2460,6 +2464,7 @@ or ErrorCode.ERR_CannotApplyOverloadResolutionPriorityToOverride
or ErrorCode.ERR_CannotApplyOverloadResolutionPriorityToMember
or ErrorCode.ERR_PartialPropertyDuplicateInitializer
or ErrorCode.WRN_UninitializedNonNullableBackingField
or ErrorCode.WRN_UnassignedInternalRefField
=> false,
};
#pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.
Expand Down
16 changes: 13 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1545,10 +1545,20 @@ private void VisitArgumentsAfterCall(ImmutableArray<BoundExpression> arguments,
for (int i = 0; i < arguments.Length; i++)
{
RefKind refKind = GetRefKind(refKindsOpt, i);
// passing as a byref argument is also a potential write
if (refKind != RefKind.None)
switch (refKind)
{
WriteArgument(arguments[i], refKind, method);
case RefKind.None:
case RefKind.In:
case RefKind.RefReadOnlyParameter:
case RefKindExtensions.StrictIn:
break;
case RefKind.Ref:
case RefKind.Out:
// passing as a byref argument is also a potential write
WriteArgument(arguments[i], refKind, method);
break;
default:
throw ExceptionUtilities.UnexpectedValue(refKind);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowsOutWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ protected override void EnterRegion()
base.EnterRegion();
}

protected override void NoteWrite(Symbol variable, BoundExpression value, bool read)
protected override void NoteWrite(Symbol variable, BoundExpression value, bool read, bool isRef)
{
// any reachable assignment to a ref or out parameter can be visible to the caller in the face of exceptions.
if (this.State.Reachable && IsInside)
Expand All @@ -107,7 +107,7 @@ protected override void NoteWrite(Symbol variable, BoundExpression value, bool r
#endif
}

base.NoteWrite(variable, value, read);
base.NoteWrite(variable, value, read: read, isRef: isRef);
}

#if DEBUG
Expand Down
67 changes: 36 additions & 31 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: parameter.RefKind != RefKind.None);
}

CurrentSymbol = save;
Expand Down Expand Up @@ -846,15 +846,17 @@ private void NoteRead(BoundNode fieldOrEventAccess)
}
}

protected virtual void NoteWrite(Symbol variable, BoundExpression value, bool read)
protected virtual void NoteWrite(Symbol variable, BoundExpression value, bool read, bool isRef)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does isRef mean literally ref or any kind like in / out. Consider adding a comment here to make the intent clear.

Copy link
Member Author

@jjonescz jjonescz Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means "ref assignment", so there's no in/out variant. Added a comment.

{
if ((object)variable != null)
{
_writtenVariables.Add(variable);
if ((object)_sourceAssembly != null && variable.Kind == SymbolKind.Field)
{
var field = (FieldSymbol)variable.OriginalDefinition;
_sourceAssembly.NoteFieldAccess(field, read: read && WriteConsideredUse(field.Type, value), write: true);
_sourceAssembly.NoteFieldAccess(field,
read: read && WriteConsideredUse(field.Type, value),
write: field.RefKind == RefKind.None || isRef);
}

var local = variable as LocalSymbol;
Expand Down Expand Up @@ -949,7 +951,7 @@ internal static bool WriteConsideredUse(TypeSymbol type, BoundExpression value)
}
}

private void NoteWrite(BoundExpression n, BoundExpression value, bool read)
private void NoteWrite(BoundExpression n, BoundExpression value, bool read, bool isRef)
{
while (n != null)
{
Expand All @@ -961,12 +963,15 @@ private void NoteWrite(BoundExpression n, BoundExpression value, bool read)
if ((object)_sourceAssembly != null)
{
var field = fieldAccess.FieldSymbol.OriginalDefinition;
_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);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}

if (MayRequireTracking(fieldAccess.ReceiverOpt, fieldAccess.FieldSymbol))
{
n = fieldAccess.ReceiverOpt;
isRef = false;
if (n.Kind == BoundKind.Local)
{
_usedVariables.Add(((BoundLocal)n).LocalSymbol);
Expand Down Expand Up @@ -1001,19 +1006,19 @@ private void NoteWrite(BoundExpression n, BoundExpression value, bool read)
}

case BoundKind.ThisReference:
NoteWrite(MethodThisParameter, value, read);
NoteWrite(MethodThisParameter, value, read: read, isRef: isRef);
return;

case BoundKind.Local:
NoteWrite(((BoundLocal)n).LocalSymbol, value, read);
NoteWrite(((BoundLocal)n).LocalSymbol, value, read: read, isRef: isRef);
return;

case BoundKind.Parameter:
NoteWrite(((BoundParameter)n).ParameterSymbol, value, read);
NoteWrite(((BoundParameter)n).ParameterSymbol, value, read: read, isRef: isRef);
return;

case BoundKind.RangeVariable:
NoteWrite(((BoundRangeVariable)n).Value, value, read);
NoteWrite(((BoundRangeVariable)n).Value, value, read: read, isRef: isRef);
return;

case BoundKind.InlineArrayAccess:
Expand Down Expand Up @@ -1542,7 +1547,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
SetSlotState(slot, assigned: written || !this.State.Reachable);
}

if (written) NoteWrite(pattern.VariableAccess, value, read);
if (written) NoteWrite(pattern.VariableAccess, value, read: read, isRef: isRef);
break;
}

Expand All @@ -1553,7 +1558,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
LocalSymbol symbol = local.LocalSymbol;
int slot = GetOrCreateSlot(symbol);
SetSlotState(slot, assigned: written || !this.State.Reachable);
if (written) NoteWrite(symbol, value, read);
if (written) NoteWrite(symbol, value, read: read, isRef: isRef);
break;
}

Expand All @@ -1570,7 +1575,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
{
int slot = MakeSlot(local);
SetSlotState(slot, written);
if (written) NoteWrite(local, value, read);
if (written) NoteWrite(local, value, read: read, isRef: isRef);
}
break;
}
Expand All @@ -1580,7 +1585,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
var elementAccess = (BoundInlineArrayAccess)node;
if (written)
{
NoteWrite(elementAccess.Expression, value: null, read);
NoteWrite(elementAccess.Expression, value: null, read: read, isRef: isRef);
}

if (elementAccess.Expression.Type.HasInlineArrayAttribute(out int length) &&
Expand Down Expand Up @@ -1618,7 +1623,19 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is

int slot = MakeSlot(paramExpr);
SetSlotState(slot, written);
if (written) NoteWrite(paramExpr, value, read);
if (written) NoteWrite(paramExpr, value, read: read, isRef: isRef);
break;
}

case BoundKind.ObjectInitializerMember:
{
var member = (BoundObjectInitializerMember)node;
if (_sourceAssembly is not null && member.MemberSymbol is FieldSymbol field)
{
_sourceAssembly.NoteFieldAccess(field.OriginalDefinition,
read: false,
write: field.RefKind == RefKind.None || isRef);
}
break;
}

Expand All @@ -1630,7 +1647,7 @@ protected virtual void AssignImpl(BoundNode node, BoundExpression value, bool is
var expression = (BoundExpression)node;
int slot = MakeSlot(expression);
SetSlotState(slot, written);
if (written) NoteWrite(expression, value, read);
if (written) NoteWrite(expression, value, read: read, isRef: isRef);
break;
}

Expand Down Expand Up @@ -1864,7 +1881,7 @@ protected override void EnterParameter(ParameterSymbol parameter)
{
// this code has no effect except in region analysis APIs such as DataFlowsOut where we unassign things
if (slot > 0) SetSlotState(slot, true);
NoteWrite(parameter, value: null, read: true);
NoteWrite(parameter, value: null, read: true, isRef: parameter.RefKind != RefKind.None);
}

if (parameter is SourceComplexParameterSymbolBase { ContainingSymbol: LocalFunctionSymbol or LambdaSymbol } sourceComplexParam)
Expand Down Expand Up @@ -2748,20 +2765,8 @@ public override void VisitForEachIterationVariables(BoundForEachStatement node)
int slot = GetOrCreateSlot(iterationVariable);
if (slot > 0) SetSlotAssigned(slot);
// NOTE: do not report unused iteration variables. They are always considered used.
NoteWrite(iterationVariable, null, read: true);
}
}

public override BoundNode VisitObjectInitializerMember(BoundObjectInitializerMember node)
{
var result = base.VisitObjectInitializerMember(node);

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: iterationVariable.RefKind != RefKind.None);
}

return result;
}

public override BoundNode VisitDynamicObjectInitializerMember(BoundDynamicObjectInitializerMember node)
Expand Down Expand Up @@ -2790,7 +2795,7 @@ protected override void AfterVisitInlineArrayAccess(BoundInlineArrayAccess node)
if (node.GetItemOrSliceHelper == WellKnownMember.System_Span_T__Slice_Int_Int)
{
// exposing ref is a potential write
NoteWrite(node.Expression, value: null, read: false);
NoteWrite(node.Expression, value: null, read: false, isRef: false);
}
}

Expand All @@ -2800,7 +2805,7 @@ protected override void AfterVisitConversion(BoundConversion node)
node.Type.OriginalDefinition.Equals(compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions))
{
// exposing ref is a potential write
NoteWrite(node.Operand, value: null, read: false);
NoteWrite(node.Operand, value: null, read: false, isRef: false);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/ReadWriteWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,11 @@ protected override void NoteRead(Symbol variable, ParameterSymbol rangeVariableU
base.NoteRead(variable, rangeVariableUnderlyingParameter);
}

protected override void NoteWrite(Symbol variable, BoundExpression value, bool read)
protected override void NoteWrite(Symbol variable, BoundExpression value, bool read, bool isRef)
{
if ((object)variable == null) return;
(IsInside ? _writtenInside : _writtenOutside).Add(variable);
base.NoteWrite(variable, value, read);
base.NoteWrite(variable, value, read: read, isRef: isRef);
}

protected override void CheckAssigned(BoundExpression expr, FieldSymbol fieldSymbol, SyntaxNode node)
Expand Down Expand Up @@ -233,7 +233,7 @@ protected override void AssignImpl(BoundNode node, BoundExpression value, bool i
switch (node.Kind)
{
case BoundKind.RangeVariable:
if (written) NoteWrite(((BoundRangeVariable)node).RangeVariableSymbol, value, read);
if (written) NoteWrite(((BoundRangeVariable)node).RangeVariableSymbol, value, read: read, isRef: isRef);
break;

case BoundKind.QueryClause:
Expand All @@ -242,7 +242,7 @@ protected override void AssignImpl(BoundNode node, BoundExpression value, bool i
var symbol = ((BoundQueryClause)node).DefinedSymbol;
if ((object)symbol != null)
{
if (written) NoteWrite(symbol, value, read);
if (written) NoteWrite(symbol, value, read: read, isRef: isRef);
}
}
break;
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2740,6 +2740,10 @@ internal ImmutableArray<Diagnostic> GetUnusedFieldWarnings(CancellationToken can
{
diagnostics.Add(ErrorCode.WRN_UnreferencedField, field.GetFirstLocationOrNone(), field);
}
else if (field.RefKind != RefKind.None)
{
diagnostics.Add(ErrorCode.WRN_UnassignedInternalRefField, field.GetFirstLocationOrNone(), field);
}
else
{
diagnostics.Add(ErrorCode.WRN_UnassignedInternalField, field.GetFirstLocationOrNone(), field, DefaultValue(field.Type));
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading