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.

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

Detect assignments to FlowCaptureReference operations #9535

Merged
merged 2 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions analyzers/its/expected/CSharpLatest/S1854-CSharpLatest-net8.0.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027person\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp11Features/RequiredMembers.cs#L20",
"Location": "Line 20 Position 9-51"
},
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027a\u0027.",
Expand Down Expand Up @@ -35,6 +41,18 @@
"Message": "Remove this useless assignment to local variable \u0027result\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S2437.cs#L10",
"Location": "Line 10 Position 9-30"
},
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027a\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S3240.cs#L12",
"Location": "Line 12 Position 9-32"
},
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027y\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S3440.cs#L8-L12",
"Location": "Lines 8-12 Position 9-10"
}
]
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027sender\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Actor/Futures.cs#L353",
"Location": "Line 353 Position 13-50"
},
{
"Id": "S1854",
"Message": "Remove this useless assignment to local variable \u0027actor\u0027.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,16 @@ public RoslynLiveVariableAnalysis(ControlFlowGraph cfg, CancellationToken cancel
Analyze();
}

public ISymbol ParameterOrLocalSymbol(IOperation operation)
public IEnumerable<ISymbol> ParameterOrLocalSymbols(IOperation operation)
{
ISymbol candidate = operation switch
var candidates = operation switch
{
_ when operation.AsParameterReference() is { } parameterReference => parameterReference.Parameter,
_ when operation.AsLocalReference() is { } localReference => localReference.Local,
_ => null
_ when operation.AsParameterReference() is { } parameterReference => [parameterReference.Parameter],
_ when operation.AsLocalReference() is { } localReference => [localReference.Local],
_ when operation.AsFlowCaptureReference() is { } flowCaptureReference => flowCaptures.TryGetValue(flowCaptureReference.Id, out var symbols) ? symbols : [],
_ => []
};
return IsLocal(candidate) ? candidate : null;
return candidates.Where(x => IsLocal(x));
}

public override bool IsLocal(ISymbol symbol) =>
Expand Down Expand Up @@ -90,15 +91,15 @@ private void ResolveCaptures()
if (flowCapture.Value.AsFlowCaptureReference() is { } captureReference
&& flowCaptures.TryGetValue(captureReference.Id, out var symbols))
{
AppendFlowCaptureReference(flowCapture.Id, symbols.ToArray());
AppendFlowCaptureReference(flowCapture.Id, symbols);
}
else if (ParameterOrLocalSymbol(flowCapture.Value) is { } symbol)
else
{
AppendFlowCaptureReference(flowCapture.Id, symbol);
AppendFlowCaptureReference(flowCapture.Id, ParameterOrLocalSymbols(flowCapture.Value));
}
}

void AppendFlowCaptureReference(CaptureId id, params ISymbol[] symbols)
void AppendFlowCaptureReference(CaptureId id, IEnumerable<ISymbol> symbols)
{
if (!flowCaptures.TryGetValue(id, out var list))
{
Expand Down Expand Up @@ -281,27 +282,23 @@ private void ProcessOperation(ControlFlowGraph cfg, IOperation operation)

private void ProcessParameterOrLocalReference(IOperationWrapper reference)
{
if (owner.ParameterOrLocalSymbol(reference.WrappedOperation) is { } symbol)
var symbols = owner.ParameterOrLocalSymbols(reference.WrappedOperation);
if (reference.IsOutArgument())
{
if (reference.IsOutArgument())
{
Assigned.Add(symbol);
UsedBeforeAssigned.Remove(symbol);
}
else if (!reference.IsAssignmentTarget())
{
UsedBeforeAssigned.Add(symbol);
}
Assigned.UnionWith(symbols);
UsedBeforeAssigned.ExceptWith(symbols);
}
else if (!reference.IsAssignmentTarget())
{
UsedBeforeAssigned.UnionWith(symbols);
}
}

private void ProcessSimpleAssignment(ISimpleAssignmentOperationWrapper assignment)
{
if (owner.ParameterOrLocalSymbol(assignment.Target) is { } localTarget)
{
Assigned.Add(localTarget);
UsedBeforeAssigned.Remove(localTarget);
}
var targets = owner.ParameterOrLocalSymbols(assignment.Target);
Assigned.UnionWith(targets);
UsedBeforeAssigned.ExceptWith(targets);
}

private void ProcessFlowAnonymousFunction(ControlFlowGraph cfg, IFlowAnonymousFunctionOperationWrapper anonymousFunction)
Expand All @@ -316,9 +313,10 @@ private void ProcessCaptured(ControlFlowGraph cfg)
{
foreach (var operation in cfg.Blocks.SelectMany(x => x.OperationsAndBranchValue).SelectMany(x => x.DescendantsAndSelf()))
{
if (owner.ParameterOrLocalSymbol(operation) is { } symbol)
var symbols = owner.ParameterOrLocalSymbols(operation);
if (symbols.Any())
{
Captured.Add(symbol);
Captured.UnionWith(symbols);
}
else
{
Expand Down
51 changes: 21 additions & 30 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/DeadStores.RoslynCfg.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,25 +73,21 @@ public override void AnalyzeBlock()

private void ProcessParameterOrLocalReference(IOperationWrapper reference)
{
if (owner.lva.ParameterOrLocalSymbol(reference.WrappedOperation) is { } symbol && IsSymbolRelevant(symbol))
var symbols = owner.lva.ParameterOrLocalSymbols(reference.WrappedOperation).Where(x => IsSymbolRelevant(x));
if (reference.IsOutArgument())
{
if (reference.IsOutArgument())
{
liveOut.Remove(symbol);
}
else if (!reference.IsAssignmentTarget() && !reference.IsCompoundAssignmentTarget())
{
liveOut.Add(symbol);
}
liveOut.ExceptWith(symbols);
}
else if (!reference.IsAssignmentTarget() && !reference.IsCompoundAssignmentTarget())
{
liveOut.UnionWith(symbols);
}
}

private void ProcessSimpleAssignment(ISimpleAssignmentOperationWrapper assignment)
{
if (ProcessAssignment(assignment, assignment.Target, assignment.Value) is { } localTarget)
{
liveOut.Remove(localTarget);
}
var targets = ProcessAssignment(assignment, assignment.Target, assignment.Value);
liveOut.ExceptWith(targets);
}

private void ProcessCompoundAssignment(ICompoundAssignmentOperationWrapper assignment) =>
Expand All @@ -106,43 +102,38 @@ private void ProcessDeconstructionAssignment(IDeconstructionAssignmentOperationW
{
foreach (var tupleElement in ITupleOperationWrapper.FromOperation(deconstructionAssignment.Target).AllElements())
{
if (ProcessAssignment(deconstructionAssignment, tupleElement) is { } target)
{
liveOut.Remove(target);
}
var targets = ProcessAssignment(deconstructionAssignment, tupleElement);
liveOut.ExceptWith(targets);
}
}
}

private ISymbol ProcessAssignment(IOperationWrapper operation, IOperation target, IOperation value = null)
private ISymbol[] ProcessAssignment(IOperationWrapper operation, IOperation target, IOperation value = null)
{
if (owner.lva.ParameterOrLocalSymbol(target) is { } localTarget && IsSymbolRelevant(localTarget))
var targets = owner.lva.ParameterOrLocalSymbols(target).Where(IsSymbolRelevant).ToArray();
foreach (var localTarget in targets)
{
if (TargetRefKind() == RefKind.None
&& !IsConst()
if (TargetRefKind(localTarget) == RefKind.None
&& !IsConst(localTarget)
&& !liveOut.Contains(localTarget)
&& !IsAllowedInitialization()
&& !IsAllowedInitialization(localTarget)
&& !ICaughtExceptionOperationWrapper.IsInstance(value)
&& !target.Syntax.Parent.IsKind(SyntaxKind.ForEachStatement)
&& !IsMuted(target.Syntax, localTarget))
{
ReportIssue(operation.WrappedOperation.Syntax.GetLocation(), localTarget);
}
return localTarget;
}
else
{
return null;
}
return targets;

bool IsConst() =>
static bool IsConst(ISymbol localTarget) =>
localTarget is ILocalSymbol local && local.IsConst;

RefKind TargetRefKind() =>
static RefKind TargetRefKind(ISymbol localTarget) =>
// Only ILocalSymbol and IParameterSymbol can be returned by ParameterOrLocalSymbol
localTarget is ILocalSymbol local ? local.RefKind() : ((IParameterSymbol)localTarget).RefKind;

bool IsAllowedInitialization() =>
bool IsAllowedInitialization(ISymbol localTarget) =>
operation.WrappedOperation.Syntax is VariableDeclaratorSyntax variableDeclarator
&& variableDeclarator.Initializer != null
// Avoid collision with S1481: Unused is allowed. Used only in local function is also unused in current CFG.
Expand Down
1 change: 1 addition & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/DeadStores.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ protected override void Initialize(SonarAnalysisContext context)
SyntaxKind.OperatorDeclaration,
SyntaxKind.RemoveAccessorDeclaration,
SyntaxKind.SetAccessorDeclaration,
SyntaxKindEx.CoalesceAssignmentExpression,
SyntaxKindEx.InitAccessorDeclaration,
SyntaxKindEx.LocalFunctionStatement);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ public void Assignment(DeadStores sender)
string coa = SomeString();
coa ??= SomeString();
Use(coa);
coa ??= SomeString(); // Roslyn CFG FN: Branching with FlowCaptureOperation
coa ??= SomeString(); // Noncompliant
}

public void Unary()
Expand Down Expand Up @@ -650,9 +650,9 @@ private void ConditionalEvaluation(bool b1, bool b2, object coalesce, object coa
var x = false; // Compliant ignored value
x = true; // Roslyn CFG FN: Consequence of inaccurate LVA state below
x = b1 && b2; // Roslyn CFG FN: Branching with FlowCaptureOperation
x = b1 || b2; // Roslyn CFG FN: Branching with FlowCaptureOperation
coalesce = coalesce ?? "Value"; // Roslyn CFG FN: Branching with FlowCaptureOperation
coalesceAssignment ??= "Value"; // Roslyn CFG FN: Branching with FlowCaptureOperation
x = b1 || b2; // Noncompliant
coalesce = coalesce ?? "Value"; // Noncompliant
coalesceAssignment ??= "Value"; // Noncompliant
}

private void SimpleAssignment()
Expand Down Expand Up @@ -776,7 +776,7 @@ int[] Compute(int[] arr)
var lst = arr; //Compliant
var unused = arr;
lst ??= new int[0];
unused ??= new int[0]; // Roslyn CFG FN: Branching with FlowCaptureOperation
unused ??= new int[0]; // Noncompliant
return lst;
}
}
Expand Down
Loading