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

New rule S3063: "StringBuilder" data should be used #6696

Merged
merged 41 commits into from Feb 14, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
c99b62c
Base
CristianAmbrosini Jan 30, 2023
08f0ba3
Add test cases
CristianAmbrosini Feb 1, 2023
b7d0d30
First implementation CS/VB
CristianAmbrosini Feb 1, 2023
e487913
Fix UTs
CristianAmbrosini Feb 1, 2023
7ddcd31
Fixed code smells + added test cases
CristianAmbrosini Feb 1, 2023
1dee716
More test cases
CristianAmbrosini Feb 1, 2023
b4c1029
Fixed CS test cases
CristianAmbrosini Feb 1, 2023
97a2421
Fixed VB test cases
CristianAmbrosini Feb 1, 2023
d2554e6
Semplifications
CristianAmbrosini Feb 2, 2023
740a16e
Add support for TopLevelStatements and StringBuilder access methods
CristianAmbrosini Feb 3, 2023
85e22ee
Add support for null-conditional operator
CristianAmbrosini Feb 6, 2023
e6ce8bd
Add support for interpolated strings and VB null conditional operators
CristianAmbrosini Feb 6, 2023
59f1967
Avoid raising for field declarations
CristianAmbrosini Feb 6, 2023
f0003ba
Replaced name comparison with semantic model symbol checks
CristianAmbrosini Feb 7, 2023
c062371
Add support for indexer access
CristianAmbrosini Feb 7, 2023
f44dc07
Add syntax kind
CristianAmbrosini Feb 7, 2023
c207c32
Fix UTs
CristianAmbrosini Feb 7, 2023
226bfdf
Rewritten IsSameVariable to improve performance
CristianAmbrosini Feb 7, 2023
73ca43b
Removed comments
CristianAmbrosini Feb 7, 2023
344e472
Improved perf on IsSameVariable (VB)
CristianAmbrosini Feb 7, 2023
0830d34
Addressed PR comments
CristianAmbrosini Feb 8, 2023
77a23a1
Rule structure changes
CristianAmbrosini Feb 8, 2023
7da66e4
Increased performance checking var names before syntax level check
CristianAmbrosini Feb 8, 2023
7c96800
Increased perf skipping not interesting SyntaxKinds
CristianAmbrosini Feb 8, 2023
a26ea27
Add coverage amd fix code smell
CristianAmbrosini Feb 9, 2023
df5dc9c
ITs fix
CristianAmbrosini Feb 9, 2023
ba38024
Address PR comments
CristianAmbrosini Feb 9, 2023
76383c1
Raise for .Capacity and .MaxCapacity
CristianAmbrosini Feb 9, 2023
2ac2907
Small tweaks
CristianAmbrosini Feb 9, 2023
716a82d
Removed support for Null-conditional operators ?.
CristianAmbrosini Feb 9, 2023
408e5fa
Addressed more PR comments
CristianAmbrosini Feb 10, 2023
5ba0b53
TryCreate code clean up
CristianAmbrosini Feb 10, 2023
232c216
Added support for assignments
CristianAmbrosini Feb 10, 2023
add749b
Added FP
CristianAmbrosini Feb 10, 2023
424f743
Add link to the issue
CristianAmbrosini Feb 10, 2023
95e9522
Fix code smell
CristianAmbrosini Feb 10, 2023
a6b6c98
Address review comments
CristianAmbrosini Feb 13, 2023
498e6f0
More comments
CristianAmbrosini Feb 13, 2023
0d2733e
&& to ||
CristianAmbrosini Feb 13, 2023
aaa0754
Remove GetIdentifier from the Facade (not used in the current rule im…
CristianAmbrosini Feb 14, 2023
d60927c
Removed from the interface
CristianAmbrosini Feb 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 37 additions & 0 deletions analyzers/rspec/cs/S3063_c#.html
@@ -0,0 +1,37 @@
<p><code>StringBuilder</code> instances that are <code>append</code>ed but never <code>toString</code>ed needlessly clutter the code, and worse are a
drag on performance. Either they should be removed, or the missing <code>toString</code> call added.</p>
<h2>Noncompliant Code Example</h2>
<pre>
public void doSomething(List&lt;string&gt; strings) {
StringBuilder sb = new StringBuilder(); // Noncompliant
sb.Append("Got: ");
foreach(string str in strings) {
sb.Append(str).Append(", ");
// ...
}
}
</pre>
<h2>Compliant Solution</h2>
<pre>
public void doSomething(List&lt;string&gt; strings) {
foreach(string str in strings) {
// ...
}
}
</pre>
<p>or</p>
<pre>
public void doSomething(List&lt;string&gt; strings) {
StringBuilder sb = new StringBuilder(); // Noncompliant
sb.Append("Got: ");
foreach(string str in strings) {
sb.Append(str).Append(", ");
// ...
}
_logger.LogInformation(sb.toString, DateTimeOffset.UtcNow);
}
</pre>
<h2>Exceptions</h2>
<p>This rule ignores <code>StringBuilder</code>s that are passed as method arguments on the grounds that they are likely <code>toString</code>ed
there.</p>

17 changes: 17 additions & 0 deletions analyzers/rspec/cs/S3063_c#.json
@@ -0,0 +1,17 @@
{
"title": "\"StringBuilder\" data should be used",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"performance"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-3063",
"sqKey": "S3063",
"scope": "Main",
"quickfix": "unknown"
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Expand Up @@ -125,6 +125,7 @@
"S3010",
"S3011",
"S3060",
"S3063",
"S3168",
"S3169",
"S3172",
Expand Down
37 changes: 37 additions & 0 deletions analyzers/rspec/vbnet/S3063_vb.net.html
@@ -0,0 +1,37 @@
<p><code>StringBuilder</code> instances that are <code>append</code>ed but never <code>toString</code>ed needlessly clutter the code, and worse are a
drag on performance. Either they should be removed, or the missing <code>toString</code> call added.</p>
<h2>Noncompliant Code Example</h2>
<pre>
Public Sub doSomething(ByVal strings As List(Of String))
Dim sb As StringBuilder = New StringBuilder()
sb.Append("Got: ")

For Each str As String In strings
sb.Append(str).Append(", ")
Next
End Sub
</pre>
<h2>Compliant Solution</h2>
<pre>
Public Sub doSomething(ByVal strings As List(Of String))
For Each str As String In strings
Next
End Sub
</pre>
<p>or</p>
<pre>
Public Sub doSomething(ByVal strings As List(Of String))
Dim sb As StringBuilder = New StringBuilder()
sb.Append("Got: ")

For Each str As String In strings
sb.Append(str).Append(", ")
Next

My.Application.Log.WriteEntry(sb.toString)
End Sub
</pre>
<h2>Exceptions</h2>
<p>This rule ignores <code>StringBuilder</code>s that are passed as method arguments on the grounds that they are likely <code>toString</code>ed
there.</p>

17 changes: 17 additions & 0 deletions analyzers/rspec/vbnet/S3063_vb.net.json
@@ -0,0 +1,17 @@
{
"title": "\"StringBuilder\" data should be used",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"performance"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-3063",
"sqKey": "S3063",
"scope": "Main",
"quickfix": "unknown"
}
1 change: 1 addition & 0 deletions analyzers/rspec/vbnet/Sonar_way_profile.json
Expand Up @@ -71,6 +71,7 @@
"S2761",
"S2951",
"S3011",
"S3063",
"S3358",
"S3385",
"S3449",
Expand Down
@@ -0,0 +1,27 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Extensions;

internal static class VariableDeclaratorSyntaxExtensions
{
public static bool IsTopLevel(this VariableDeclaratorSyntax declarator) =>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
declarator.Parent.Parent is { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax } };
}
Expand Up @@ -79,5 +79,6 @@ internal sealed class CSharpSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind>
SyntaxKindEx.RecordClassDeclaration,
SyntaxKindEx.RecordStructDeclaration,
};
public SyntaxKind VariableDeclarator => SyntaxKind.VariableDeclarator;
public SyntaxKind WhitespaceTrivia => SyntaxKind.WhitespaceTrivia;
}
100 changes: 100 additions & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs
@@ -0,0 +1,100 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class UnusedStringBuilder : UnusedStringBuilderBase<SyntaxKind, VariableDeclaratorSyntax, InvocationExpressionSyntax, ReturnStatementSyntax, InterpolationSyntax>
{
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

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

protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) =>
declaration.Initializer is not null
&& declaration.Parent.Parent is LocalDeclarationStatementSyntax
&& declaration.Initializer.Value is { } expression
&& IsStringBuilderObjectCreation(expression, semanticModel);
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

protected override IList<InvocationExpressionSyntax> GetInvocations(VariableDeclaratorSyntax declaration) =>
declaration.IsTopLevel()
? GetTopLevelStatementSyntax<InvocationExpressionSyntax>(declaration)
: declaration.Parent.Parent.Parent.DescendantNodes().OfType<InvocationExpressionSyntax>().ToList();

protected override IList<ReturnStatementSyntax> GetReturnStatements(VariableDeclaratorSyntax declaration) =>
declaration.IsTopLevel()
? new()
: declaration.Parent.Parent.Parent.DescendantNodes().OfType<ReturnStatementSyntax>().ToList();

protected override IList<InterpolationSyntax> GetInterpolatedStrings(VariableDeclaratorSyntax declaration) =>
declaration.IsTopLevel()
? GetTopLevelStatementSyntax<InterpolationSyntax>(declaration)
: declaration.Parent.Parent.Parent.DescendantNodes().OfType<InterpolationSyntax>().ToList();

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(IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableSymbol, semanticModel)));

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

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

protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList<InvocationExpressionSyntax> invocations, ISymbol variableSymbol, SemanticModel semanticModel) =>
GetElementAccessExpressions(declaration).Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel));

private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) =>
(expression is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression))
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
&& ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel);

private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel)
{
var identifiers = expression.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>();
if (!identifiers.Any() && expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().Any())
{
identifiers = expression.Ancestors().OfType<ConditionalAccessExpressionSyntax>().First().DescendantNodesAndSelf().OfType<IdentifierNameSyntax>();
}
return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel));
}

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

private static IList<ElementAccessExpressionSyntax> GetElementAccessExpressions(VariableDeclaratorSyntax declaration) =>
declaration.IsTopLevel()
? GetTopLevelStatementSyntax<ElementAccessExpressionSyntax>(declaration)
: declaration.Parent.Parent.Parent.DescendantNodes().OfType<ElementAccessExpressionSyntax>().ToList();

private static IList<T> GetTopLevelStatementSyntax<T>(VariableDeclaratorSyntax declaration)
{
List<T> list = new();
foreach (var globalStatement in declaration.Parent.Parent.Parent.Parent.DescendantNodes().OfType<GlobalStatementSyntax>())
{
foreach (var interpolation in globalStatement.DescendantNodes().OfType<T>())
{
list.Add(interpolation);
}
}
return list;
}
}
Expand Up @@ -54,5 +54,6 @@ public interface ISyntaxKindFacade<out TSyntaxKind>
abstract TSyntaxKind[] StringLiteralExpressions { get; }
abstract TSyntaxKind StructDeclaration { get; }
abstract TSyntaxKind[] TypeDeclaration { get; }
abstract TSyntaxKind VariableDeclarator { get; }
abstract TSyntaxKind WhitespaceTrivia { get; }
}
@@ -0,0 +1,68 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules;

public abstract class UnusedStringBuilderBase<TSyntaxKind, TVariableDeclarator, TInvocationExpression, TReturnStatement, TInterpolatedString> : SonarDiagnosticAnalyzer<TSyntaxKind>
where TSyntaxKind : struct
where TVariableDeclarator : SyntaxNode
where TInvocationExpression : SyntaxNode
where TReturnStatement : SyntaxNode
where TInterpolatedString : SyntaxNode
{
private const string DiagnosticId = "S3063";
protected override string MessageFormat => """Remove this "StringBuilder"; ".ToString()" is never called.""";

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

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 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 abstract bool IsPropertyReferenced(TVariableDeclarator declaration, IList<TInvocationExpression> invocations, ISymbol variableSymbol, SemanticModel semanticModel);

protected UnusedStringBuilderBase() : base(DiagnosticId) { }

protected sealed override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c =>
{
var variableDeclaration = (TVariableDeclarator)c.Node;
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
if (!NeedsToTrack(variableDeclaration, c.SemanticModel))
{
return;
}
var variableSymbol = GetSymbol(variableDeclaration, c.SemanticModel);
var invocations = GetInvocations(variableDeclaration);
if (IsStringBuilderContentRead(invocations, variableSymbol, c.SemanticModel)
|| IsPassedToMethod(invocations, variableSymbol, c.SemanticModel)
|| IsReturned(GetReturnStatements(variableDeclaration), variableSymbol, c.SemanticModel)
|| IsWithinInterpolatedString(GetInterpolatedStrings(variableDeclaration), variableSymbol, c.SemanticModel)
|| IsPropertyReferenced(variableDeclaration, invocations, variableSymbol, c.SemanticModel))
{
return;
}
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
c.ReportIssue(Diagnostic.Create(Rule, variableDeclaration.GetLocation()));
}, Language.SyntaxKind.VariableDeclarator);
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Up @@ -65,5 +65,6 @@ internal sealed class VisualBasicSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind
public SyntaxKind[] StringLiteralExpressions => new[] { SyntaxKind.StringLiteralExpression };
public SyntaxKind StructDeclaration => SyntaxKind.StructureBlock;
public SyntaxKind[] TypeDeclaration => new[] { SyntaxKind.ClassBlock, SyntaxKind.StructureBlock, SyntaxKind.InterfaceBlock, SyntaxKind.EnumBlock };
public SyntaxKind VariableDeclarator => SyntaxKind.VariableDeclarator;
public SyntaxKind WhitespaceTrivia => SyntaxKind.WhitespaceTrivia;
}