Skip to content

Commit

Permalink
Fix assignment analysis of ref fields
Browse files Browse the repository at this point in the history
  • Loading branch information
@jjonescz
jjonescz committed Oct 3, 2024
1 parent 0d41e66 commit 500e713
Show file tree
Hide file tree
Showing 8 changed files with 535 additions and 96 deletions.
16 changes: 13 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
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
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
66 changes: 35 additions & 31 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
View file
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: false);
}

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)
{
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,7 +963,9 @@ 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);
}

if (MayRequireTracking(fieldAccess.ReceiverOpt, fieldAccess.FieldSymbol))
Expand Down Expand Up @@ -1001,19 +1005,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 +1546,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 +1557,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 +1574,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 +1584,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 +1622,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 +1646,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 +1880,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: false);
}

if (parameter is SourceComplexParameterSymbolBase { ContainingSymbol: LocalFunctionSymbol or LambdaSymbol } sourceComplexParam)
Expand Down Expand Up @@ -2748,20 +2764,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: false);
}

return result;
}

public override BoundNode VisitDynamicObjectInitializerMember(BoundDynamicObjectInitializerMember node)
Expand Down Expand Up @@ -2790,7 +2794,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 +2804,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
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
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceAssemblySymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2742,7 +2742,7 @@ internal ImmutableArray<Diagnostic> GetUnusedFieldWarnings(CancellationToken can
}
else
{
diagnostics.Add(ErrorCode.WRN_UnassignedInternalField, field.GetFirstLocationOrNone(), field, DefaultValue(field.Type));
diagnostics.Add(ErrorCode.WRN_UnassignedInternalField, field.GetFirstLocationOrNone(), field, DefaultValue(field.Type, isRef: field.RefKind != RefKind.None));
}
}

Expand Down Expand Up @@ -2773,9 +2773,10 @@ internal ImmutableArray<Diagnostic> GetUnusedFieldWarnings(CancellationToken can
return _unusedFieldWarnings;
}

private static string DefaultValue(TypeSymbol type)
private static string DefaultValue(TypeSymbol type, bool isRef)
{
// TODO: localize these strings
if (isRef) return "(null reference)";
if (type.IsReferenceType) return "null";
switch (type.SpecialType)
{
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Test/Emit3/FlowAnalysis/RegionAnalysisTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
// ref int ri;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "ri").WithArguments("RS.ri", "(null reference)").WithLocation(3, 13),
// (4,20): warning CS9201: Ref field 'ri' should be ref-assigned before use.
// public RS() => ri = 0;
Diagnostic(ErrorCode.WRN_UseDefViolationRefField, "ri").WithArguments("ri").WithLocation(4, 20));
Expand Down
14 changes: 13 additions & 1 deletion src/Compilers/CSharp/Test/Emit3/Semantics/InlineArrayTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4676,7 +4676,10 @@ static async Task<T> FromResult<T>(T r)
}
";
var comp = CreateCompilation(src + Buffer10Definition, targetFramework: TargetFramework.Net80, options: TestOptions.ReleaseExe);
var verifier = CompileAndVerify(comp, expectedOutput: "111", verify: Verification.Fails).VerifyDiagnostics();
var verifier = CompileAndVerify(comp, expectedOutput: "111", verify: Verification.Fails).VerifyDiagnostics(
// (6,45): warning CS0649: Field 'C.F' is never assigned to, and will always have its default value
// public readonly Buffer10<Buffer10<int>> F;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "F").WithArguments("C.F", "").WithLocation(6, 45));

verifier.VerifyIL("Program.<M1>d__1.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext",
@"
Expand Down Expand Up @@ -4832,6 +4835,9 @@ static async Task<T> FromResult<T>(T r)
";
var comp = CreateCompilation(src + Buffer10Definition, targetFramework: TargetFramework.Net80, options: TestOptions.ReleaseExe);
comp.VerifyEmitDiagnostics(
// (8,45): warning CS0649: Field 'C.F' is never assigned to, and will always have its default value
// public readonly Buffer10<Buffer10<int>> F;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "F").WithArguments("C.F", "").WithLocation(8, 45),
// (20,12): error CS4007: Instance of type 'System.ReadOnlySpan<Buffer10<int>>' cannot be preserved across 'await' or 'yield' boundary.
// => MemoryMarshal.CreateReadOnlySpan(
Diagnostic(ErrorCode.ERR_ByRefTypeAndAwait, @"MemoryMarshal.CreateReadOnlySpan(
Expand Down Expand Up @@ -4887,6 +4893,9 @@ static async Task<T> FromResult<T>(T r)
";
var comp = CreateCompilation(src + Buffer10Definition, targetFramework: TargetFramework.Net80, options: TestOptions.ReleaseExe);
comp.VerifyEmitDiagnostics(
// (8,45): warning CS0649: Field 'C.F' is never assigned to, and will always have its default value
// public readonly Buffer10<Buffer10<int>> F;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "F").WithArguments("C.F", "").WithLocation(8, 45),
// (20,12): error CS4007: Instance of type 'System.ReadOnlySpan<int>' cannot be preserved across 'await' or 'yield' boundary.
// => MemoryMarshal.CreateReadOnlySpan(
Diagnostic(ErrorCode.ERR_ByRefTypeAndAwait, @"MemoryMarshal.CreateReadOnlySpan(
Expand Down Expand Up @@ -4940,6 +4949,9 @@ static async Task<T> FromResult<T>(T r)
";
var comp = CreateCompilation(src + Buffer10Definition, targetFramework: TargetFramework.Net80, options: TestOptions.ReleaseExe);
comp.VerifyEmitDiagnostics(
// (8,45): warning CS0649: Field 'C.F' is never assigned to, and will always have its default value
// public readonly Buffer10<Buffer10<int>> F;
Diagnostic(ErrorCode.WRN_UnassignedInternalField, "F").WithArguments("C.F", "").WithLocation(8, 45),
// (20,12): error CS8178: A reference returned by a call to 'Program.GetItem(ReadOnlySpan<Buffer10<int>>, int)' cannot be preserved across 'await' or 'yield' boundary.
// => GetItem(MemoryMarshal.CreateReadOnlySpan(ref Unsafe.As<Buffer10<Buffer10<int>>, Buffer10<int>>(ref Unsafe.AsRef(in GetC(x).F)),10), Get01())[await FromResult(Get02(x))];
Diagnostic(ErrorCode.ERR_RefReturningCallAndAwait, "GetItem(MemoryMarshal.CreateReadOnlySpan(ref Unsafe.As<Buffer10<Buffer10<int>>, Buffer10<int>>(ref Unsafe.AsRef(in GetC(x).F)),10), Get01())").WithArguments("Program.GetItem(System.ReadOnlySpan<Buffer10<int>>, int)").WithLocation(20, 12)
Expand Down
Loading

0 comments on commit 500e713

Please sign in to comment.