Skip to content

Commit

Permalink
Replaced name comparison with semantic model symbol checks
Browse files Browse the repository at this point in the history
  • Loading branch information
@CristianAmbrosini
CristianAmbrosini committed Feb 7, 2023
1 parent f37bcc9 commit 38a68cb
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 37 deletions.
27 changes: 15 additions & 12 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase<SyntaxKind, Va
{
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.GetName();
protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration);

protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) =>
declaration.Initializer is not null
Expand All @@ -48,26 +48,29 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase<SyntaxKind, Va
? GetTopLevelStatementSyntax<InterpolationSyntax>(declaration)
: declaration.Parent.Parent.Parent.DescendantNodes().OfType<InterpolationSyntax>().ToList();

protected override bool IsStringBuilderAccessed(string variableName, IList<InvocationExpressionSyntax> invocations) =>
invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableName));
protected override bool IsStringBuilderContentRead(IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableSymbol, semanticModel));

protected override bool IsPassedToMethod(string variableName, IList<InvocationExpressionSyntax> invocations) =>
invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName)));
protected override bool IsPassedToMethod(IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableSymbol, semanticModel)));

protected override bool IsReturned(string variableName, IList<ReturnStatementSyntax> returnStatements) =>
returnStatements.Any(x => IsSameVariable(x.Expression, variableName));
protected override bool IsReturned(IList<ReturnStatementSyntax> returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) =>
returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel));

protected override bool IsWithinInterpolatedString(string variableName, IList<InterpolationSyntax> interpolations) =>
interpolations.Any(x => IsSameVariable(x.Expression, variableName));
protected override bool IsWithinInterpolatedString(IList<InterpolationSyntax> interpolations, ISymbol variableSymbol, SemanticModel semanticModel) =>
interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel));

private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) =>
(expression is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression))
&& ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel);

private static bool IsSameVariable(ExpressionSyntax expression, string variableName) =>
expression.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => p.NameIs(variableName))
private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) =>
expression.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => IsSameVariable(p, variableSymbol, semanticModel))
|| (expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().Any() && expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().First()
.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => p.NameIs(variableName)));
.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => IsSameVariable(p, variableSymbol, semanticModel)));

private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) =>
variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol);

private static IList<T> GetTopLevelStatementSyntax<T>(VariableDeclaratorSyntax declaration)
{
Expand Down
20 changes: 10 additions & 10 deletions analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ public abstract class UnusedStringBuilderBase<TSyntaxKind, TVariableDeclarator,

internal readonly string[] StringBuilderAccessMethods = { "ToString", "CopyTo", "GetChunks" };

protected abstract string GetVariableName(TVariableDeclarator declaration);
protected abstract ISymbol GetSymbol(TVariableDeclarator declaration, SemanticModel semanticModel);
protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel);
protected abstract IList<TInvocationExpression> GetInvocations(TVariableDeclarator declaration);
protected abstract IList<TReturnStatement> GetReturnStatements(TVariableDeclarator declaration);
protected abstract IList<TInterpolatedString> GetInterpolatedStrings(TVariableDeclarator declaration);
protected abstract bool IsStringBuilderAccessed(string variableName, IList<TInvocationExpression> invocations);
protected abstract bool IsPassedToMethod(string variableName, IList<TInvocationExpression> invocations);
protected abstract bool IsReturned(string variableName, IList<TReturnStatement> returnStatements);
protected abstract bool IsWithinInterpolatedString(string variableName, IList<TInterpolatedString> interpolations);
protected abstract bool IsStringBuilderContentRead(IList<TInvocationExpression> invocations, ISymbol variableSymbol, SemanticModel semanticModel);
protected abstract bool IsPassedToMethod(IList<TInvocationExpression> invocations, ISymbol variableSymbol, SemanticModel semanticModel);
protected abstract bool IsReturned(IList<TReturnStatement> returnStatements, ISymbol variableSymbol, SemanticModel semanticModel);
protected abstract bool IsWithinInterpolatedString(IList<TInterpolatedString> interpolations, ISymbol variableSymbol, SemanticModel semanticModel);

protected UnusedStringBuilderBase() : base(DiagnosticId) { }

Expand All @@ -52,12 +52,12 @@ public abstract class UnusedStringBuilderBase<TSyntaxKind, TVariableDeclarator,
{
return;
}
var variableName = GetVariableName(variableDeclaration);
var variableSymbol = GetSymbol(variableDeclaration, c.SemanticModel);
var invocations = GetInvocations(variableDeclaration);
if (IsStringBuilderAccessed(variableName, invocations)
|| IsPassedToMethod(variableName, invocations)
|| IsReturned(variableName, GetReturnStatements(variableDeclaration))
|| IsWithinInterpolatedString(variableName, GetInterpolatedStrings(variableDeclaration)))
if (IsStringBuilderContentRead(invocations, variableSymbol, c.SemanticModel)
|| IsPassedToMethod(invocations, variableSymbol, c.SemanticModel)
|| IsReturned(GetReturnStatements(variableDeclaration), variableSymbol, c.SemanticModel)
|| IsWithinInterpolatedString(GetInterpolatedStrings(variableDeclaration), variableSymbol, c.SemanticModel))
{
return;
}
Expand Down
27 changes: 15 additions & 12 deletions analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase<SyntaxKind, Va
{
protected override ILanguageFacade<SyntaxKind> Language => VisualBasicFacade.Instance;

protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.Names.FirstOrDefault().ToString();
protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration.Names.First());

protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) =>
declaration.Initializer is not null
Expand All @@ -42,20 +42,23 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase<SyntaxKind, Va
protected override IList<InterpolationSyntax> GetInterpolatedStrings(VariableDeclaratorSyntax declaration) =>
declaration.Parent.Parent.Parent.DescendantNodes().OfType<InterpolationSyntax>().ToList();

protected override bool IsStringBuilderAccessed(string variableName, IList<InvocationExpressionSyntax> invocations) =>
invocations.Any(x => IsSameVariable(x.Expression, variableName) && StringBuilderAccessMethods.Contains(x.GetName()));
protected override bool IsStringBuilderContentRead(IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && StringBuilderAccessMethods.Contains(x.GetName()));

protected override bool IsPassedToMethod(string variableName, IList<InvocationExpressionSyntax> invocations) =>
invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName)));
protected override bool IsPassedToMethod(IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableSymbol, semanticModel)));

protected override bool IsReturned(string variableName, IList<ReturnStatementSyntax> returnStatements) =>
returnStatements.Any(x => IsSameVariable(x.Expression, variableName));
protected override bool IsReturned(IList<ReturnStatementSyntax> returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) =>
returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel));

protected override bool IsWithinInterpolatedString(string variableName, IList<InterpolationSyntax> interpolations) =>
interpolations.Any(x => IsSameVariable(x.Expression, variableName));
protected override bool IsWithinInterpolatedString(IList<InterpolationSyntax> interpolations, ISymbol variableSymbol, SemanticModel semanticModel) =>
interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel));

private static bool IsSameVariable(ExpressionSyntax expression, string variableName) =>
expression.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => p.NameIs(variableName))
private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) =>
expression.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => IsSameVariable(p, variableSymbol, semanticModel))
|| (expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().Any() && expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().First()
.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => p.NameIs(variableName)));
.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>().Any(p => IsSameVariable(p, variableSymbol, semanticModel)));

private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) =>
variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol);
}
3 changes: 0 additions & 3 deletions analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ public class UnusedStringBuilderTest
[DataRow("""@sb.Append("").ToString();""", true)]
[DataRow("sb.Remove(sb.Length - 1, 1);", true)]
[DataRow("""var a = $"{sb} is ToStringed here";""", true)]
[DataRow("var a = sb.Length;", false)] // FP
public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant)
{
var code = $$"""
Expand Down Expand Up @@ -89,7 +88,6 @@ public void MyMethod()
[DataRow("""@sb.Append("").ToString();""", true)]
[DataRow("sb.Remove(sb.Length - 1, 1);", true)]
[DataRow("""var a = $"{sb} is ToStringed here";""", true)]
[DataRow("var a = sb.Length;", false)] // FP
public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant)
{
var code = $$"""
Expand Down Expand Up @@ -122,7 +120,6 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl
[DataRow("""sb.Append("").ToString()""", true)]
[DataRow("sb.Remove(sb.Length - 1, 1)", true)]
[DataRow("""Dim a = $"{sb} is ToStringed here" """, true)]
[DataRow("Dim a = sb.Length", false)] // FP
[DataRow("""SB.ToString()""", true)]
public void UnusedStringBuilder_VBExpressionsTest(string expression, bool compliant)
{
Expand Down
12 changes: 12 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ public class Program

(StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN

StringBuilder builderInLine1 = new StringBuilder(), builderInLine2 = new StringBuilder(); // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1

StringBuilder builderCfg = new StringBuilder(); // FN (we should use cfg with significant impact on performance)
if (false)
{
Expand All @@ -55,6 +58,10 @@ public class Program
builderCalls.Clear();
builderCalls.GetType();

StringBuilder mySbField = new StringBuilder(); // Noncompliant
MyClass myClass = new MyClass();
myClass.mySbField.ToString();

var builderReturn = new StringBuilder(); // Compliant
return builderReturn;

Expand Down Expand Up @@ -87,3 +94,8 @@ public string MyProperty

private StringBuilder myField = new StringBuilder(); // Compliant
}

public class MyClass
{
public StringBuilder mySbField = new StringBuilder();
}
10 changes: 10 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ Public Class Program
Dim builder8 As StringBuilder = New StringBuilder() ' Compliant
builder8.Append("&").ToString()

Dim builderInLine1 As StringBuilder = New StringBuilder(), builderInLine2 As StringBuilder = New StringBuilder() ' Noncompliant
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ' Noncompliant@-1

Dim builderCfg As StringBuilder = New StringBuilder() ' FN (we should use cfg with significant impact on performance)
If False Then
builderCfg.ToString()
Expand All @@ -29,6 +32,10 @@ Public Class Program
Dim builderCalls2 As StringBuilder = New StringBuilder() ' Compliant
builderCalls2.Remove(builderCalls2.Length - 1, 1)

Dim mySbField As StringBuilder = New StringBuilder() ' Noncompliant
Dim [myClass] As [MyClass] = New [MyClass]()
[myClass].mySbField.ToString()

Dim builderReturn = New StringBuilder() ' Compliant
Return builderReturn
End Function
Expand All @@ -51,3 +58,6 @@ Public Class Program
Private myField As StringBuilder = New StringBuilder() ' Compliant
End Class

Public Class [MyClass]
Public mySbField As StringBuilder = New StringBuilder()
End Class

0 comments on commit 38a68cb

Please sign in to comment.