Skip to content

Commit

Permalink
Reworked secondary locations
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tim-Pohlmann
Tim-Pohlmann committed Aug 10, 2023
1 parent bb7d235 commit 4137efa
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 60 deletions.
17 changes: 3 additions & 14 deletions analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,7 @@ public override void Visit(SyntaxNode node)
}
}

protected override bool IsLeftCoalesceExpression(SyntaxNode syntax) =>
syntax.Parent is BinaryExpressionSyntax { } binary
&& binary.OperatorToken.IsKind(SyntaxKind.QuestionQuestionToken)
&& binary.Left == syntax;

protected override bool IsConditionalAccessExpression(SyntaxNode syntax) =>
syntax.Parent is ConditionalAccessExpressionSyntax conditional && conditional.Expression == syntax;

protected override bool IsForLoopIncrementor(SyntaxNode syntax) =>
syntax.Parent is ForStatementSyntax forStatement && forStatement.Incrementors.Contains(syntax);

protected override bool IsUsing(SyntaxNode syntax) =>
(syntax.IsKind(SyntaxKind.VariableDeclaration) && syntax.Parent.IsKind(SyntaxKind.UsingStatement))
|| (syntax is LocalDeclarationStatementSyntax local && local.UsingKeyword().IsKind(SyntaxKind.UsingKeyword));
protected override bool IsInsideUsingDeclaration(SyntaxNode node) =>
(node.IsKind(SyntaxKind.VariableDeclaration) && node.Parent.IsKind(SyntaxKind.UsingStatement))
|| (node is LocalDeclarationStatementSyntax local && local.UsingKeyword().IsKind(SyntaxKind.UsingKeyword));
}
67 changes: 34 additions & 33 deletions ...arAnalyzer.Common/SymbolicExecution/Roslyn/RuleChecks/ConditionEvaluatesToConstantBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,9 @@ public abstract class ConditionEvaluatesToConstantBase : SymbolicRuleCheck

private readonly Dictionary<IOperation, BasicBlock> trueOperations = new();
private readonly Dictionary<IOperation, BasicBlock> falseOperations = new();
private readonly HashSet<IOperation> reached = new();
private readonly List<IOperation> reached = new();

protected abstract bool IsLeftCoalesceExpression(SyntaxNode syntax);
protected abstract bool IsConditionalAccessExpression(SyntaxNode syntax);
protected abstract bool IsForLoopIncrementor(SyntaxNode syntax);
protected abstract bool IsUsing(SyntaxNode syntax);
protected abstract bool IsInsideUsingDeclaration(SyntaxNode node);

public override ProgramState[] PreProcess(SymbolicContext context)
{
Expand All @@ -55,7 +52,7 @@ public override ProgramState ConditionEvaluated(SymbolicContext context)
{
var operation = context.Operation.Instance;
if (operation.Kind is not OperationKindEx.Literal
&& !operation.Syntax.Ancestors().Any(IsUsing)
&& !operation.Syntax.Ancestors().Any(IsInsideUsingDeclaration)
&& operation.TrackedSymbol(context.State) is not IFieldSymbol { IsConst: true }
&& !IsDiscardPattern(operation))
{
Expand Down Expand Up @@ -93,61 +90,66 @@ public override void ExecutionCompleted()
private void ReportIssue(IOperation operation, BasicBlock block, bool conditionValue)
{
var issueMessage = operation.Kind == OperationKindEx.IsNull ? MessageNull : string.Format(MessageBool, conditionValue);
var secondaryLocations = SecondaryLocations(block, conditionValue);
var syntax = ToBranchValueCondition(operation.Syntax);
var secondaryLocations = SecondaryLocations(block, conditionValue, syntax);
if (secondaryLocations.Any())
{
ReportIssue(Rule2583, operation, secondaryLocations, issueMessage, S2583MessageSuffix);
ReportIssue(Rule2583, syntax, secondaryLocations, issueMessage, S2583MessageSuffix);
}
else
{
ReportIssue(Rule2589, operation, null, issueMessage, string.Empty);
ReportIssue(Rule2589, syntax, null, issueMessage, string.Empty);
}
}

private List<Location> SecondaryLocations(BasicBlock block, bool conditionValue)
private List<Location> SecondaryLocations(BasicBlock block, bool conditionValue, SyntaxNode conditionSyntax)
{
List<Location> locations = new();
IOperation currentStart = null;
IOperation currentEnd = null;
var unreachable = UnreachableOperations(block, conditionValue).Where(x => !IsIgnoredLocation(x.Syntax)).ToHashSet();
foreach (var operation in unreachable.Concat(reached).OrderBy(x => x.Syntax.SpanStart))
var unreachable = UnreachableOperations(block, conditionValue).ToHashSet();
var currentStart = conditionSyntax.Span.End;
var reachedNodes = reached.Select(x => x.Syntax).Where(x => x.SpanStart > conditionSyntax.Span.End).OrderBy(x => x.SpanStart);

foreach (var reachedNode in reachedNodes)
{
if (unreachable.Contains(operation))
{
currentStart ??= operation;
currentEnd = operation;
}
else
if (AddLocation(reachedNode.SpanStart))
{
AddCurrent();
currentStart = reachedNode.Span.End;
}
}
AddCurrent();
AddLocation(int.MaxValue);
return locations;

void AddCurrent()
bool AddLocation(int end)
{
if (currentStart is not null)
var nodes = unreachable.Where(x => x.SpanStart > currentStart && x.Span.End < end);
if (nodes.Any())
{
locations.Add(currentStart.Syntax.CreateLocation(currentEnd.Syntax));
currentStart = null;
var first = nodes.OrderBy(x => x.SpanStart).First();
var last = nodes.OrderBy(x => x.Span.End).Last();
locations.Add(first.CreateLocation(last));
return true;
}
return false;
}
}

private IEnumerable<IOperation> UnreachableOperations(BasicBlock block, bool conditionValue)
private IEnumerable<SyntaxNode> UnreachableOperations(BasicBlock block, bool conditionValue)
{
if (block.SuccessorBlocks.Distinct().Count() != 2)
{
return Enumerable.Empty<IOperation>();
return Enumerable.Empty<SyntaxNode>();
}
HashSet<BasicBlock> reachable = new() { block };
HashSet<BasicBlock> unreachable = new();

var conditionalIsRechable = (block.ConditionKind == ControlFlowConditionKind.WhenTrue) == conditionValue;
Traverse(conditionalIsRechable ? block.ConditionalSuccessor : block.FallThroughSuccessor, reachable, new List<BasicBlock>());
Traverse(conditionalIsRechable ? block.FallThroughSuccessor : block.ConditionalSuccessor, unreachable, reachable);
return unreachable.SelectMany(x => x.OperationsAndBranchValue).Except(reached);
return unreachable
.SelectMany(x => x.OperationsAndBranchValue)
.Except(reached)
.SelectMany(x => x.DescendantsAndSelf().Select(x => x.Syntax))
.ToList();

static void Traverse(ControlFlowBranch branch, HashSet<BasicBlock> result, ICollection<BasicBlock> excluded)
{
Expand All @@ -168,8 +170,7 @@ static void Traverse(ControlFlowBranch branch, HashSet<BasicBlock> result, IColl
}
}

private bool IsIgnoredLocation(SyntaxNode x) =>
IsForLoopIncrementor(x)
|| IsConditionalAccessExpression(x)
|| IsLeftCoalesceExpression(x);
// For SwitchExpressionArms like `true => 5` we are only interested in the left part (`true`).
private static SyntaxNode ToBranchValueCondition(SyntaxNode syntax) =>
syntax.IsKind(SyntaxKindEx.SwitchExpressionArm) ? ((SwitchExpressionArmSyntaxWrapper)syntax).Pattern : syntax;
}
7 changes: 5 additions & 2 deletions analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/SymbolicRuleCheck.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,12 @@ protected void ReportIssue(IOperation operation, IEnumerable<Location> additiona
ReportIssue(Rule, operation, additionalLocations, messageArgs);
}

protected void ReportIssue(DiagnosticDescriptor rule, IOperation operation, IEnumerable<Location> additionalLocations, params object[] messageArgs)
protected void ReportIssue(DiagnosticDescriptor rule, IOperation operation, IEnumerable<Location> additionalLocations, params object[] messageArgs) =>
ReportIssue(rule, operation.Syntax, additionalLocations, messageArgs);

protected void ReportIssue(DiagnosticDescriptor rule, SyntaxNode syntax, IEnumerable<Location> additionalLocations, params object[] messageArgs)
{
var location = operation.Syntax.GetLocation();
var location = syntax.GetLocation();
if (reportedDiagnostics.Add(location))
{
context.ReportIssue(Diagnostic.Create(rule, location, additionalLocations, messageArgs));
Expand Down
15 changes: 8 additions & 7 deletions ...lyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.CSharp8.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ public class CSharp8
int SwitchExpression()
{
var a = false;
return a switch // Secondary FP
return a switch
{
true => 0, // Noncompliant: true branch is always false
// Secondary@-1
true => 0,
// ^^^^ Noncompliant
// ^ Secondary@-1
false => 1 // Noncompliant: false branch is always true
};
}
Expand Down Expand Up @@ -234,8 +235,9 @@ void NullCoalesceAssignment_Useless(string a, string b, string c, string d)

//Left operand: Values notNull, notEmpty and ret are known to be not-null
ret = notNull;
ret ??= a; // Noncompliant
// Secondary@-1
ret ??= a;
// ^^^ Noncompliant
// ^ Secondary@-1

ret = notNull;
ret = "Lorem " + (ret ??= a) + " ipsum"; // Noncompliant
Expand All @@ -255,7 +257,6 @@ void NullCoalesceAssignment_Useless(string a, string b, string c, string d)

ret = null;
ret = "Lorem " + (ret ??= a) + " ipsum"; // Noncompliant
// Secondary@-1

//Right operand: isNull is known to be null, therefore ?? is useless
ret = a;
Expand Down Expand Up @@ -460,7 +461,7 @@ void IfStatement()

void LogicalAndExpression()
{
bool a = true;
bool a = false;
var b = a && true; // Noncompliant
// Secondary@-1
}
Expand Down
15 changes: 11 additions & 4 deletions ...SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ public void NotExecutedLoops(object o1, object o2, object o3)
// ^^

for (int i = 0; c3; i++) // Noncompliant {{Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.}}
// ^^
// Secondary@-1 ^33#239
{
if (o3 != null) // Secondary
if (o3 != null)
// secondary location starts at incrementor and ends at the end of the above line
break;
}
}
Expand Down Expand Up @@ -176,6 +177,13 @@ public void Foo7(bool a, bool b)
}
}

public void Foo8(bool a, bool b)
{
a = true;
_ = a && b;
// ^ Noncompliant
}

void Pointer(int* a) // Error [CS0214]
{
if (a != null) // Error [CS0214]
Expand Down Expand Up @@ -1917,7 +1925,7 @@ public static void NonCompliant6()
S sObj = null;
if (sObj?.str?.Length > 2)
// ^^^^ Noncompliant
// ^^^^^^^ Secondary@-1
// ^^^^^^^^^^^^ Secondary@-1
// ^^^^^^^^^^^^^^^^^^^^^ Noncompliant@-2
{
Console.WriteLine("a"); // Secondary
Expand Down Expand Up @@ -2742,7 +2750,6 @@ void NullCoalesce_Useless(string a, string b, string c, string d)
ret = null ?? a; // Noncompliant
ret = isNull ?? a; // Noncompliant
ret = ((isNull)) ?? a; // Noncompliant
// Secondary@-1 FP
ret = "Lorem " + (isNull ?? a) + " ipsum"; // Noncompliant

//Right operand: isNull is known to be null, therefore ?? is useless
Expand Down

0 comments on commit 4137efa

Please sign in to comment.