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

* Base

* Add test cases

* First implementation CS/VB

* Fix UTs

* Fixed code smells + added test cases

* More test cases

* Fixed CS test cases

* Fixed VB test cases

* Semplifications

* Add support for TopLevelStatements and StringBuilder access methods

* Add support for null-conditional operator

* Add support for interpolated strings and VB null conditional operators

* Avoid raising for field declarations

* Replaced name comparison with semantic model symbol checks

* Add support for indexer access

* Add syntax kind

* Fix UTs

* Rewritten IsSameVariable to improve performance

* Removed comments

* Improved perf on IsSameVariable (VB)

* Addressed PR comments

* Rule structure changes

* Increased performance checking var names before syntax level check

* Increased perf skipping not interesting SyntaxKinds

* Add coverage amd fix code smell

* ITs fix

* Address PR comments

* Raise for .Capacity and .MaxCapacity

* Small tweaks

* Removed support for Null-conditional operators ?.

* Addressed more PR comments

* TryCreate code clean up

* Added support for assignments

* Added FP

* Add link to the issue

* Fix code smell

* Address review comments

* More comments

* && to ||

* Remove GetIdentifier from the Facade (not used in the current rule implementation)

* Removed from the interface

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>
+

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"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -127,6 +127,7 @@
     "S3010",
     "S3011",
     "S3060",
+    "S3063",
     "S3168",
     "S3169",
     "S3172",

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>
+

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"
+}

analyzers/rspec/vbnet/Sonar_way_profile.json

@@ -72,6 +72,7 @@
     "S2761",
     "S2951",
     "S3011",
+    "S3063",
     "S3358",
     "S3385",
     "S3449",

analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs

@@ -19,7 +19,6 @@
  */
 
 using SonarAnalyzer.Helpers.Facade;
-using StyleCop.Analyzers.Lightup;
 
 namespace SonarAnalyzer.Helpers;
 
@@ -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();
 }

analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs

@@ -80,5 +80,6 @@ internal sealed class CSharpSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind>
         SyntaxKindEx.RecordClassDeclaration,
         SyntaxKindEx.RecordStructDeclaration,
     };
+    public SyntaxKind VariableDeclarator => SyntaxKind.VariableDeclarator;
     public SyntaxKind WhitespaceTrivia => SyntaxKind.WhitespaceTrivia;
 }

analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs

@@ -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
             };
 

analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/LooseFilePermissions.cs

@@ -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);
     }
 }

analyzers/src/SonarAnalyzer.CSharp/Rules/MemberInitializedToDefault.cs

@@ -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)
                         {
-                            var objectCreation = ObjectCreationFactory.Create(initializer.Value);
                             var argCount = objectCreation.ArgumentList?.Arguments.Count;
                             if (argCount == null || argCount == 0)
                             {

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,
+        SyntaxKind.StructDeclaration,
+        SyntaxKind.EnumDeclaration,
+        SyntaxKind.InterfaceDeclaration,
+        SyntaxKind.UsingDirective,
+        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,
+        };
+
+    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);
+}

analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs

@@ -44,6 +44,11 @@ node switch
                 _ => throw new InvalidOperationException("Unexpected type: " + node.GetType().Name)
             };
 
+        public static IObjectCreation TryCreate(SyntaxNode node) =>
+             node.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression)
+                 ? Create(node)
+                 : null;
+
         private class ObjectCreation : IObjectCreation
         {
             private readonly ObjectCreationExpressionSyntax objectCreation;

analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs

@@ -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

analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs

@@ -55,5 +55,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; }
 }

analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs

@@ -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);
+
+    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) =>
+        stringBuilderAccessExpressions.Any(x => x.Equals(expression, Language.NameComparison));
+}

analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs

@@ -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();
 }

analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs

@@ -70,5 +70,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;
 }