diff --git a/analyzers/rspec/cs/S3063_c#.html b/analyzers/rspec/cs/S3063_c#.html new file mode 100644 index 00000000000..e4076481597 --- /dev/null +++ b/analyzers/rspec/cs/S3063_c#.html @@ -0,0 +1,37 @@ +

StringBuilder instances that are appended but never toStringed needlessly clutter the code, and worse are a +drag on performance. Either they should be removed, or the missing toString call added.

+

Noncompliant Code Example

+
+public void doSomething(List<string> strings) {
+  StringBuilder sb = new StringBuilder();  // Noncompliant
+  sb.Append("Got: ");
+  foreach(string str in strings) {
+    sb.Append(str).Append(", ");
+    // ...
+  }
+}
+
+

Compliant Solution

+
+public void doSomething(List<string> strings) {
+  foreach(string str in strings) {
+    // ...
+  }
+}
+
+

or

+
+public void doSomething(List<string> strings) {
+  StringBuilder sb = new StringBuilder();  // Noncompliant
+  sb.Append("Got: ");
+  foreach(string str in strings) {
+    sb.Append(str).Append(", ");
+    // ...
+  }
+  _logger.LogInformation(sb.toString, DateTimeOffset.UtcNow);
+}
+
+

Exceptions

+

This rule ignores StringBuilders that are passed as method arguments on the grounds that they are likely toStringed +there.

+ diff --git a/analyzers/rspec/cs/S3063_c#.json b/analyzers/rspec/cs/S3063_c#.json new file mode 100644 index 00000000000..0827c147f3d --- /dev/null +++ b/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" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index c91e8e43fef..2b7c15c5eb5 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -127,6 +127,7 @@ "S3010", "S3011", "S3060", + "S3063", "S3168", "S3169", "S3172", diff --git a/analyzers/rspec/vbnet/S3063_vb.net.html b/analyzers/rspec/vbnet/S3063_vb.net.html new file mode 100644 index 00000000000..3d204c44c43 --- /dev/null +++ b/analyzers/rspec/vbnet/S3063_vb.net.html @@ -0,0 +1,37 @@ +

StringBuilder instances that are appended but never toStringed needlessly clutter the code, and worse are a +drag on performance. Either they should be removed, or the missing toString call added.

+

Noncompliant Code Example

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

Compliant Solution

+
+Public Sub doSomething(ByVal strings As List(Of String))
+    For Each str As String In strings
+    Next
+End Sub
+
+

or

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

Exceptions

+

This rule ignores StringBuilders that are passed as method arguments on the grounds that they are likely toStringed +there.

+ diff --git a/analyzers/rspec/vbnet/S3063_vb.net.json b/analyzers/rspec/vbnet/S3063_vb.net.json new file mode 100644 index 00000000000..0827c147f3d --- /dev/null +++ b/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" +} diff --git a/analyzers/rspec/vbnet/Sonar_way_profile.json b/analyzers/rspec/vbnet/Sonar_way_profile.json index e6a352709de..d6da56534fc 100644 --- a/analyzers/rspec/vbnet/Sonar_way_profile.json +++ b/analyzers/rspec/vbnet/Sonar_way_profile.json @@ -72,6 +72,7 @@ "S2761", "S2951", "S3011", + "S3063", "S3358", "S3385", "S3449", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs index f05e097d57a..c0ecf611a64 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs +++ b/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(); } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs index 4262139bf3f..3a60ed1408e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs @@ -80,5 +80,6 @@ internal sealed class CSharpSyntaxKindFacade : ISyntaxKindFacade SyntaxKindEx.RecordClassDeclaration, SyntaxKindEx.RecordStructDeclaration, }; + public SyntaxKind VariableDeclarator => SyntaxKind.VariableDeclarator; public SyntaxKind WhitespaceTrivia => SyntaxKind.WhitespaceTrivia; } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs index 4e95d59b98a..2a39598e0eb 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs +++ b/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 }; diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/LooseFilePermissions.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/LooseFilePermissions.cs index 02465e52e5b..a285ecd291f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/LooseFilePermissions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/LooseFilePermissions.cs @@ -103,6 +103,6 @@ private static IObjectCreation GetObjectCreation(InvocationExpressionSyntax invo private static IEnumerable FilterObjectCreations(IEnumerable nodes) => nodes.Where(x => x.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression)) - .Select(x => ObjectCreationFactory.Create(x)); + .Select(ObjectCreationFactory.Create); } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MemberInitializedToDefault.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MemberInitializedToDefault.cs index 555d3b1a385..dd40b6f9a2b 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MemberInitializedToDefault.cs +++ b/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) { diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs new file mode 100644 index 00000000000..d550b991293 --- /dev/null +++ b/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 +{ + private static readonly SyntaxKind[] SkipChildren = + { + SyntaxKind.ClassDeclaration, + SyntaxKind.StructDeclaration, + SyntaxKind.EnumDeclaration, + SyntaxKind.InterfaceDeclaration, + SyntaxKind.UsingDirective, + SyntaxKindEx.RecordClassDeclaration, + SyntaxKindEx.RecordStructDeclaration + }; + + protected override ILanguageFacade 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); +} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs b/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs index fd2fdbfc7c9..1f9aa8edff8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs +++ b/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; diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs index 8fd28a001be..4c675baa6d5 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs +++ b/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 : ILanguageFacade diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs index 807708293b4..d2828707e29 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs @@ -55,5 +55,6 @@ public interface ISyntaxKindFacade abstract TSyntaxKind[] StringLiteralExpressions { get; } abstract TSyntaxKind StructDeclaration { get; } abstract TSyntaxKind[] TypeDeclaration { get; } + abstract TSyntaxKind VariableDeclarator { get; } abstract TSyntaxKind WhitespaceTrivia { get; } } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs new file mode 100644 index 00000000000..8245a808cdf --- /dev/null +++ b/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 : SonarDiagnosticAnalyzer + 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 GetLocalReferences(SyntaxNode node) => + node.DescendantNodesAndSelf().OfType(); + + 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)); +} diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs index f559f857757..83d6afc1fab 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs +++ b/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(); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs index 2a388144ee2..42983cd7a8f 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs @@ -70,5 +70,6 @@ internal sealed class VisualBasicSyntaxKindFacade : ISyntaxKindFacade 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; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs new file mode 100644 index 00000000000..7da4aef250c --- /dev/null +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -0,0 +1,63 @@ +/* + * 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.VisualBasic; + +[DiagnosticAnalyzer(LanguageNames.VisualBasic)] +public sealed class UnusedStringBuilder : UnusedStringBuilderBase +{ + private static readonly SyntaxKind[] SkipChildren = { }; + + protected override ILanguageFacade Language => VisualBasicFacade.Instance; + + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => + declarator is { Parent: LocalDeclarationStatementSyntax { Parent: { } block } } + ? block + : null; + + protected override ILocalSymbol RetrieveStringBuilderObject(SemanticModel semanticModel, VariableDeclaratorSyntax declarator) => + declarator is + { + Parent: LocalDeclarationStatementSyntax, + Initializer.Value: ObjectCreationExpressionSyntax { } objectCreation, + Names: { Count: 1 } names, // Must be 1, because otherwise "BC30671: Explicit initialization is not permitted with multiple variables declared with a single type specifier." is raised. + } + && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel) + ? semanticModel.GetDeclaredSymbol(names[0]) as ILocalSymbol + : null; + + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol symbol, SyntaxNode node) => + node switch + { + InvocationExpressionSyntax invocation => + (IsAccessInvocation(invocation.GetName()) && IsSameReference(model, symbol, invocation.Expression)) + || (IsSameReference(model, symbol, invocation.Expression) && model.GetSymbolInfo(invocation).Symbol is IPropertySymbol { IsIndexer: true }), + ReturnStatementSyntax returnStatement => IsSameReference(model, symbol, returnStatement.Expression), + InterpolationSyntax interpolation => IsSameReference(model, symbol, interpolation.Expression), + ArgumentSyntax argument => IsSameReference(model, symbol, argument.GetExpression()), + MemberAccessExpressionSyntax memberAccess => IsAccessExpression(memberAccess.Name.GetName()) && IsSameReference(model, symbol, memberAccess.Expression), + VariableDeclaratorSyntax { Initializer.Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + AssignmentStatementSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + _ => false, + }; + + protected override bool DescendIntoChildren(SyntaxNode node) => + !node.IsAnyKind(SkipChildren); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs index 7477a3e62e6..4dc49032b0f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs @@ -2987,7 +2987,7 @@ internal static class RuleTypeMappingCS ["S3060"] = "CODE_SMELL", // ["S3061"], // ["S3062"], - // ["S3063"], + ["S3063"] = "CODE_SMELL", // ["S3064"], // ["S3065"], // ["S3066"], diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingVB.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingVB.cs index 25791bb1cfd..c4f42cdce08 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingVB.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingVB.cs @@ -2987,7 +2987,7 @@ internal static class RuleTypeMappingVB // ["S3060"], // ["S3061"], // ["S3062"], - // ["S3063"], + ["S3063"] = "CODE_SMELL", // ["S3064"], // ["S3065"], // ["S3066"], diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs new file mode 100644 index 00000000000..cb2a69612c8 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -0,0 +1,179 @@ +/* + * 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. + */ + +using CS = SonarAnalyzer.Rules.CSharp; +using VB = SonarAnalyzer.Rules.VisualBasic; + +namespace SonarAnalyzer.UnitTest.Rules; + +[TestClass] +public class UnusedStringBuilderTest +{ + private readonly VerifierBuilder builderCS = new VerifierBuilder(); + private readonly VerifierBuilder builderVB = new VerifierBuilder(); + + [TestMethod] + public void UnusedStringBuilder_CS() => + builderCS.AddPaths("UnusedStringBuilder.cs").Verify(); + + [TestMethod] + public void UnusedStringBuilder_VB() => + builderVB.AddPaths("UnusedStringBuilder.vb").Verify(); + +#if NET + + [TestMethod] + public void UnusedStringBuilder_CSharp9() => + builderCS.AddPaths("UnusedStringBuilder.CSharp9.cs") + .WithOptions(ParseOptionsHelper.FromCSharp9) + .Verify(); + + [DataTestMethod] + [DataRow("", false)] + [DataRow("sb.ToString();", true)] + [DataRow("""var a = sb.Append("").Append("").Append("").Append("").ToString().ToLower();""", true)] + [DataRow("sb.CopyTo(0, new char[1], 0, 1);", true)] + [DataRow("sb.GetChunks();", true)] + [DataRow("var a = sb[0];", true)] + [DataRow("""sb?.Append("").ToString().ToLower();""", false)] // FP see https://github.com/SonarSource/sonar-dotnet/issues/6743 + [DataRow("sb?.ToString().ToLower();", false)] // FP + [DataRow("""@sb.Append("").ToString();""", true)] + [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("var a = sb.Length;", true)] + [DataRow("var a = sb.Capacity;", false)] + [DataRow("var a = sb.MaxCapacity;", false)] + [DataRow("""var a = $"{sb} is ToStringed here";""", true)] + [DataRow("var a = sb;", true)] + public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) + { + var code = $$""" + using System.Text; + + var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} + {{expression}} + """; + var builder = builderCS.AddSnippet(code).WithTopLevelStatements(); + if (compliant) + { + builder.VerifyNoIssueReported(); + } + else + { + builder.Verify(); + } + } + +#endif + + [DataTestMethod] + [DataRow("", false)] + [DataRow("sb.ToString();", true)] + [DataRow("""var a = sb.Append("").Append("").Append("").Append("").ToString().ToLower();""", true)] + [DataRow("sb.CopyTo(0, new char[1], 0, 1);", true)] + [DataRow("var a = sb[0];", true)] + [DataRow("""sb?.Append("").ToString().ToLower();""", false)] // FP see https://github.com/SonarSource/sonar-dotnet/issues/6743 + [DataRow("sb?.ToString();", false)] // FP + [DataRow("""@sb.Append("").ToString();""", true)] + [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("var a = sb.Length;", true)] + [DataRow("var a = sb.Capacity;", false)] + [DataRow("var a = sb.MaxCapacity;", false)] + [DataRow("""var a = $"{sb} is ToStringed here";""", true)] + [DataRow("var a = sb;", true)] + +#if NET + + [DataRow("sb.GetChunks();", true)] + +#endif + + public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) + { + var code = $$""" + using System.Text; + + public class MyClass + { + public void MyMethod() + { + var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} + {{expression}} + } + } + """; + var builder = builderCS.AddSnippet(code); + if (compliant) + { + builder.VerifyNoIssueReported(); + } + else + { + builder.Verify(); + } + } + + [DataTestMethod] + [DataRow("", false)] + [DataRow("sb.ToString()", true)] + [DataRow("""Dim a = sb.Append("").Append("").Append("").Append("").ToString().ToLower()""", true)] + [DataRow("sb.CopyTo(0, New Char(0) {}, 0, 1)", true)] + [DataRow("Dim a = sb(0)", true)] + [DataRow("""sb?.Append("").ToString().ToLower()""", false)] // FP see https://github.com/SonarSource/sonar-dotnet/issues/6743 + [DataRow("sb?.ToString().ToLower()", false)] // FP + [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", true)] + [DataRow("Dim a = sb.Capacity", false)] + [DataRow("Dim a = sb.MaxCapacity", false)] + [DataRow("Dim a = SB.ToString()", true)] + [DataRow("Dim a = sb.TOSTRING()", true)] + [DataRow("Dim a = sb.LENGTH", true)] + [DataRow("Dim a = sb", true)] + +#if NET + + [DataRow("sb.GetChunks()", true)] + +#endif + + public void UnusedStringBuilder_VBExpressionsTest(string expression, bool compliant) + { + var code = $$""" + Imports System.Text + + Public Class [MyClass] + Public Sub MyMethod() + Dim sb = New StringBuilder() ' {{(compliant ? "Compliant" : "Noncompliant")}} + {{expression}} + End Sub + End Class + """; + var builder = builderVB.AddSnippet(code); + if (compliant) + { + builder.VerifyNoIssueReported(); + } + else + { + builder.Verify(); + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs new file mode 100644 index 00000000000..981c0ea68db --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -0,0 +1,26 @@ +using System.Text; + +public class MyClass +{ + public void MyMethod(StringBuilder builder) // Compliant + { + StringBuilder builder1 = new(); // Compliant + + StringBuilder builder2 = new(); // Noncompliant +// ^^^^^^^^^^^^^^^^ + + StringBuilder builder3 = new(); // Noncompliant + builder3.Append(builder1.ToString()); + + (StringBuilder, StringBuilder) builder4 = (new(), new()); // FN + + StringBuilder builderInLine1 = new(), builderInLine2 = new(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 + + static void LocalStaticMethod() + { + StringBuilder builder2 = new StringBuilder(); // Compliant + var a = builder2.Length; + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs new file mode 100644 index 00000000000..14c4cd04369 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -0,0 +1,123 @@ +using System.Text; + +public class Program +{ + public StringBuilder MyMethod(StringBuilder builder, string myString) // Compliant + { + StringBuilder sb1 = GetStringBuilder(); // Compliant + + var sb2 = new StringBuilder(); // Compliant + ExternalMethod(sb2); + + var sb3 = new StringBuilder(); // Compliant + LocalMethod(sb3); + + var sb4 = new StringBuilder(); // Compliant + + StringBuilder sb5 = new StringBuilder(); // Noncompliant {{Remove this "StringBuilder"; ".ToString()" is never called.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + var sb6 = new StringBuilder(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + + StringBuilder sb7 = new StringBuilder(); // Noncompliant + sb7.Append(sb4.ToString()); + + StringBuilder sb8 = new StringBuilder(); // Compliant + StringBuilder sb9 = new StringBuilder(); // Noncompliant + sb9 = sb8; + + StringBuilder sb10 = new StringBuilder(); // Noncompliant FP see https://github.com/SonarSource/sonar-dotnet/issues/6747 + sb10.ToStringAndFree(); + + if (true) + { + if (true) + { + if (true) + { + if (true) + { + if (true) + { + var buildernested = new StringBuilder(); // Noncompliant + } + } + } + } + } + + (StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN + + StringBuilder builderInLine1 = new StringBuilder(), builderInLine2 = new StringBuilder(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 + + StringBuilder builderCfg = new StringBuilder(); // FN (requires use of cfg with significant impact on performance) + if (false) + { + builderCfg.ToString(); + } + + StringBuilder builderCalls = new StringBuilder(); // Noncompliant + builderCalls.Append("Append"); + builderCalls.AppendLine("AppendLine"); + builderCalls.Replace("\r\n", "\n"); + builderCalls.Clear(); + builderCalls.GetType(); + + StringBuilder mySbField = new StringBuilder(); // Noncompliant + MyClass myClass = new MyClass(); + myClass.mySbField.ToString(); + + var builderReturn = new StringBuilder(); // Compliant + return builderReturn; + + void LocalMethod(StringBuilder local) + { + local.ToString(); + } + } + + public StringBuilder GetStringBuilder() => // Compliant + new StringBuilder(); + + public void ExternalMethod(StringBuilder builder) { } // Compliant + + public string AnotherMethod() + { + var builder = new StringBuilder(); // FP + return $"{builder} is ToStringed here"; + } + + public void Scoping() + { + { + var sb = new StringBuilder(); // Noncompliant + } + { + var sb = new StringBuilder(); + sb.ToString(); + } + } + + public string MyProperty + { + get + { + var builder1 = new StringBuilder(); // Noncompliant + var builder2 = new StringBuilder(); // Compliant + return builder2.ToString(); + } + } + + private StringBuilder myField = new StringBuilder(); // Compliant +} + +public class MyClass +{ + public StringBuilder mySbField = new StringBuilder(); +} + +public static class StringBuilderExtensions +{ + public static string ToStringAndFree(this StringBuilder sb) => string.Empty; +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb new file mode 100644 index 00000000000..b8b636d71a8 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -0,0 +1,70 @@ +Imports System.Text + +Public Class Program + Public Function NotUsed(ByVal builder As StringBuilder) As StringBuilder + Dim builder1 As StringBuilder = GetStringBuilder() ' Compliant + Dim builder2 = New StringBuilder() ' Compliant + ExternalMethod(builder2) + Dim builder3 As StringBuilder = New StringBuilder() ' Compliant + builder3.ToString() + Dim builder4 = New StringBuilder() ' Compliant + Dim builder5 As StringBuilder = New StringBuilder() ' Noncompliant + Dim builder6 = New StringBuilder() ' Noncompliant + Dim builder7 As StringBuilder = New StringBuilder() ' Noncompliant + builder7.Append(builder4.ToString()) + 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 sb1, sb2 ' Compliant + Dim sb3, sb4 As StringBuilder = New StringBuilder() ' Error [BC30671]: Explicit initialization is not permitted with multiple variables declared with a single type specifier. + + Dim sb5 As StringBuilder = New StringBuilder() ' Compliant + Dim sb6 As StringBuilder = New StringBuilder() ' Noncompliant + sb6 = sb5 + + Dim builderCfg As StringBuilder = New StringBuilder() ' FN (requires use of cfg with significant impact on performance) + If False Then + builderCfg.ToString() + End If + + Dim builderCalls As StringBuilder = New StringBuilder() ' Noncompliant + builderCalls.Append("Append") + builderCalls.AppendLine("AppendLine") + builderCalls.Replace("a", "b") + builderCalls.Clear() + + 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 + + Public Function GetStringBuilder() As StringBuilder ' Compliant + Return New StringBuilder() + End Function + + Public Sub ExternalMethod(ByVal builder As StringBuilder) ' Compliant + End Sub + + Public ReadOnly Property MyProperty As String + Get + Dim builder1 = New StringBuilder() ' Noncompliant + Dim builder2 = New StringBuilder() ' Compliant + Return builder2.ToString() + End Get + End Property + + Private myField As StringBuilder = New StringBuilder() ' Compliant +End Class + +Public Class [MyClass] + Public mySbField As StringBuilder = New StringBuilder() +End Class