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 all 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
4 changes: 3 additions & 1 deletion analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs
Expand Up @@ -19,7 +19,6 @@
*/

using SonarAnalyzer.Helpers.Facade;
using StyleCop.Analyzers.Lightup;

namespace SonarAnalyzer.Helpers;

Expand Down Expand Up @@ -61,4 +60,7 @@ invocation switch
new CSharpMethodParameterLookup(((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList, methodSymbol),
_ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)),
};

public string GetName(SyntaxNode expression) =>
expression.GetName();
}
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;
}
Expand Up @@ -40,8 +40,7 @@ public sealed class CertificateValidationCheck : CertificateValidationCheckBase<
argumentListNode switch
{
InvocationExpressionSyntax invocation => new CSharpMethodParameterLookup(invocation.ArgumentList, method),
{ } when argumentListNode.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) =>
new CSharpMethodParameterLookup(ObjectCreationFactory.Create(argumentListNode).ArgumentList, method),
_ when ObjectCreationFactory.TryCreate(argumentListNode) is { ArgumentList: { } argumentList } => new CSharpMethodParameterLookup(argumentList, method),
_ => throw new ArgumentException("Unexpected type.", nameof(argumentListNode)) // This should be throw only by bad usage of this method, not by input dependency
};

Expand Down
Expand Up @@ -103,6 +103,6 @@ private static IObjectCreation GetObjectCreation(InvocationExpressionSyntax invo

private static IEnumerable<IObjectCreation> FilterObjectCreations(IEnumerable<SyntaxNode> nodes) =>
nodes.Where(x => x.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression))
.Select(x => ObjectCreationFactory.Create(x));
.Select(ObjectCreationFactory.Create);
}
}
Expand Up @@ -139,9 +139,8 @@ private static bool IsValueTypeDefaultValueInitializer(EqualsValueClauseSyntax i
{
return true;
}
else if (initializer.Value is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(initializer.Value))
else if (ObjectCreationFactory.TryCreate(initializer.Value) is { } objectCreation)
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
var objectCreation = ObjectCreationFactory.Create(initializer.Value);
var argCount = objectCreation.ArgumentList?.Arguments.Count;
if (argCount == null || argCount == 0)
{
Expand Down
77 changes: 77 additions & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs
@@ -0,0 +1,77 @@
/*
* 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, IdentifierNameSyntax>
{
private static readonly SyntaxKind[] SkipChildren =
{
SyntaxKind.ClassDeclaration,
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
SyntaxKind.StructDeclaration,
SyntaxKind.EnumDeclaration,
SyntaxKind.InterfaceDeclaration,
SyntaxKind.UsingDirective,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of top-level statements we are not going through the children of the using directives (see here).

  • The test for top-level statements is called UnusedStringBuilder_TopLevelStatements and it's a parameterized test.

SyntaxKindEx.RecordClassDeclaration,
SyntaxKindEx.RecordStructDeclaration
};

protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) =>
declarator switch
{
{ Parent: VariableDeclarationSyntax { Parent: LocalDeclarationStatementSyntax { Parent: BlockSyntax block } } } => block,
{ Parent: VariableDeclarationSyntax { Parent: LocalDeclarationStatementSyntax { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax compilationUnit } } } } => compilationUnit,
_ => null,
};

protected override ILocalSymbol RetrieveStringBuilderObject(SemanticModel semanticModel, VariableDeclaratorSyntax declarator) =>
declarator is
{
Parent.Parent: LocalDeclarationStatementSyntax,
Initializer.Value: { } expression,
}
&& IsStringBuilderObjectCreation(expression, semanticModel)
? semanticModel.GetDeclaredSymbol(declarator) as ILocalSymbol
: null;

protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol symbol, SyntaxNode node) =>
node switch
{
InvocationExpressionSyntax invocation => IsAccessInvocation(invocation.Expression.GetName()) && IsSameReference(model, symbol, invocation.Expression),
ReturnStatementSyntax returnStatement => IsSameReference(model, symbol, returnStatement.Expression),
InterpolationSyntax interpolation => IsSameReference(model, symbol, interpolation.Expression),
ElementAccessExpressionSyntax elementAccess => IsSameReference(model, symbol, elementAccess.Expression),
ArgumentSyntax argument => IsSameReference(model, symbol, argument.Expression),
MemberAccessExpressionSyntax memberAccess => IsAccessExpression(memberAccess.Name.GetName()) && IsSameReference(model, symbol, memberAccess.Expression),
VariableDeclaratorSyntax { Initializer.Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier),
AssignmentExpressionSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier),
_ => false,
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
};

protected override bool DescendIntoChildren(SyntaxNode node) =>
!(node.IsAnyKind(SkipChildren) || (node.IsKind(SyntaxKindEx.LocalFunctionStatement) && ((LocalFunctionStatementSyntaxWrapper)node).Modifiers.Any(SyntaxKind.StaticKeyword)));

private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) =>
ObjectCreationFactory.TryCreate(expression) is { } creation
&& creation.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel);
}
Expand Up @@ -44,6 +44,11 @@ node switch
_ => throw new InvalidOperationException("Unexpected type: " + node.GetType().Name)
};

public static IObjectCreation TryCreate(SyntaxNode node) =>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
node.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression)
? Create(node)
: null;

private class ObjectCreation : IObjectCreation
{
private readonly ObjectCreationExpressionSyntax objectCreation;
Expand Down
Expand Up @@ -33,6 +33,7 @@ public interface ILanguageFacade
DiagnosticDescriptor CreateDescriptor(string id, string messageFormat, bool? isEnabledByDefault = null, bool fadeOutCode = false);
object FindConstantValue(SemanticModel model, SyntaxNode node);
IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol);
string GetName(SyntaxNode expression);
}

public interface ILanguageFacade<TSyntaxKind> : ILanguageFacade
Expand Down
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,72 @@
/*
* 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, TIdentifierName> : SonarDiagnosticAnalyzer<TSyntaxKind>
where TSyntaxKind : struct
where TVariableDeclarator : SyntaxNode
where TIdentifierName : SyntaxNode
{
private const string DiagnosticId = "S3063";

private readonly string[] stringBuilderAccessInvocations = { "ToString", "CopyTo", "GetChunks" };
private readonly string[] stringBuilderAccessExpressions = { "Length" };

protected override string MessageFormat => """Remove this "StringBuilder"; ".ToString()" is never called.""";

protected abstract SyntaxNode GetScope(TVariableDeclarator declarator);
protected abstract ILocalSymbol RetrieveStringBuilderObject(SemanticModel semanticModel, TVariableDeclarator declaration);
protected abstract bool IsStringBuilderRead(SemanticModel model, ILocalSymbol symbol, SyntaxNode node);
protected abstract bool DescendIntoChildren(SyntaxNode node);

protected UnusedStringBuilderBase() : base(DiagnosticId) { }

protected sealed override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c =>
{
var variableDeclarator = (TVariableDeclarator)c.Node;

if (RetrieveStringBuilderObject(c.SemanticModel, variableDeclarator) is { } symbol
&& GetScope(variableDeclarator) is { } scope
&& !IsStringBuilderAccessed(c.SemanticModel, symbol, scope))
{
c.ReportIssue(Diagnostic.Create(Rule, variableDeclarator.GetLocation()));
}
}, Language.SyntaxKind.VariableDeclarator);
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

internal bool IsSameReference(SemanticModel semanticModel, ILocalSymbol symbol, SyntaxNode expression) =>
expression is not null && GetLocalReferences(expression).Any(x => IsSameVariable(semanticModel, symbol, x));

private bool IsSameVariable(SemanticModel semanticModel, ILocalSymbol symbol, SyntaxNode identifier) =>
Language.GetName(identifier).Equals(symbol.Name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol);

private static IEnumerable<TIdentifierName> GetLocalReferences(SyntaxNode node) =>
node.DescendantNodesAndSelf().OfType<TIdentifierName>();

private bool IsStringBuilderAccessed(SemanticModel model, ILocalSymbol symbol, SyntaxNode scope) =>
scope.DescendantNodes(DescendIntoChildren).Any(node => IsStringBuilderRead(model, symbol, node));

internal bool IsAccessInvocation(string invocation) =>
stringBuilderAccessInvocations.Any(x => x.Equals(invocation, Language.NameComparison));

internal bool IsAccessExpression(string expression) =>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
stringBuilderAccessExpressions.Any(x => x.Equals(expression, Language.NameComparison));
}
Expand Up @@ -58,4 +58,7 @@ invocation switch
InvocationExpressionSyntax x => new VisualBasicMethodParameterLookup(x, methodSymbol),
_ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)),
};

public string GetName(SyntaxNode expression) =>
expression.GetName();
}
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;
}