Skip to content

Commit

Permalink
Fix S1854 FN: Proper support of try/catch statements (#9450)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pavel-mikula-sonarsource
pavel-mikula-sonarsource authored Jun 21, 2024
1 parent f0a203a commit d5db7c1
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 130 deletions.
149 changes: 60 additions & 89 deletions analyzers/src/SonarAnalyzer.CSharp/Common/Walkers/MutedSyntaxWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,110 +18,81 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Common.Walkers
namespace SonarAnalyzer.Common.Walkers;

/// <summary>
/// This class find syntax cases that are not properly supported by current CFG/SE/LVA and we mute all issues related to these scenarios.
/// </summary>
internal class MutedSyntaxWalker : CSharpSyntaxWalker
{
/// <summary>
/// This class find syntax cases that are not properly supported by current CFG/SE/LVA and we mute all issues related to these scenarios.
/// </summary>
internal class MutedSyntaxWalker : CSharpSyntaxWalker
{
// All kinds that SonarAnalysisContextExtensions.RegisterExplodedGraphBasedAnalysis registers for
private static readonly SyntaxKind[] RootKinds =
[
SyntaxKind.ConstructorDeclaration,
SyntaxKind.DestructorDeclaration,
SyntaxKind.ConversionOperatorDeclaration,
SyntaxKind.OperatorDeclaration,
SyntaxKind.MethodDeclaration,
SyntaxKind.PropertyDeclaration,
SyntaxKind.GetAccessorDeclaration,
SyntaxKind.SetAccessorDeclaration,
SyntaxKindEx.InitAccessorDeclaration,
SyntaxKind.AddAccessorDeclaration,
SyntaxKind.RemoveAccessorDeclaration,
SyntaxKind.AnonymousMethodExpression,
SyntaxKind.SimpleLambdaExpression,
SyntaxKind.ParenthesizedLambdaExpression
];
// All kinds that SonarAnalysisContextExtensions.RegisterExplodedGraphBasedAnalysis registers for
private static readonly SyntaxKind[] RootKinds =
[
SyntaxKind.AddAccessorDeclaration,
SyntaxKind.AnonymousMethodExpression,
SyntaxKind.ConstructorDeclaration,
SyntaxKind.ConversionOperatorDeclaration,
SyntaxKind.DestructorDeclaration,
SyntaxKind.GetAccessorDeclaration,
SyntaxKindEx.InitAccessorDeclaration,
SyntaxKind.MethodDeclaration,
SyntaxKind.OperatorDeclaration,
SyntaxKind.ParenthesizedLambdaExpression,
SyntaxKind.PropertyDeclaration,
SyntaxKind.RemoveAccessorDeclaration,
SyntaxKind.SetAccessorDeclaration,
SyntaxKind.SimpleLambdaExpression
];

private readonly SemanticModel semanticModel;
private readonly SyntaxNode root;
private readonly ISymbol[] symbols;
private bool isMuted;
private bool isInTryOrCatch;
private bool isOutsideTryCatch;
private readonly SemanticModel model;
private readonly SyntaxNode root;
private readonly ISymbol[] symbols;
private bool isMuted;

public MutedSyntaxWalker(SemanticModel semanticModel, SyntaxNode node)
: this(semanticModel, node, node.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Select(x => semanticModel.GetSymbolInfo(x).Symbol).WhereNotNull().ToArray()) { }
public MutedSyntaxWalker(SemanticModel model, SyntaxNode node)
: this(model, node, node.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Select(x => model.GetSymbolInfo(x).Symbol).WhereNotNull().ToArray()) { }

public MutedSyntaxWalker(SemanticModel semanticModel, SyntaxNode node, params ISymbol[] symbols)
{
this.semanticModel = semanticModel;
this.symbols = symbols;
root = node.Ancestors().FirstOrDefault(x => x.IsAnyKind(RootKinds));
}
public MutedSyntaxWalker(SemanticModel model, SyntaxNode node, params ISymbol[] symbols)
{
this.model = model;
this.symbols = symbols;
root = node.Ancestors().FirstOrDefault(x => x.IsAnyKind(RootKinds));
}

public bool IsMuted()
public bool IsMuted()
{
if (symbols.Any() && root is not null)
{
if (symbols.Any() && root != null)
{
Visit(root);
}
return IsMutedState();
Visit(root);
}
return isMuted;
}

public override void Visit(SyntaxNode node)
public override void Visit(SyntaxNode node)
{
if (!isMuted) // Performance optimization, we can stop visiting once we know the answer
{
if (!IsMutedState()) // Performance optimization, we can stop visiting once we know the answer
{
base.Visit(node);
}
base.Visit(node);
}
}

public override void VisitIdentifierName(IdentifierNameSyntax node)
public override void VisitIdentifierName(IdentifierNameSyntax node)
{
if (Array.Find(symbols, x => node.NameIs(x.Name) && x.Equals(model.GetSymbolInfo(node).Symbol)) is { } symbol)
{
if (Array.Find(symbols, x => node.NameIs(x.Name) && x.Equals(semanticModel.GetSymbolInfo(node).Symbol)) is { } symbol)
{
isMuted = IsInTupleAssignmentTarget() || IsUsedInLocalFunction(symbol) || IsInUnsupportedExpression();
InspectTryCatch(node);
}
base.VisitIdentifierName(node);

bool IsInTupleAssignmentTarget() =>
node.Parent is ArgumentSyntax argument && argument.IsInTupleAssignmentTarget();

bool IsUsedInLocalFunction(ISymbol symbol) =>
// We don't mute it if it's declared and used in local function
!(symbol.ContainingSymbol is IMethodSymbol containingSymbol && containingSymbol.MethodKind == MethodKindEx.LocalFunction)
&& node.HasAncestor(SyntaxKindEx.LocalFunctionStatement);

bool IsInUnsupportedExpression() =>
node.FirstAncestorOrSelf<SyntaxNode>(x => x.IsAnyKind(SyntaxKindEx.IndexExpression, SyntaxKindEx.RangeExpression)) != null;
isMuted = IsInTupleAssignmentTarget() || IsUsedInLocalFunction(symbol) || IsInUnsupportedExpression();
}
base.VisitIdentifierName(node);

public override void VisitVariableDeclarator(VariableDeclaratorSyntax node)
{
if (Array.Find(symbols, x => node.Identifier.ValueText == x.Name && x.Equals(semanticModel.GetDeclaredSymbol(node))) is not null)
{
InspectTryCatch(node);
}
base.VisitVariableDeclarator(node);
}
bool IsInTupleAssignmentTarget() =>
node.Parent is ArgumentSyntax argument && argument.IsInTupleAssignmentTarget();

private bool IsMutedState() =>
isMuted || (isInTryOrCatch && isOutsideTryCatch);
bool IsUsedInLocalFunction(ISymbol symbol) =>
// We don't mute it if it's declared and used in local function
!(symbol.ContainingSymbol is IMethodSymbol containingSymbol && containingSymbol.MethodKind == MethodKindEx.LocalFunction)
&& node.HasAncestor(SyntaxKindEx.LocalFunctionStatement);

private void InspectTryCatch(SyntaxNode node)
{
if (node.HasAncestor(SyntaxKind.TryStatement))
{
// We're only interested in "try" and "catch" blocks. Don't count "finally" block
isInTryOrCatch = isInTryOrCatch || !node.HasAncestor(SyntaxKind.FinallyClause);
}
else
{
isOutsideTryCatch = true;
}
}
bool IsInUnsupportedExpression() =>
node.FirstAncestorOrSelf<SyntaxNode>(x => x.IsAnyKind(SyntaxKindEx.IndexExpression, SyntaxKindEx.RangeExpression)) is not null;
}
}
32 changes: 16 additions & 16 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,22 +186,22 @@ var x // Noncompliant
void calculateRate2(int a, int b)
{
var x = 0; // Compliant, ignored value
x = 1; // FN, muted due to try/catch
x = 1; // Noncompliant
try
{
x = 11; // FN, muted due to try/catch
x = 11; // Noncompliant
x = 12;
Console.Write(x);
x = 13; // FN, muted due to try/catch
x = 13; // Noncompliant
}
catch (Exception)
{
x = 21; // FN, muted due to try/catch
x = 21; // Noncompliant
x = 22;
Console.Write(x);
x = 23; // FN, muted due to try/catch
x = 23; // Noncompliant
}
x = 31; // FN, muted due to try/catch
x = 31; // Noncompliant
}

void storeI(int i) { }
Expand Down Expand Up @@ -824,9 +824,9 @@ internal void Finally_Followed_By_Deadstore(object actor)
}
finally
{
actor = null; // FN, muted due to try/catch
actor = null; // Noncompliant
}
actor = null; // FN, muted due to try/catch
actor = null; // Noncompliant
}

internal void SnippetTwo(object actor)
Expand Down Expand Up @@ -1149,7 +1149,7 @@ public int Start()
}
catch (SystemException e)
{
exception = e; // FN, muted by try/catch
exception = e; // Noncompliant
}

return exitCode;
Expand Down Expand Up @@ -1180,22 +1180,22 @@ public class Verify2607
public static void DeadStore(int[] array)
{
var x = 0; // Compliant, ignored value
x = array[^1]; // FN, muted due to try/catch
x = array[^1]; // Noncompliant
try
{
x = 11; // FN, muted due to try/catch
x = 11; // Noncompliant
x = 12;
Console.Write(x);
x = 13; // FN, muted due to try/catch
x = 13; // Noncompliant
}
catch (Exception)
{
x = 21; // FN, muted due to try/catch
x = 21; // Noncompliant
x = 22;
Console.Write(x);
x = 23; // FN, muted due to try/catch
x = 23; // Noncompliant
}
x = 31; // FN, muted due to try/catch
x = 31; // Noncompliant
}
}

Expand Down Expand Up @@ -1473,7 +1473,7 @@ public void UsedInFinally_Throw_FilteredCatch()

public void UsedInFinally_Throw_CatchAll()
{
var value = 42; // FN
var value = 42; // Noncompliant
try
{
throw new Exception();
Expand Down
40 changes: 20 additions & 20 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.SonarCfg.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,22 +149,22 @@ var x
void calculateRate2(int a, int b)
{
var x = 0;
x = 1; // FN, muted due to try/catch
x = 1; // Noncompliant
try
{
x = 11; // FN, muted due to try/catch
x = 11; // Noncompliant
x = 12;
Console.Write(x);
x = 13; // Compliant, Console.Write could throw and value is read fater try/catch
x = 13; // Noncompliant
}
catch (Exception)
{
x = 21; // FN, muted due to try/catch
x = 21; // Noncompliant
x = 22;
Console.Write(x);
x = 23; // FN, muted due to try/catch
x = 23; // Noncompliant
}
x = 31; // FN, muted due to try/catch
x = 31; // Noncompliant
}

void storeI(int i) { }
Expand Down Expand Up @@ -628,9 +628,9 @@ internal void Finally_Followed_By_Deadstore(object actor)
}
finally
{
actor = null; // FN, muted due to try/catch
actor = null; // Noncompliant
}
actor = null; // FN, muted due to try/catch
actor = null; // Noncompliant
}

internal void SnippetTwo(object actor)
Expand Down Expand Up @@ -834,7 +834,7 @@ public static class ReproIssues
public static long WithConstantValue(string path)
{
const int unknownfilelength = -1;
long length = unknownfilelength; // Compliant
long length = unknownfilelength; // Noncompliant FP
try
{
length = new System.IO.FileInfo(path).Length;
Expand Down Expand Up @@ -863,7 +863,7 @@ public static long WithMinus1(string path)
const int UNKNOWN = -1;
public static long WithClassConstant(string path)
{
long length = UNKNOWN; // Compliant
long length = UNKNOWN; // Noncompliant FP
try
{
length = new System.IO.FileInfo(path).Length;
Expand All @@ -878,7 +878,7 @@ public static long WithClassConstant(string path)
// https://github.com/SonarSource/sonar-dotnet/issues/2598
public static string WithCastedNull(string path)
{
var length = (string)null; // Compliant
var length = (string)null; // Noncompliant FP
try
{
length = new System.IO.FileInfo(path).Length.ToString();
Expand Down Expand Up @@ -911,7 +911,7 @@ public class FooBarBaz
public int Start()
{
const int x = -1;
int exitCode = x; // Compliant
int exitCode = x; // Noncompliant FP
Exception exception = null;

try
Expand All @@ -922,7 +922,7 @@ public int Start()
}
catch (SystemException e)
{
exception = e; // FN, muted by try/catch
exception = e; // Noncompliant
}

return exitCode;
Expand Down Expand Up @@ -953,22 +953,22 @@ public class Verify2607
public static void DeadStore(int[] array)
{
var x = 0;
x = array[^1]; // FN, muted due to try/catch
x = array[^1]; // Noncompliant
try
{
x = 11; // FN, muted due to try/catch
x = 11; // Noncompliant
x = 12;
Console.Write(x);
x = 13; // Compliant, Console.Write can throw
x = 13; // Noncompliant
}
catch (Exception)
{
x = 21; // FN, muted due to try/catch
x = 21; // Noncompliant
x = 22;
Console.Write(x);
x = 23; // FN, muted due to try/catch
x = 23; // Noncompliant
}
x = 31; // FN, muted due to try/catch
x = 31; // Noncompliant
}
}

Expand Down Expand Up @@ -1110,7 +1110,7 @@ public int WithTryCatch()
try
{
var values = GetValues();
name = values.name; // Compliant, DoWork can throw
name = values.name; // Noncompliant FP, DoWork can throw

DoWork();
return 5;
Expand Down
Loading

0 comments on commit d5db7c1

Please sign in to comment.