From c99b62c88a91f50694f66ddfc45f53086b2f65e2 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 30 Jan 2023 17:58:18 +0100 Subject: [PATCH 01/41] Base --- analyzers/rspec/cs/S3063_c#.html | 37 ++++++++++++++++++ analyzers/rspec/cs/S3063_c#.json | 17 +++++++++ analyzers/rspec/cs/Sonar_way_profile.json | 1 + analyzers/rspec/vbnet/S3063_vb.net.html | 37 ++++++++++++++++++ analyzers/rspec/vbnet/S3063_vb.net.json | 17 +++++++++ analyzers/rspec/vbnet/Sonar_way_profile.json | 1 + .../Rules/UnusedStringBuilder.cs | 38 +++++++++++++++++++ .../Rules/UnusedStringBuilderBase.cs | 31 +++++++++++++++ .../Rules/UnusedStringBuilder.cs | 38 +++++++++++++++++++ .../PackagingTests/RuleTypeMappingCS.cs | 2 +- .../PackagingTests/RuleTypeMappingVB.cs | 2 +- .../Rules/UnusedStringBuilderTest.cs | 36 ++++++++++++++++++ .../TestCases/UnusedStringBuilder.cs | 5 +++ .../TestCases/UnusedStringBuilder.vb | 7 ++++ 14 files changed, 267 insertions(+), 2 deletions(-) create mode 100644 analyzers/rspec/cs/S3063_c#.html create mode 100644 analyzers/rspec/cs/S3063_c#.json create mode 100644 analyzers/rspec/vbnet/S3063_vb.net.html create mode 100644 analyzers/rspec/vbnet/S3063_vb.net.json create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs create mode 100644 analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs create mode 100644 analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb 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 5879a1c7933..809c3d98db9 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -125,6 +125,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 46bcb99963e..72241e5a595 100644 --- a/analyzers/rspec/vbnet/Sonar_way_profile.json +++ b/analyzers/rspec/vbnet/Sonar_way_profile.json @@ -71,6 +71,7 @@ "S2761", "S2951", "S3011", + "S3063", "S3358", "S3385", "S3449", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs new file mode 100644 index 00000000000..78672b95e8b --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -0,0 +1,38 @@ +/* + * 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 +{ + protected override ILanguageFacade Language => CSharpFacade.Instance; + + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => + { + var node = c.Node; + if (true) + { + c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); + } + }, + SyntaxKind.InvocationExpression); +} diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs new file mode 100644 index 00000000000..1270123e22a --- /dev/null +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -0,0 +1,31 @@ +/* + * 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 +{ + private const string DiagnosticId = "S3063"; + + protected override string MessageFormat => "Remove this \"StringBuilder\"; it is appended to but \".toString()\" is never called."; + + protected UnusedStringBuilderBase() : base(DiagnosticId) { } +} diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs new file mode 100644 index 00000000000..3e192e1d7d8 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -0,0 +1,38 @@ +/* + * 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 +{ + protected override ILanguageFacade Language => VisualBasicFacade.Instance; + + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => + { + var node = c.Node; + if (true) + { + c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); + } + }, + SyntaxKind.InvocationExpression); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs index 1f44dcb4ef6..39b679f51e0 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 54584469d62..9bdb150411e 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..4322a148d1a --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -0,0 +1,36 @@ +/* + * 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 +{ + [TestMethod] + public void UnusedStringBuilder_CS() => + new VerifierBuilder().AddPaths("UnusedStringBuilder.cs").Verify(); + + [TestMethod] + public void UnusedStringBuilder_VB() => + new VerifierBuilder().AddPaths("UnusedStringBuilder.vb").Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs new file mode 100644 index 00000000000..a790d6d1f92 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -0,0 +1,5 @@ +using System; + +public class Program +{ +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb new file mode 100644 index 00000000000..f5b9e9276c6 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -0,0 +1,7 @@ +Public Class Program + + Public Sub Test() + + End Sub + +End Class From 08f0ba3d89e1d72bd92044092f8e1bfe8827be83 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 10:24:30 +0100 Subject: [PATCH 02/41] Add test cases --- .../TestCases/UnusedStringBuilder.CSharp9.cs | 10 +++ .../TestCases/UnusedStringBuilder.cs | 64 ++++++++++++++++++- .../TestCases/UnusedStringBuilder.vb | 45 ++++++++++++- 3 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs 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..3b51905c4b8 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -0,0 +1,10 @@ +using System.Text; + +StringBuilder builder1 = new(); // Compliant + +StringBuilder builder2 = new(); // Noncompliant + +StringBuilder builder3 = new(); // Noncompliant +builder3.Append(builder1.ToString()); + +(StringBuilder, StringBuilder) builder4 = (new(), new()); // FN diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index a790d6d1f92..300332b5eb7 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -1,5 +1,67 @@ -using System; +using System.Collections.Generic; +using System.Text; public class Program { + public StringBuilder NotUsed(StringBuilder builder) // Compliant + { + StringBuilder builder1 = GetStringBuilder(); // Compliant + + var builder2 = new StringBuilder(); // Compliant + ExternalMethod(builder2); + + var builder3 = new StringBuilder(); // Compliant + LocalMethod(builder3); + + StringBuilder builder4 = new StringBuilder(); // Compliant + builder4.ToString(); + + var builder5 = new StringBuilder(); // Compliant + + StringBuilder builder6 = new StringBuilder(); // Noncompliant + var builder7 = new StringBuilder(); // Noncompliant + + StringBuilder builder8 = new StringBuilder(); // Noncompliant + builder8.Append(builder5.ToString()); + + (StringBuilder, StringBuilder) builder9 = (new StringBuilder(), new StringBuilder()); // FN + + StringBuilder builder10 = new StringBuilder(); // FN + if (false) + { + builder10.ToString(); + } + + StringBuilder builder11 = new StringBuilder(); // Noncompliant + builder11.Append("Append"); + builder11.AppendLine("AppendLine"); + builder11.Replace("\r\n", "\n"); + builder11.Remove(builder11.Length - 1, 1); + builder11.Insert(builder11.Length, "\r\n"); + builder11.Clear(); + builder11.GetType(); + + var builder12 = new StringBuilder(); // Compliant + return builder12; + + void LocalMethod(StringBuilder b) + { + b.ToString(); + } + } + + public StringBuilder GetStringBuilder() => // Compliant + new StringBuilder(); + + public void ExternalMethod(StringBuilder builder) { } // Compliant + + public string MyProperty + { + get + { + var builder1 = new StringBuilder(); // Noncompliant + var builder2 = new StringBuilder(); // Compliant + return builder2.ToString(); + } + } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index f5b9e9276c6..0cdbb08a77f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -1,7 +1,48 @@ -Public Class Program +Imports System.Text - Public Sub Test() +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() ' FN + + If False Then + builder8.ToString() + End If + + Dim builder9 As StringBuilder = New StringBuilder() ' Noncompliant + builder9.Append("Append") + builder9.AppendLine("AppendLine") + builder9.Replace("a", "b") + builder9.Remove(builder9.Length - 1, 1) + builder9.Insert(builder9.Length, "a") + builder9.Clear() + Dim builder10 = New StringBuilder() ' Compliant + Return builder10 + 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 End Class + From b7d0d30a0a7e140b29a56e7024aac8bb5bd30c17 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 14:05:27 +0100 Subject: [PATCH 03/41] First implementation CS/VB --- .../Rules/UnusedStringBuilder.cs | 45 ++++++++++++++----- .../Rules/UnusedStringBuilderBase.cs | 43 ++++++++++++++++-- .../Rules/UnusedStringBuilder.cs | 44 +++++++++++++----- .../Rules/UnusedStringBuilderTest.cs | 14 +++++- .../TestCases/UnusedStringBuilder.CSharp9.cs | 19 ++++++-- .../TestCases/UnusedStringBuilder.cs | 6 ++- 6 files changed, 139 insertions(+), 32 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 78672b95e8b..de0e0935be7 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -21,18 +21,41 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => CSharpFacade.Instance; - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => - { - var node = c.Node; - if (true) - { - c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); - } - }, - SyntaxKind.InvocationExpression); + private const string StringBuilderToString = "System.Text.StringBuilder.ToString()"; + + protected override string GetVariableName(VariableDeclaratorSyntax declaration) => + declaration.GetIdentifier().ToString(); + + protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => + declaration.Initializer.Value is { } expression && expression switch + { + ObjectCreationExpressionSyntax => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), + _ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression) => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), + _ => false + }; + + protected override SyntaxNode GetAncestorBlock(VariableDeclaratorSyntax declaration) => + declaration.Ancestors().OfType().FirstOrDefault(); + + protected override bool IsIsStringInvoked(string variableName, List invocations, SemanticModel semanticModel) => + invocations.Where(x => x.Expression is MemberAccessExpressionSyntax { } member + && IsSameVariable(member.Expression, variableName) + && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel) + && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol + && symbol.OriginalDefinition.ToString().Equals(StringBuilderToString)).Any(); + + protected override bool IsPassedToMethod(string variableName, List invocations) => + invocations.Where(x => x.ArgumentList.Arguments.Where(y => IsSameVariable(y.Expression, variableName)).Any()).Any(); + + protected override bool IsReturned(string variableName, List returnStatements) => + returnStatements.Where(x => IsSameVariable(x.Expression, variableName)).Any(); + + private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => + expression is IdentifierNameSyntax identifierName + && identifierName.GetIdentifier().Value is SyntaxToken { } token + && token.ToString().Equals(variableName); } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 1270123e22a..8c01deec6aa 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -20,12 +20,49 @@ namespace SonarAnalyzer.Rules; -public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer +public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer where TSyntaxKind : struct + where TVariableDeclarator : SyntaxNode + where TInvocationExpression : SyntaxNode + where TReturnStatement : SyntaxNode { private const string DiagnosticId = "S3063"; + protected override string MessageFormat => """Remove this "StringBuilder"; ".ToString()" is never called."""; - protected override string MessageFormat => "Remove this \"StringBuilder\"; it is appended to but \".toString()\" is never called."; + private readonly DiagnosticDescriptor rule; - protected UnusedStringBuilderBase() : base(DiagnosticId) { } + protected abstract string GetVariableName(TVariableDeclarator declaration); + protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel); + protected abstract SyntaxNode GetAncestorBlock(TVariableDeclarator declaration); + protected abstract bool IsIsStringInvoked(string variableName, List invocations, SemanticModel semanticModel); + protected abstract bool IsPassedToMethod(string variableName, List invocations); + protected abstract bool IsReturned(string variableName, List returnStatements); + + protected UnusedStringBuilderBase() : base(DiagnosticId) => + rule = Language.CreateDescriptor(DiagnosticId, MessageFormat); + + protected sealed override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c => + { + var variableDeclaration = (TVariableDeclarator)c.Node; + if (!NeedsToTrack(variableDeclaration, c.SemanticModel)) + { + return; + } + var variableName = GetVariableName(variableDeclaration); + + var block = GetAncestorBlock(variableDeclaration); + if (block == null || string.IsNullOrEmpty(variableName)) + { + return; + } + var invocations = block.DescendantNodes().OfType().ToList(); + if (IsIsStringInvoked(variableName, invocations, c.SemanticModel) + || IsPassedToMethod(variableName, invocations) + || IsReturned(variableName, block.DescendantNodes().OfType().ToList())) + { + return; + } + c.ReportIssue(Diagnostic.Create(rule, variableDeclaration.GetLocation())); + }, Language.SyntaxKind.VariableDeclarator); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 3e192e1d7d8..a31f08b051b 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -18,21 +18,43 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using System.Text; + namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => VisualBasicFacade.Instance; - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => - { - var node = c.Node; - if (true) - { - c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); - } - }, - SyntaxKind.InvocationExpression); + private const string StringBuilderToString = "System.Text.StringBuilder.ToString()"; + + protected override string GetVariableName(VariableDeclaratorSyntax declaration) => + declaration.Names.FirstOrDefault().ToString(); + + protected override bool NeedsToTrack(VariableDeclaratorSyntax expression, SemanticModel semanticModel) => + expression.Initializer.Value is ObjectCreationExpressionSyntax objectCreation + && objectCreation.Type.ToString().Equals(nameof(StringBuilder)); + + protected override SyntaxNode GetAncestorBlock(VariableDeclaratorSyntax declaration) => + declaration.Ancestors().OfType().Any() + ? declaration.Ancestors().OfType().First() + : declaration.Ancestors().OfType().FirstOrDefault(); + + protected override bool IsIsStringInvoked(string variableName, List invocations, SemanticModel semanticModel) => + invocations.Where(x => x.Expression is MemberAccessExpressionSyntax { } member + && IsSameVariable(member.Expression, variableName) + && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel) + && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol).Any(); + + protected override bool IsPassedToMethod(string variableName, List invocations) => + invocations.Where(x => x.ArgumentList.Arguments.Where(y => IsSameVariable(y.GetExpression(), variableName)).Any()).Any(); + + protected override bool IsReturned(string variableName, List returnStatements) => + returnStatements.Where(x => IsSameVariable(x.Expression, variableName)).Any(); + + private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => + expression is IdentifierNameSyntax identifierName + && identifierName.GetIdentifier().Value is SyntaxToken { } token + && token.ToString().Equals(variableName); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 4322a148d1a..65b9de4a6d1 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -26,9 +26,21 @@ namespace SonarAnalyzer.UnitTest.Rules; [TestClass] public class UnusedStringBuilderTest { + private readonly VerifierBuilder builderCS = new VerifierBuilder(); + [TestMethod] public void UnusedStringBuilder_CS() => - new VerifierBuilder().AddPaths("UnusedStringBuilder.cs").Verify(); + builderCS.AddPaths("UnusedStringBuilder.cs").Verify(); + +#if NET + + [TestMethod] + public void UnusedStringBuilder_CSharp9() => + builderCS.AddPaths("UnusedStringBuilder.CSharp9.cs") + .WithTopLevelStatements() + .Verify(); + +#endif [TestMethod] public void UnusedStringBuilder_VB() => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs index 3b51905c4b8..84bcf5b6fb7 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -1,10 +1,21 @@ using System.Text; StringBuilder builder1 = new(); // Compliant +StringBuilder builder2 = new(); // FN (Top level statement not covered yet) +builder2.Append(builder1.ToString()); -StringBuilder builder2 = new(); // Noncompliant +public class MyClass +{ + public void MyMethod(StringBuilder builder) // Compliant + { + StringBuilder builder1 = new(); // Compliant -StringBuilder builder3 = new(); // Noncompliant -builder3.Append(builder1.ToString()); + StringBuilder builder2 = new(); // Noncompliant +// ^^^^^^^^^^^^^^^^ -(StringBuilder, StringBuilder) builder4 = (new(), new()); // FN + StringBuilder builder3 = new(); // Noncompliant + builder3.Append(builder1.ToString()); + + (StringBuilder, StringBuilder) builder4 = (new(), new()); // FN + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 300332b5eb7..f58e6ca7735 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -3,7 +3,7 @@ public class Program { - public StringBuilder NotUsed(StringBuilder builder) // Compliant + public StringBuilder MyMethod(StringBuilder builder) // Compliant { StringBuilder builder1 = GetStringBuilder(); // Compliant @@ -18,8 +18,10 @@ public class Program var builder5 = new StringBuilder(); // Compliant - StringBuilder builder6 = new StringBuilder(); // Noncompliant + StringBuilder builder6 = new StringBuilder(); // Noncompliant {{Remove this "StringBuilder"; ".ToString()" is never called.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ var builder7 = new StringBuilder(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ StringBuilder builder8 = new StringBuilder(); // Noncompliant builder8.Append(builder5.ToString()); From e4879131f4e6ca52ae58075861aa4d4868ed4ea3 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 15:32:26 +0100 Subject: [PATCH 04/41] Fix UTs --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 4 +++- .../SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index de0e0935be7..ad34f7ec72e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -31,7 +31,9 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - declaration.Initializer.Value is { } expression && expression switch + declaration.Initializer is not null + && declaration.Initializer.Value is { } expression + && expression switch { ObjectCreationExpressionSyntax => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), _ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression) => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index a31f08b051b..abef59d8558 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -33,7 +33,8 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - expression.Initializer.Value is ObjectCreationExpressionSyntax objectCreation + expression.Initializer is not null + && expression.Initializer.Value is ObjectCreationExpressionSyntax objectCreation && objectCreation.Type.ToString().Equals(nameof(StringBuilder)); protected override SyntaxNode GetAncestorBlock(VariableDeclaratorSyntax declaration) => From 7ddcd31bbbb849c9d5f469b91bf7e19053a3beb5 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 16:07:27 +0100 Subject: [PATCH 05/41] Fixed code smells + added test cases --- .../Rules/UnusedStringBuilder.cs | 14 ++++---- .../Rules/UnusedStringBuilderBase.cs | 6 ++-- .../Rules/UnusedStringBuilder.cs | 17 ++++----- .../TestCases/UnusedStringBuilder.cs | 35 ++++++++++++------- .../TestCases/UnusedStringBuilder.vb | 25 ++++++------- 5 files changed, 52 insertions(+), 45 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index ad34f7ec72e..c00e479bebb 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -43,18 +43,18 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase declaration.Ancestors().OfType().FirstOrDefault(); - protected override bool IsIsStringInvoked(string variableName, List invocations, SemanticModel semanticModel) => - invocations.Where(x => x.Expression is MemberAccessExpressionSyntax { } member + protected override bool IsIsStringInvoked(string variableName, IList invocations, SemanticModel semanticModel) => + invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel) && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol - && symbol.OriginalDefinition.ToString().Equals(StringBuilderToString)).Any(); + && symbol.OriginalDefinition.ToString().Equals(StringBuilderToString)); - protected override bool IsPassedToMethod(string variableName, List invocations) => - invocations.Where(x => x.ArgumentList.Arguments.Where(y => IsSameVariable(y.Expression, variableName)).Any()).Any(); + protected override bool IsPassedToMethod(string variableName, IList invocations) => + invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); - protected override bool IsReturned(string variableName, List returnStatements) => - returnStatements.Where(x => IsSameVariable(x.Expression, variableName)).Any(); + protected override bool IsReturned(string variableName, IList returnStatements) => + returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => expression is IdentifierNameSyntax identifierName diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 8c01deec6aa..cc4f15b5ba7 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -34,9 +34,9 @@ public abstract class UnusedStringBuilderBase invocations, SemanticModel semanticModel); - protected abstract bool IsPassedToMethod(string variableName, List invocations); - protected abstract bool IsReturned(string variableName, List returnStatements); + protected abstract bool IsIsStringInvoked(string variableName, IList invocations, SemanticModel semanticModel); + protected abstract bool IsPassedToMethod(string variableName, IList invocations); + protected abstract bool IsReturned(string variableName, IList returnStatements); protected UnusedStringBuilderBase() : base(DiagnosticId) => rule = Language.CreateDescriptor(DiagnosticId, MessageFormat); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index abef59d8558..6735e4712ce 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -27,8 +27,6 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - private const string StringBuilderToString = "System.Text.StringBuilder.ToString()"; - protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.Names.FirstOrDefault().ToString(); @@ -42,17 +40,16 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase().First() : declaration.Ancestors().OfType().FirstOrDefault(); - protected override bool IsIsStringInvoked(string variableName, List invocations, SemanticModel semanticModel) => - invocations.Where(x => x.Expression is MemberAccessExpressionSyntax { } member + protected override bool IsIsStringInvoked(string variableName, IList invocations, SemanticModel semanticModel) => + invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel) - && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol).Any(); + && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel)); - protected override bool IsPassedToMethod(string variableName, List invocations) => - invocations.Where(x => x.ArgumentList.Arguments.Where(y => IsSameVariable(y.GetExpression(), variableName)).Any()).Any(); + protected override bool IsPassedToMethod(string variableName, IList invocations) => + invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); - protected override bool IsReturned(string variableName, List returnStatements) => - returnStatements.Where(x => IsSameVariable(x.Expression, variableName)).Any(); + protected override bool IsReturned(string variableName, IList returnStatements) => + returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => expression is IdentifierNameSyntax identifierName diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index f58e6ca7735..ba34757129d 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -26,25 +26,28 @@ public class Program StringBuilder builder8 = new StringBuilder(); // Noncompliant builder8.Append(builder5.ToString()); - (StringBuilder, StringBuilder) builder9 = (new StringBuilder(), new StringBuilder()); // FN + StringBuilder builder9 = new StringBuilder(); // Compliant + var a = builder9.Append("&").ToString(); - StringBuilder builder10 = new StringBuilder(); // FN + (StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN + + StringBuilder builderCfg = new StringBuilder(); // FN (we should use cfg with significant impact on performance) if (false) { - builder10.ToString(); + builderCfg.ToString(); } - StringBuilder builder11 = new StringBuilder(); // Noncompliant - builder11.Append("Append"); - builder11.AppendLine("AppendLine"); - builder11.Replace("\r\n", "\n"); - builder11.Remove(builder11.Length - 1, 1); - builder11.Insert(builder11.Length, "\r\n"); - builder11.Clear(); - builder11.GetType(); + StringBuilder builderCalls = new StringBuilder(); // Noncompliant + builderCalls.Append("Append"); + builderCalls.AppendLine("AppendLine"); + builderCalls.Replace("\r\n", "\n"); + builderCalls.Remove(builderCalls.Length - 1, 1); + builderCalls.Insert(builderCalls.Length, "\r\n"); + builderCalls.Clear(); + builderCalls.GetType(); - var builder12 = new StringBuilder(); // Compliant - return builder12; + var builderReturn = new StringBuilder(); // Compliant + return builderReturn; void LocalMethod(StringBuilder b) { @@ -57,6 +60,12 @@ void LocalMethod(StringBuilder b) public void ExternalMethod(StringBuilder builder) { } // Compliant + public string AnotherMethod() + { + var builder = new StringBuilder(); // FP + return $"{builder} is ToStringed here"; + } + public string MyProperty { get diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 0cdbb08a77f..10707c4bacf 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -12,22 +12,23 @@ Public Class Program 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 builder8 As StringBuilder = New StringBuilder() ' FN - + Dim builderCfg As StringBuilder = New StringBuilder() ' FN (we should use cfg with significant impact on performance) If False Then - builder8.ToString() + builderCfg.ToString() End If - Dim builder9 As StringBuilder = New StringBuilder() ' Noncompliant - builder9.Append("Append") - builder9.AppendLine("AppendLine") - builder9.Replace("a", "b") - builder9.Remove(builder9.Length - 1, 1) - builder9.Insert(builder9.Length, "a") - builder9.Clear() - Dim builder10 = New StringBuilder() ' Compliant - Return builder10 + Dim builderCalls As StringBuilder = New StringBuilder() ' Noncompliant + builderCalls.Append("Append") + builderCalls.AppendLine("AppendLine") + builderCalls.Replace("a", "b") + builderCalls.Remove(builderCalls.Length - 1, 1) + builderCalls.Insert(builderCalls.Length, "a") + builderCalls.Clear() + Dim builderReturn = New StringBuilder() ' Compliant + Return builderReturn End Function Public Function GetStringBuilder() As StringBuilder ' Compliant From 1dee71603e01146d20f4f6726fdba8c228f614bb Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 16:50:11 +0100 Subject: [PATCH 06/41] More test cases --- .../Rules/UnusedStringBuilder.cs | 2 +- .../TestCases/UnusedStringBuilder.cs | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index c00e479bebb..10e1573905a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -46,7 +46,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase invocations, SemanticModel semanticModel) => invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel) + && member.NameIs(nameof(ToString)) && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol && symbol.OriginalDefinition.ToString().Equals(StringBuilderToString)); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index ba34757129d..e0b5d3c569c 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -27,7 +27,13 @@ public class Program builder8.Append(builder5.ToString()); StringBuilder builder9 = new StringBuilder(); // Compliant - var a = builder9.Append("&").ToString(); + var a = builder9.ToString(); + + StringBuilder builder10 = new StringBuilder(); // Compliant + var b = builder10.ToString().ToLower(); + + StringBuilder builder11 = new StringBuilder(); // Compliant + var c = builder11.Append("").Append("").Append("").Append("").ToString().ToLower(); (StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN @@ -49,9 +55,9 @@ public class Program var builderReturn = new StringBuilder(); // Compliant return builderReturn; - void LocalMethod(StringBuilder b) + void LocalMethod(StringBuilder local) { - b.ToString(); + local.ToString(); } } From b4c1029280c8c548b1cd95d1e6dc76836d413276 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 17:51:51 +0100 Subject: [PATCH 07/41] Fixed CS test cases --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 6 ++---- .../SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs | 2 +- .../SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs | 6 ++---- .../SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs | 5 +++-- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 10e1573905a..d6c6203a63d 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -28,7 +28,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - declaration.GetIdentifier().ToString(); + declaration.GetName(); protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration.Initializer is not null @@ -57,7 +57,5 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase IsSameVariable(x.Expression, variableName)); private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression is IdentifierNameSyntax identifierName - && identifierName.GetIdentifier().Value is SyntaxToken { } token - && token.ToString().Equals(variableName); + expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index cc4f15b5ba7..5cb13889643 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -50,13 +50,13 @@ public abstract class UnusedStringBuilderBase().ToList(); + if (IsIsStringInvoked(variableName, invocations, c.SemanticModel) || IsPassedToMethod(variableName, invocations) || IsReturned(variableName, block.DescendantNodes().OfType().ToList())) diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 6735e4712ce..4bd93e6f992 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -28,7 +28,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; protected override string GetVariableName(VariableDeclaratorSyntax declaration) => - declaration.Names.FirstOrDefault().ToString(); + declaration.GetName(); protected override bool NeedsToTrack(VariableDeclaratorSyntax expression, SemanticModel semanticModel) => expression.Initializer is not null @@ -52,7 +52,5 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase IsSameVariable(x.Expression, variableName)); private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression is IdentifierNameSyntax identifierName - && identifierName.GetIdentifier().Value is SyntaxToken { } token - && token.ToString().Equals(variableName); + expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index e0b5d3c569c..8bf14458518 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -47,11 +47,12 @@ public class Program builderCalls.Append("Append"); builderCalls.AppendLine("AppendLine"); builderCalls.Replace("\r\n", "\n"); - builderCalls.Remove(builderCalls.Length - 1, 1); - builderCalls.Insert(builderCalls.Length, "\r\n"); builderCalls.Clear(); builderCalls.GetType(); + var builderCalls2 = new StringBuilder(); // Compliant + builderCalls2.Remove(builderCalls2.Length - 1, 1); + var builderReturn = new StringBuilder(); // Compliant return builderReturn; From 97a242110f390bd0a1e2b0171d3f6ff958654b4c Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 1 Feb 2023 18:06:45 +0100 Subject: [PATCH 08/41] Fixed VB test cases --- .../SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs | 8 ++------ .../Rules/UnusedStringBuilder.cs | 2 +- .../TestCases/UnusedStringBuilder.vb | 6 ++++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 5cb13889643..99551cdb288 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -29,8 +29,6 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; - private readonly DiagnosticDescriptor rule; - protected abstract string GetVariableName(TVariableDeclarator declaration); protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel); protected abstract SyntaxNode GetAncestorBlock(TVariableDeclarator declaration); @@ -38,8 +36,7 @@ public abstract class UnusedStringBuilderBase invocations); protected abstract bool IsReturned(string variableName, IList returnStatements); - protected UnusedStringBuilderBase() : base(DiagnosticId) => - rule = Language.CreateDescriptor(DiagnosticId, MessageFormat); + protected UnusedStringBuilderBase() : base(DiagnosticId) { } protected sealed override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c => @@ -56,13 +53,12 @@ public abstract class UnusedStringBuilderBase().ToList(); - if (IsIsStringInvoked(variableName, invocations, c.SemanticModel) || IsPassedToMethod(variableName, invocations) || IsReturned(variableName, block.DescendantNodes().OfType().ToList())) { return; } - c.ReportIssue(Diagnostic.Create(rule, variableDeclaration.GetLocation())); + c.ReportIssue(Diagnostic.Create(Rule, variableDeclaration.GetLocation())); }, Language.SyntaxKind.VariableDeclarator); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 4bd93e6f992..ee7f936fa73 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -28,7 +28,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; protected override string GetVariableName(VariableDeclaratorSyntax declaration) => - declaration.GetName(); + declaration.Names.FirstOrDefault().ToString(); protected override bool NeedsToTrack(VariableDeclaratorSyntax expression, SemanticModel semanticModel) => expression.Initializer is not null diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 10707c4bacf..7b419371f62 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -24,9 +24,11 @@ Public Class Program builderCalls.Append("Append") builderCalls.AppendLine("AppendLine") builderCalls.Replace("a", "b") - builderCalls.Remove(builderCalls.Length - 1, 1) - builderCalls.Insert(builderCalls.Length, "a") builderCalls.Clear() + + Dim builderCalls2 As StringBuilder = New StringBuilder() ' Compliant + builderCalls2.Remove(builderCalls2.Length - 1, 1) + Dim builderReturn = New StringBuilder() ' Compliant Return builderReturn End Function From d2554e66e46d2ba30ad0566be5940d1672c2dfc5 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 2 Feb 2023 15:15:02 +0100 Subject: [PATCH 09/41] Semplifications --- .../Rules/UnusedStringBuilder.cs | 22 +++++++------------ .../Rules/UnusedStringBuilderBase.cs | 4 ++-- .../Rules/UnusedStringBuilder.cs | 11 ++++------ 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index d6c6203a63d..0bb7c77668e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -25,30 +25,20 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => CSharpFacade.Instance; - private const string StringBuilderToString = "System.Text.StringBuilder.ToString()"; - - protected override string GetVariableName(VariableDeclaratorSyntax declaration) => - declaration.GetName(); + protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.GetName(); protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration.Initializer is not null && declaration.Initializer.Value is { } expression - && expression switch - { - ObjectCreationExpressionSyntax => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), - _ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression) => ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel), - _ => false - }; + && IsStringBuilderObjectCreation(expression, semanticModel); protected override SyntaxNode GetAncestorBlock(VariableDeclaratorSyntax declaration) => declaration.Ancestors().OfType().FirstOrDefault(); - protected override bool IsIsStringInvoked(string variableName, IList invocations, SemanticModel semanticModel) => + protected override bool IsIsStringInvoked(string variableName, IList invocations) => invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.NameIs(nameof(ToString)) - && semanticModel.GetSymbolInfo(x).Symbol is IMethodSymbol symbol - && symbol.OriginalDefinition.ToString().Equals(StringBuilderToString)); + && member.NameIs(nameof(ToString))); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); @@ -56,6 +46,10 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase returnStatements) => returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); + private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => + (expression is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression)) + && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); + private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 99551cdb288..9572b88fa5e 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -32,7 +32,7 @@ public abstract class UnusedStringBuilderBase invocations, SemanticModel semanticModel); + protected abstract bool IsIsStringInvoked(string variableName, IList invocations); protected abstract bool IsPassedToMethod(string variableName, IList invocations); protected abstract bool IsReturned(string variableName, IList returnStatements); @@ -53,7 +53,7 @@ public abstract class UnusedStringBuilderBase().ToList(); - if (IsIsStringInvoked(variableName, invocations, c.SemanticModel) + if (IsIsStringInvoked(variableName, invocations) || IsPassedToMethod(variableName, invocations) || IsReturned(variableName, block.DescendantNodes().OfType().ToList())) { diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index ee7f936fa73..19f2be69a72 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -27,8 +27,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - protected override string GetVariableName(VariableDeclaratorSyntax declaration) => - declaration.Names.FirstOrDefault().ToString(); + protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.Names.FirstOrDefault().ToString(); protected override bool NeedsToTrack(VariableDeclaratorSyntax expression, SemanticModel semanticModel) => expression.Initializer is not null @@ -36,14 +35,12 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - declaration.Ancestors().OfType().Any() - ? declaration.Ancestors().OfType().First() - : declaration.Ancestors().OfType().FirstOrDefault(); + declaration.Ancestors().OfType().First(); - protected override bool IsIsStringInvoked(string variableName, IList invocations, SemanticModel semanticModel) => + protected override bool IsIsStringInvoked(string variableName, IList invocations) => invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.IsMemberAccessOnKnownType(nameof(ToString), KnownType.System_Text_StringBuilder, semanticModel)); + && member.NameIs(nameof(ToString))); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); From 740a16e4f1937ef0b901e2b6e47ceb1822e13555 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 3 Feb 2023 17:49:14 +0100 Subject: [PATCH 10/41] Add support for TopLevelStatements and StringBuilder access methods --- .../VariableDeclaratorSyntaxExtensions.cs | 27 ++++++++++++++++ .../Rules/UnusedStringBuilder.cs | 28 ++++++++++++++--- .../Rules/UnusedStringBuilderBase.cs | 18 +++++------ .../Rules/UnusedStringBuilder.cs | 15 ++++----- .../TestCases/UnusedStringBuilder.CSharp9.cs | 6 ++-- .../TestCases/UnusedStringBuilder.cs | 31 ++++++++++++++++++- 6 files changed, 100 insertions(+), 25 deletions(-) create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs new file mode 100644 index 00000000000..4af1add82cc --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs @@ -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) => + declarator.Parent.Parent is { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax } }; +} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 0bb7c77668e..9c11517999a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -32,13 +32,20 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - declaration.Ancestors().OfType().FirstOrDefault(); + protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => + declaration.IsTopLevel() + ? GetTopLevelInvocations(declaration) + : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsIsStringInvoked(string variableName, IList invocations) => + protected override IList GetReturnStatements(VariableDeclaratorSyntax declaration) => + declaration.IsTopLevel() + ? new() + : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); + + protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.NameIs(nameof(ToString))); + && StringBuilderAccessMethods.Contains(member.GetName())); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); @@ -52,4 +59,17 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); + + private static IList GetTopLevelInvocations(VariableDeclaratorSyntax declaration) + { + List list = new(); + foreach (var globalStatement in declaration.Parent.Parent.Parent.Parent.DescendantNodes().OfType()) + { + foreach (var invocation in globalStatement.DescendantNodes().OfType()) + { + list.Add(invocation); + } + } + return list; + } } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 9572b88fa5e..649889a7933 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -29,10 +29,13 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; + internal readonly string[] StringBuilderAccessMethods = { "ToString", "CopyTo", "GetChunks" }; + protected abstract string GetVariableName(TVariableDeclarator declaration); protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel); - protected abstract SyntaxNode GetAncestorBlock(TVariableDeclarator declaration); - protected abstract bool IsIsStringInvoked(string variableName, IList invocations); + protected abstract IList GetInvocations(TVariableDeclarator declaration); + protected abstract IList GetReturnStatements(TVariableDeclarator declaration); + protected abstract bool IsStringBuilderAccessed(string variableName, IList invocations); protected abstract bool IsPassedToMethod(string variableName, IList invocations); protected abstract bool IsReturned(string variableName, IList returnStatements); @@ -47,15 +50,10 @@ public abstract class UnusedStringBuilderBase().ToList(); - if (IsIsStringInvoked(variableName, invocations) + var invocations = GetInvocations(variableDeclaration); + if (IsStringBuilderAccessed(variableName, invocations) || IsPassedToMethod(variableName, invocations) - || IsReturned(variableName, block.DescendantNodes().OfType().ToList())) + || IsReturned(variableName, GetReturnStatements(variableDeclaration))) { return; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 19f2be69a72..b3c8e92aad9 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -18,8 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -using System.Text; - namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] @@ -32,15 +30,18 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase expression.Initializer is not null && expression.Initializer.Value is ObjectCreationExpressionSyntax objectCreation - && objectCreation.Type.ToString().Equals(nameof(StringBuilder)); + && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); + + protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => + declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override SyntaxNode GetAncestorBlock(VariableDeclaratorSyntax declaration) => - declaration.Ancestors().OfType().First(); + protected override IList GetReturnStatements(VariableDeclaratorSyntax declaration) => + declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsIsStringInvoked(string variableName, IList invocations) => + protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member && IsSameVariable(member.Expression, variableName) - && member.NameIs(nameof(ToString))); + && StringBuilderAccessMethods.Contains(member.GetName())); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs index 84bcf5b6fb7..1863beb4d25 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -1,8 +1,8 @@ using System.Text; -StringBuilder builder1 = new(); // Compliant -StringBuilder builder2 = new(); // FN (Top level statement not covered yet) -builder2.Append(builder1.ToString()); +StringBuilder builder1 = new(); // Noncompliant +StringBuilder builder2 = new(); // Compliant +builder1.Append(builder2.ToString()); public class MyClass { diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 8bf14458518..ff4d4c0427e 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -3,7 +3,7 @@ public class Program { - public StringBuilder MyMethod(StringBuilder builder) // Compliant + public StringBuilder MyMethod(StringBuilder builder, string myString) // Compliant { StringBuilder builder1 = GetStringBuilder(); // Compliant @@ -35,6 +35,35 @@ public class Program StringBuilder builder11 = new StringBuilder(); // Compliant var c = builder11.Append("").Append("").Append("").Append("").ToString().ToLower(); + StringBuilder builder12 = new StringBuilder(); // Compliant + builder12.CopyTo(0, new char[1], 0, 1); + + StringBuilder builder13 = new StringBuilder(); // Compliant + var d = builder13.GetChunks(); + + StringBuilder builder14 = new StringBuilder(); // Noncompliant FP + var m = builder14[0]; + + StringBuilder builder15 = new StringBuilder(); // Noncompliant FP + builder15?.ToString(); + + if (true) + { + if (true) + { + if (true) + { + if (true) + { + if (true) + { + var builder16 = new StringBuilder(); // Noncompliant + } + } + } + } + } + (StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN StringBuilder builderCfg = new StringBuilder(); // FN (we should use cfg with significant impact on performance) From 85e22ee58f267c8f854d9828e7ec12e08b2b9fb3 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 6 Feb 2023 10:32:42 +0100 Subject: [PATCH 11/41] Add support for null-conditional operator --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 8 +++++--- .../Rules/UnusedStringBuilder.cs | 6 +++--- .../TestCases/UnusedStringBuilder.cs | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 9c11517999a..703343cda02 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -43,9 +43,11 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase().ToList(); protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member - && IsSameVariable(member.Expression, variableName) - && StringBuilderAccessMethods.Contains(member.GetName())); + invocations.Any(x => x.Expression switch { + MemberAccessExpressionSyntax { } member => IsSameVariable(member.Expression, variableName) && StringBuilderAccessMethods.Contains(member.GetName()), + MemberBindingExpressionSyntax { } member => IsSameVariable(member.Ancestors().OfType().First(), variableName) && StringBuilderAccessMethods.Contains(member.GetName()), + _ => false + }); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index b3c8e92aad9..e9c27a7bb81 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -39,9 +39,9 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase().ToList(); protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member - && IsSameVariable(member.Expression, variableName) - && StringBuilderAccessMethods.Contains(member.GetName())); + invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member + && IsSameVariable(member.Expression, variableName) + && StringBuilderAccessMethods.Contains(member.GetName())); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index ff4d4c0427e..f034c0ec4ae 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -45,7 +45,7 @@ public class Program var m = builder14[0]; StringBuilder builder15 = new StringBuilder(); // Noncompliant FP - builder15?.ToString(); + builder15?.Append("").ToString(); if (true) { From e6ce8bd476fe0e97dc9433116c234a7e68a4d01f Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 6 Feb 2023 17:06:33 +0100 Subject: [PATCH 12/41] Add support for interpolated strings and VB null conditional operators --- .../Rules/UnusedStringBuilder.cs | 30 +++-- .../Rules/UnusedStringBuilderBase.cs | 8 +- .../Rules/UnusedStringBuilder.cs | 16 ++- .../Rules/UnusedStringBuilderTest.cs | 108 +++++++++++++++++- .../TestCases/UnusedStringBuilder.CSharp9.cs | 4 - .../TestCases/UnusedStringBuilder.cs | 53 +++------ 6 files changed, 154 insertions(+), 65 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 703343cda02..6e9db389516 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -21,7 +21,7 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => CSharpFacade.Instance; @@ -34,7 +34,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase GetInvocations(VariableDeclaratorSyntax declaration) => declaration.IsTopLevel() - ? GetTopLevelInvocations(declaration) + ? GetTopLevelStatementSyntax(declaration) : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); protected override IList GetReturnStatements(VariableDeclaratorSyntax declaration) => @@ -42,12 +42,13 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase().ToList(); + protected override IList GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => + declaration.IsTopLevel() + ? GetTopLevelStatementSyntax(declaration) + : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); + protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => x.Expression switch { - MemberAccessExpressionSyntax { } member => IsSameVariable(member.Expression, variableName) && StringBuilderAccessMethods.Contains(member.GetName()), - MemberBindingExpressionSyntax { } member => IsSameVariable(member.Ancestors().OfType().First(), variableName) && StringBuilderAccessMethods.Contains(member.GetName()), - _ => false - }); + invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableName)); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); @@ -55,21 +56,26 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase returnStatements) => returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsWithinInterpolatedString(string variableName, IList interpolations) => + interpolations.Any(x => IsSameVariable(x.Expression, variableName)); + private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => (expression is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression)) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); + expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)) + || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() + .DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName))); - private static IList GetTopLevelInvocations(VariableDeclaratorSyntax declaration) + private static IList GetTopLevelStatementSyntax(VariableDeclaratorSyntax declaration) { - List list = new(); + List list = new(); foreach (var globalStatement in declaration.Parent.Parent.Parent.Parent.DescendantNodes().OfType()) { - foreach (var invocation in globalStatement.DescendantNodes().OfType()) + foreach (var interpolation in globalStatement.DescendantNodes().OfType()) { - list.Add(invocation); + list.Add(interpolation); } } return list; diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 649889a7933..8ef4a80e0d3 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -20,11 +20,12 @@ namespace SonarAnalyzer.Rules; -public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer +public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer 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."""; @@ -35,9 +36,11 @@ public abstract class UnusedStringBuilderBase GetInvocations(TVariableDeclarator declaration); protected abstract IList GetReturnStatements(TVariableDeclarator declaration); + protected abstract IList GetInterpolatedStrings(TVariableDeclarator declaration); protected abstract bool IsStringBuilderAccessed(string variableName, IList invocations); protected abstract bool IsPassedToMethod(string variableName, IList invocations); protected abstract bool IsReturned(string variableName, IList returnStatements); + protected abstract bool IsWithinInterpolatedString(string variableName, IList interpolations); protected UnusedStringBuilderBase() : base(DiagnosticId) { } @@ -53,7 +56,8 @@ public abstract class UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => VisualBasicFacade.Instance; @@ -38,10 +38,11 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase GetReturnStatements(VariableDeclaratorSyntax declaration) => declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); + protected override IList GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => + declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); + protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => x.Expression is MemberAccessExpressionSyntax { } member - && IsSameVariable(member.Expression, variableName) - && StringBuilderAccessMethods.Contains(member.GetName())); + invocations.Any(x => IsSameVariable(x.Expression, variableName) && StringBuilderAccessMethods.Contains(x.GetName())); protected override bool IsPassedToMethod(string variableName, IList invocations) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); @@ -49,6 +50,11 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase returnStatements) => returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsWithinInterpolatedString(string variableName, IList interpolations) => + interpolations.Any(x => IsSameVariable(x.Expression, variableName)); + private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)); + expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)) + || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() + .DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName))); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 65b9de4a6d1..b2bcccf4100 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -27,22 +27,126 @@ namespace SonarAnalyzer.UnitTest.Rules; 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(); + [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];", false)] // FP + [DataRow("""sb?.Append("").ToString().ToLower();""", true)] + [DataRow("""@sb.Append("").ToString();""", true)] + [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("""var a = $"{sb} is ToStringed here";""", true)] + [DataRow("var a = sb.Length;", false)] // FP + public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) + { + var code = $$""" + 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(); + } + } + #if NET [TestMethod] public void UnusedStringBuilder_CSharp9() => builderCS.AddPaths("UnusedStringBuilder.CSharp9.cs") - .WithTopLevelStatements() + .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];", false)] // FP + [DataRow("""sb?.Append("").ToString().ToLower();""", true)] + [DataRow("""@sb.Append("").ToString();""", true)] + [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("""var a = $"{sb} is ToStringed here";""", true)] + [DataRow("var a = sb.Length;", false)] // FP + public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) + { + var code = $$""" + 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("""Dim a = sb.Append("").Append("").Append("").Append("").ToString().ToLower()""", true)] + [DataRow("sb.CopyTo(0, New Char(0) {}, 0, 1)", true)] + [DataRow("sb.GetChunks()", true)] + [DataRow("Dim a = sb(0)", false)] // FP + [DataRow("""sb?.Append("").ToString().ToLower()""", true)] + [DataRow("""sb.Append("").ToString()""", true)] + [DataRow("sb.Remove(sb.Length - 1, 1)", true)] + [DataRow("""Dim a = $"{sb} is ToStringed here" """, true)] + [DataRow("Dim a = sb.Length", false)] // FP + 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(); + } + } + [TestMethod] public void UnusedStringBuilder_VB() => - new VerifierBuilder().AddPaths("UnusedStringBuilder.vb").Verify(); + builderVB.AddPaths("UnusedStringBuilder.vb").Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs index 1863beb4d25..58222b0d369 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -1,9 +1,5 @@ using System.Text; -StringBuilder builder1 = new(); // Noncompliant -StringBuilder builder2 = new(); // Compliant -builder1.Append(builder2.ToString()); - public class MyClass { public void MyMethod(StringBuilder builder) // Compliant diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index f034c0ec4ae..9474f4b7f51 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -5,47 +5,23 @@ public class Program { public StringBuilder MyMethod(StringBuilder builder, string myString) // Compliant { - StringBuilder builder1 = GetStringBuilder(); // Compliant + StringBuilder sb1 = GetStringBuilder(); // Compliant - var builder2 = new StringBuilder(); // Compliant - ExternalMethod(builder2); + var sb2 = new StringBuilder(); // Compliant + ExternalMethod(sb2); - var builder3 = new StringBuilder(); // Compliant - LocalMethod(builder3); + var sb3 = new StringBuilder(); // Compliant + LocalMethod(sb3); - StringBuilder builder4 = new StringBuilder(); // Compliant - builder4.ToString(); + var sb4 = new StringBuilder(); // Compliant - var builder5 = new StringBuilder(); // Compliant + StringBuilder sb5 = new StringBuilder(); // Noncompliant {{Remove this "StringBuilder"; ".ToString()" is never called.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + var sb6 = new StringBuilder(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^ - StringBuilder builder6 = new StringBuilder(); // Noncompliant {{Remove this "StringBuilder"; ".ToString()" is never called.}} -// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - var builder7 = new StringBuilder(); // Noncompliant -// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - - StringBuilder builder8 = new StringBuilder(); // Noncompliant - builder8.Append(builder5.ToString()); - - StringBuilder builder9 = new StringBuilder(); // Compliant - var a = builder9.ToString(); - - StringBuilder builder10 = new StringBuilder(); // Compliant - var b = builder10.ToString().ToLower(); - - StringBuilder builder11 = new StringBuilder(); // Compliant - var c = builder11.Append("").Append("").Append("").Append("").ToString().ToLower(); - - StringBuilder builder12 = new StringBuilder(); // Compliant - builder12.CopyTo(0, new char[1], 0, 1); - - StringBuilder builder13 = new StringBuilder(); // Compliant - var d = builder13.GetChunks(); - - StringBuilder builder14 = new StringBuilder(); // Noncompliant FP - var m = builder14[0]; - - StringBuilder builder15 = new StringBuilder(); // Noncompliant FP - builder15?.Append("").ToString(); + StringBuilder sb7 = new StringBuilder(); // Noncompliant + sb7.Append(sb4.ToString()); if (true) { @@ -57,7 +33,7 @@ public class Program { if (true) { - var builder16 = new StringBuilder(); // Noncompliant + var buildernested = new StringBuilder(); // Noncompliant } } } @@ -79,9 +55,6 @@ public class Program builderCalls.Clear(); builderCalls.GetType(); - var builderCalls2 = new StringBuilder(); // Compliant - builderCalls2.Remove(builderCalls2.Length - 1, 1); - var builderReturn = new StringBuilder(); // Compliant return builderReturn; From 59f1967d2235ca2d296fcac86132a8c4790c3762 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 6 Feb 2023 17:46:57 +0100 Subject: [PATCH 13/41] Avoid raising for field declarations --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 1 + .../SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs | 7 ++++--- .../Rules/UnusedStringBuilderTest.cs | 1 + .../TestCases/UnusedStringBuilder.CSharp9.cs | 3 +++ .../TestCases/UnusedStringBuilder.cs | 2 ++ .../TestCases/UnusedStringBuilder.vb | 2 ++ 6 files changed, 13 insertions(+), 3 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 6e9db389516..2517cdc75d6 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -29,6 +29,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase declaration.Initializer is not null + && declaration.Parent.Parent is LocalDeclarationStatementSyntax && declaration.Initializer.Value is { } expression && IsStringBuilderObjectCreation(expression, semanticModel); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 57807d2de7e..b80071299cf 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -27,9 +27,10 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase declaration.Names.FirstOrDefault().ToString(); - protected override bool NeedsToTrack(VariableDeclaratorSyntax expression, SemanticModel semanticModel) => - expression.Initializer is not null - && expression.Initializer.Value is ObjectCreationExpressionSyntax objectCreation + protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => + declaration.Initializer is not null + && declaration.Parent is LocalDeclarationStatementSyntax + && declaration.Initializer.Value is ObjectCreationExpressionSyntax objectCreation && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index b2bcccf4100..b9340151d8e 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -123,6 +123,7 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("sb.Remove(sb.Length - 1, 1)", true)] [DataRow("""Dim a = $"{sb} is ToStringed here" """, true)] [DataRow("Dim a = sb.Length", false)] // FP + [DataRow("""SB.ToString()""", true)] public void UnusedStringBuilder_VBExpressionsTest(string expression, bool compliant) { var code = $$""" diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs index 58222b0d369..6ee1656569f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -13,5 +13,8 @@ public class MyClass builder3.Append(builder1.ToString()); (StringBuilder, StringBuilder) builder4 = (new(), new()); // FN + + StringBuilder builderInLine1 = new(), builderInLine2 = new(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 9474f4b7f51..ccd1fd86469 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -84,4 +84,6 @@ public string MyProperty return builder2.ToString(); } } + + private StringBuilder myField = new StringBuilder(); // Compliant } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 7b419371f62..3ed088b1dda 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -47,5 +47,7 @@ Public Class Program Return builder2.ToString() End Get End Property + + Private myField As StringBuilder = New StringBuilder() ' Compliant End Class From f0003ba7e46076a362059a8e09d8637b39729ea4 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 13:43:28 +0100 Subject: [PATCH 14/41] Replaced name comparison with semantic model symbol checks --- .../Rules/UnusedStringBuilder.cs | 27 ++++++++++--------- .../Rules/UnusedStringBuilderBase.cs | 20 +++++++------- .../Rules/UnusedStringBuilder.cs | 27 ++++++++++--------- .../Rules/UnusedStringBuilderTest.cs | 3 --- .../TestCases/UnusedStringBuilder.cs | 12 +++++++++ .../TestCases/UnusedStringBuilder.vb | 10 +++++++ 6 files changed, 62 insertions(+), 37 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 2517cdc75d6..8a53d1f0eb1 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -25,7 +25,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => CSharpFacade.Instance; - protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.GetName(); + protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration); protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration.Initializer is not null @@ -48,26 +48,29 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase(declaration) : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableName)); + protected override bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsPassedToMethod(string variableName, IList invocations) => - invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableName))); + protected override bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableSymbol, semanticModel))); - protected override bool IsReturned(string variableName, IList returnStatements) => - returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) => + returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsWithinInterpolatedString(string variableName, IList interpolations) => - interpolations.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => + interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => (expression is ObjectCreationExpressionSyntax || ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(expression)) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)) + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => + expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() - .DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName))); + .DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel))); + + private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => + variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); private static IList GetTopLevelStatementSyntax(VariableDeclaratorSyntax declaration) { diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 8ef4a80e0d3..c0db0e9905e 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -32,15 +32,15 @@ public abstract class UnusedStringBuilderBase GetInvocations(TVariableDeclarator declaration); protected abstract IList GetReturnStatements(TVariableDeclarator declaration); protected abstract IList GetInterpolatedStrings(TVariableDeclarator declaration); - protected abstract bool IsStringBuilderAccessed(string variableName, IList invocations); - protected abstract bool IsPassedToMethod(string variableName, IList invocations); - protected abstract bool IsReturned(string variableName, IList returnStatements); - protected abstract bool IsWithinInterpolatedString(string variableName, IList interpolations); + protected abstract bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel); protected UnusedStringBuilderBase() : base(DiagnosticId) { } @@ -52,12 +52,12 @@ public abstract class UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - protected override string GetVariableName(VariableDeclaratorSyntax declaration) => declaration.Names.FirstOrDefault().ToString(); + protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration.Names.First()); protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration.Initializer is not null @@ -42,20 +42,23 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsStringBuilderAccessed(string variableName, IList invocations) => - invocations.Any(x => IsSameVariable(x.Expression, variableName) && StringBuilderAccessMethods.Contains(x.GetName())); + protected override bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && StringBuilderAccessMethods.Contains(x.GetName())); - protected override bool IsPassedToMethod(string variableName, IList invocations) => - invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableName))); + protected override bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableSymbol, semanticModel))); - protected override bool IsReturned(string variableName, IList returnStatements) => - returnStatements.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) => + returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsWithinInterpolatedString(string variableName, IList interpolations) => - interpolations.Any(x => IsSameVariable(x.Expression, variableName)); + protected override bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => + interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - private static bool IsSameVariable(ExpressionSyntax expression, string variableName) => - expression.DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName)) + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => + expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() - .DescendantNodesAndSelf().OfType().Any(p => p.NameIs(variableName))); + .DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel))); + + private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => + variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index b9340151d8e..bbd9e0cf81d 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -44,7 +44,6 @@ public class UnusedStringBuilderTest [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] - [DataRow("var a = sb.Length;", false)] // FP public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) { var code = $$""" @@ -89,7 +88,6 @@ public void MyMethod() [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] - [DataRow("var a = sb.Length;", false)] // FP public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) { var code = $$""" @@ -122,7 +120,6 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("""sb.Append("").ToString()""", true)] [DataRow("sb.Remove(sb.Length - 1, 1)", true)] [DataRow("""Dim a = $"{sb} is ToStringed here" """, true)] - [DataRow("Dim a = sb.Length", false)] // FP [DataRow("""SB.ToString()""", true)] public void UnusedStringBuilder_VBExpressionsTest(string expression, bool compliant) { diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index ccd1fd86469..1421eb17319 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -42,6 +42,9 @@ public class Program (StringBuilder, StringBuilder) builderTuple = (new StringBuilder(), new StringBuilder()); // FN + StringBuilder builderInLine1 = new StringBuilder(), builderInLine2 = new StringBuilder(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 + StringBuilder builderCfg = new StringBuilder(); // FN (we should use cfg with significant impact on performance) if (false) { @@ -55,6 +58,10 @@ public class Program builderCalls.Clear(); builderCalls.GetType(); + StringBuilder mySbField = new StringBuilder(); // Noncompliant + MyClass myClass = new MyClass(); + myClass.mySbField.ToString(); + var builderReturn = new StringBuilder(); // Compliant return builderReturn; @@ -87,3 +94,8 @@ public string MyProperty private StringBuilder myField = new StringBuilder(); // Compliant } + +public class MyClass +{ + public StringBuilder mySbField = new StringBuilder(); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 3ed088b1dda..8afda91f5ba 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -15,6 +15,9 @@ Public Class Program Dim builder8 As StringBuilder = New StringBuilder() ' Compliant builder8.Append("&").ToString() + Dim builderInLine1 As StringBuilder = New StringBuilder(), builderInLine2 As StringBuilder = New StringBuilder() ' Noncompliant + ' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ' Noncompliant@-1 + Dim builderCfg As StringBuilder = New StringBuilder() ' FN (we should use cfg with significant impact on performance) If False Then builderCfg.ToString() @@ -29,6 +32,10 @@ Public Class Program Dim builderCalls2 As StringBuilder = New StringBuilder() ' Compliant builderCalls2.Remove(builderCalls2.Length - 1, 1) + Dim mySbField As StringBuilder = New StringBuilder() ' Noncompliant + Dim [myClass] As [MyClass] = New [MyClass]() + [myClass].mySbField.ToString() + Dim builderReturn = New StringBuilder() ' Compliant Return builderReturn End Function @@ -51,3 +58,6 @@ Public Class Program Private myField As StringBuilder = New StringBuilder() ' Compliant End Class +Public Class [MyClass] + Public mySbField As StringBuilder = New StringBuilder() +End Class From c062371a6c4a92255863c2bc29cdc6ad984c598d Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 14:44:36 +0100 Subject: [PATCH 15/41] Add support for indexer access --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 8 ++++++++ .../SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs | 4 +++- .../Rules/UnusedStringBuilder.cs | 5 +++++ .../Rules/UnusedStringBuilderTest.cs | 6 +++--- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 8a53d1f0eb1..6bdc2c77c0c 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -60,6 +60,9 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList 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)) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); @@ -72,6 +75,11 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); + private static IList GetElementAccessExpressions(VariableDeclaratorSyntax declaration) => + declaration.IsTopLevel() + ? GetTopLevelStatementSyntax(declaration) + : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); + private static IList GetTopLevelStatementSyntax(VariableDeclaratorSyntax declaration) { List list = new(); diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index c0db0e9905e..b96c0eeaa92 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -41,6 +41,7 @@ public abstract class UnusedStringBuilderBase invocations, ISymbol variableSymbol, SemanticModel semanticModel); protected abstract bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel); protected abstract bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsPropertyReferenced(TVariableDeclarator declaration, IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); protected UnusedStringBuilderBase() : base(DiagnosticId) { } @@ -57,7 +58,8 @@ public abstract class UnusedStringBuilderBase interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && semanticModel.GetOperation(x).Kind is OperationKindEx.PropertyReference); + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index bbd9e0cf81d..30bab62e502 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -39,7 +39,7 @@ public class UnusedStringBuilderTest [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];", false)] // FP + [DataRow("var a = sb[0];", true)] [DataRow("""sb?.Append("").ToString().ToLower();""", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] @@ -83,7 +83,7 @@ public void MyMethod() [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];", false)] // FP + [DataRow("var a = sb[0];", true)] [DataRow("""sb?.Append("").ToString().ToLower();""", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] @@ -115,7 +115,7 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("""Dim a = sb.Append("").Append("").Append("").Append("").ToString().ToLower()""", true)] [DataRow("sb.CopyTo(0, New Char(0) {}, 0, 1)", true)] [DataRow("sb.GetChunks()", true)] - [DataRow("Dim a = sb(0)", false)] // FP + [DataRow("Dim a = sb(0)", true)] [DataRow("""sb?.Append("").ToString().ToLower()""", true)] [DataRow("""sb.Append("").ToString()""", true)] [DataRow("sb.Remove(sb.Length - 1, 1)", true)] From f44dc07439dcf718402a50a7e6f302394303bd2f Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 15:15:04 +0100 Subject: [PATCH 16/41] Add syntax kind --- .../src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs | 1 + analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs | 1 + .../Facade/VisualBasicSyntaxKindFacade.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs index 83954886377..c6be39961a9 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs @@ -79,5 +79,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.Common/Facade/ISyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs index 9394383d9f8..e2d0335a817 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs @@ -54,5 +54,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.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs index 2e855745d74..5be1818e6e9 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxKindFacade.cs @@ -65,5 +65,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; } From c207c32036d4b8e6b39cfdbf00de8bde99d392f2 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 15:25:52 +0100 Subject: [PATCH 17/41] Fix UTs --- .../Rules/UnusedStringBuilderTest.cs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 30bab62e502..a65f32cf96f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -38,12 +38,18 @@ public class UnusedStringBuilderTest [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();""", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] + +#if NET + + [DataRow("sb.GetChunks();", true)] + +#endif + public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) { var code = $$""" @@ -114,13 +120,19 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [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("sb.GetChunks()", true)] [DataRow("Dim a = sb(0)", true)] [DataRow("""sb?.Append("").ToString().ToLower()""", true)] [DataRow("""sb.Append("").ToString()""", true)] [DataRow("sb.Remove(sb.Length - 1, 1)", true)] [DataRow("""Dim a = $"{sb} is ToStringed here" """, true)] [DataRow("""SB.ToString()""", true)] + +#if NET + + [DataRow("sb.GetChunks()", true)] + +#endif + public void UnusedStringBuilder_VBExpressionsTest(string expression, bool compliant) { var code = $$""" From 226bfdfbc1c1643c9b49340f26e200b81d1d8588 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 17:23:08 +0100 Subject: [PATCH 18/41] Rewritten IsSameVariable to improve performance --- .../Rules/UnusedStringBuilder.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 6bdc2c77c0c..1d30bc955a7 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -67,10 +67,40 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase + expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)); + */ + + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) + { + var identifiers = expression.DescendantNodesAndSelf().OfType(); + if (!identifiers.Any() && expression.Ancestors().OfType().Any()) + { + identifiers = expression.Ancestors().OfType().First().DescendantNodesAndSelf().OfType(); + } + return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + /* + IdentifierNameSyntax identifier = null; + if (node is IdentifierNameSyntax identifierName) + { + identifier = identifierName; + } + else if (node is ConditionalAccessExpressionSyntax conditionalAccessExpression + && conditionalAccessExpression.Expression is IdentifierNameSyntax identifier2) + { + identifier = identifier2; + } + + return identifier != null && IsSameVariable(identifier, variableSymbol, semanticModel); + + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() .DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel))); + */ + + } private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); From 73ca43bfcb495fe1a98186e6d5e6602b95b789e6 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 17:24:11 +0100 Subject: [PATCH 19/41] Removed comments --- .../Rules/UnusedStringBuilder.cs | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 1d30bc955a7..64a3a64ad87 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -67,11 +67,6 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase - expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)); - */ - private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) { var identifiers = expression.DescendantNodesAndSelf().OfType(); @@ -80,26 +75,6 @@ private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variable identifiers = expression.Ancestors().OfType().First().DescendantNodesAndSelf().OfType(); } return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); - /* - IdentifierNameSyntax identifier = null; - if (node is IdentifierNameSyntax identifierName) - { - identifier = identifierName; - } - else if (node is ConditionalAccessExpressionSyntax conditionalAccessExpression - && conditionalAccessExpression.Expression is IdentifierNameSyntax identifier2) - { - identifier = identifier2; - } - - return identifier != null && IsSameVariable(identifier, variableSymbol, semanticModel); - - private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => - expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) - || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() - .DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel))); - */ - } private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => From 344e4727a765ac2b2eeb11fe833290bd92d4e789 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 7 Feb 2023 18:21:31 +0100 Subject: [PATCH 20/41] Improved perf on IsSameVariable (VB) --- .../Rules/UnusedStringBuilder.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 61208fe892d..424e615ab10 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -59,10 +59,15 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase invocations, ISymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && semanticModel.GetOperation(x).Kind is OperationKindEx.PropertyReference); - private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) => - expression.DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel)) - || (expression.Ancestors().OfType().Any() && expression.Ancestors().OfType().First() - .DescendantNodesAndSelf().OfType().Any(p => IsSameVariable(p, variableSymbol, semanticModel))); + private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) + { + var identifiers = expression.DescendantNodesAndSelf().OfType(); + if (!identifiers.Any() && expression.Ancestors().OfType().Any()) + { + identifiers = expression.Ancestors().OfType().First().DescendantNodesAndSelf().OfType(); + } + return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + } private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); From 0830d34e22703561c374a90ff97650a2f0631289 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 8 Feb 2023 11:29:06 +0100 Subject: [PATCH 21/41] Addressed PR comments --- .../Rules/UnusedStringBuilder.cs | 43 +++++++---- .../Rules/UnusedStringBuilderBase.cs | 16 ++-- .../Rules/UnusedStringBuilder.cs | 41 +++++++---- .../Rules/UnusedStringBuilderTest.cs | 73 ++++++++++--------- .../TestCases/UnusedStringBuilder.vb | 3 + 5 files changed, 104 insertions(+), 72 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 64a3a64ad87..6bf207bae54 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -25,14 +25,19 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => CSharpFacade.Instance; - protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration); + protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)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 + declaration is + { + Parent.Parent: LocalDeclarationStatementSyntax, + Initializer.Value: { } expression, + } && IsStringBuilderObjectCreation(expression, semanticModel); + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + throw new NotImplementedException(); + protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => declaration.IsTopLevel() ? GetTopLevelStatementSyntax(declaration) @@ -48,36 +53,42 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase(declaration) : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableSymbol, semanticModel))); - protected override bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel) => returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ILocalSymbol 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)) + expression.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) + private static bool IsSameVariable(ExpressionSyntax expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) { - var identifiers = expression.DescendantNodesAndSelf().OfType(); - if (!identifiers.Any() && expression.Ancestors().OfType().Any()) + var references = GetLocalReferences(expression, semanticModel); + if (!references.Any() && expression.Ancestors().OfType().Any()) { - identifiers = expression.Ancestors().OfType().First().DescendantNodesAndSelf().OfType(); + references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); } - return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); } - private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => + private static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => + node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); + + private static bool IsLocalReference(IdentifierNameSyntax identifier, SemanticModel semanticModel) => + semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; + + private static bool IsSameVariable(IdentifierNameSyntax identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); private static IList GetElementAccessExpressions(VariableDeclaratorSyntax declaration) => diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index b96c0eeaa92..8d829d88a79 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -32,16 +32,18 @@ public abstract class UnusedStringBuilderBase GetInvocations(TVariableDeclarator declaration); protected abstract IList GetReturnStatements(TVariableDeclarator declaration); protected abstract IList GetInterpolatedStrings(TVariableDeclarator declaration); - protected abstract bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsPropertyReferenced(TVariableDeclarator declaration, IList invocations, ISymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel); + protected abstract bool IsPropertyReferenced(TVariableDeclarator declaration, IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); + + protected abstract bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node); protected UnusedStringBuilderBase() : base(DiagnosticId) { } @@ -55,6 +57,8 @@ public abstract class UnusedStringBuilderBase IsStringBuilderRead(c.SemanticModel, symbol, node)); if (IsStringBuilderContentRead(invocations, variableSymbol, c.SemanticModel) || IsPassedToMethod(invocations, variableSymbol, c.SemanticModel) || IsReturned(GetReturnStatements(variableDeclaration), variableSymbol, c.SemanticModel) diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 424e615ab10..5f3a80a3439 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -27,14 +27,19 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - protected override ISymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => semanticModel.GetDeclaredSymbol(declaration.Names.First()); + protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration.Names.First()); protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => - declaration.Initializer is not null - && declaration.Parent is LocalDeclarationStatementSyntax - && declaration.Initializer.Value is ObjectCreationExpressionSyntax objectCreation + declaration is + { + Parent: LocalDeclarationStatementSyntax, + Initializer.Value: ObjectCreationExpressionSyntax { } objectCreation, + } && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + throw new NotImplementedException(); + protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); @@ -44,31 +49,37 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - protected override bool IsStringBuilderContentRead(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && StringBuilderAccessMethods.Contains(x.GetName())); - protected override bool IsPassedToMethod(IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableSymbol, semanticModel))); - protected override bool IsReturned(IList returnStatements, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel) => returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsWithinInterpolatedString(IList interpolations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ISymbol variableSymbol, SemanticModel semanticModel) => + protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && semanticModel.GetOperation(x).Kind is OperationKindEx.PropertyReference); - private static bool IsSameVariable(ExpressionSyntax expression, ISymbol variableSymbol, SemanticModel semanticModel) + private static bool IsSameVariable(ExpressionSyntax expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) { - var identifiers = expression.DescendantNodesAndSelf().OfType(); - if (!identifiers.Any() && expression.Ancestors().OfType().Any()) + var references = GetLocalReferences(expression, semanticModel); + if (!references.Any() && expression.Ancestors().OfType().Any()) { - identifiers = expression.Ancestors().OfType().First().DescendantNodesAndSelf().OfType(); + references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); } - return identifiers.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); } - private static bool IsSameVariable(IdentifierNameSyntax identifier, ISymbol variableSymbol, SemanticModel semanticModel) => + private static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => + node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); + + private static bool IsLocalReference(IdentifierNameSyntax identifier, SemanticModel semanticModel) => + semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; + + private static bool IsSameVariable(IdentifierNameSyntax identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index a65f32cf96f..79225494c42 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -33,38 +33,39 @@ public class UnusedStringBuilderTest 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();""", true)] + [DataRow("sb?.ToString().ToLower();", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] - -#if NET - - [DataRow("sb.GetChunks();", true)] - -#endif - - public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) + public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) { var code = $$""" using System.Text; - public class MyClass - { - public void MyMethod() - { - var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} - {{expression}} - } - } + var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} + {{expression}} """; - var builder = builderCS.AddSnippet(code); + var builder = builderCS.AddSnippet(code).WithTopLevelStatements(); if (compliant) { builder.VerifyNoIssueReported(); @@ -75,34 +76,41 @@ public void MyMethod() } } -#if NET - - [TestMethod] - public void UnusedStringBuilder_CSharp9() => - builderCS.AddPaths("UnusedStringBuilder.CSharp9.cs") - .WithOptions(ParseOptionsHelper.FromCSharp9) - .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("sb.GetChunks();", true)] [DataRow("var a = sb[0];", true)] [DataRow("""sb?.Append("").ToString().ToLower();""", true)] + [DataRow("sb?.ToString();", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] - public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) + +#if NET + + [DataRow("sb.GetChunks();", true)] + +#endif + + public void UnusedStringBuilder_CSExpressionsTest(string expression, bool compliant) { var code = $$""" using System.Text; - var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} - {{expression}} + public class MyClass + { + public void MyMethod() + { + var sb = new StringBuilder(); // {{(compliant ? "Compliant" : "Noncompliant")}} + {{expression}} + } + } """; - var builder = builderCS.AddSnippet(code).WithTopLevelStatements(); + var builder = builderCS.AddSnippet(code); if (compliant) { builder.VerifyNoIssueReported(); @@ -113,8 +121,6 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl } } -#endif - [DataTestMethod] [DataRow("", false)] [DataRow("sb.ToString()", true)] @@ -122,6 +128,7 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("sb.CopyTo(0, New Char(0) {}, 0, 1)", true)] [DataRow("Dim a = sb(0)", true)] [DataRow("""sb?.Append("").ToString().ToLower()""", true)] + [DataRow("sb?.ToString().ToLower()", true)] [DataRow("""sb.Append("").ToString()""", true)] [DataRow("sb.Remove(sb.Length - 1, 1)", true)] [DataRow("""Dim a = $"{sb} is ToStringed here" """, true)] @@ -155,8 +162,4 @@ End Class builder.Verify(); } } - - [TestMethod] - public void UnusedStringBuilder_VB() => - builderVB.AddPaths("UnusedStringBuilder.vb").Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 8afda91f5ba..6406c8c1bef 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -18,6 +18,9 @@ Public Class Program 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 builderCfg As StringBuilder = New StringBuilder() ' FN (we should use cfg with significant impact on performance) If False Then builderCfg.ToString() From 77a23a1d71db5b07c1d9d81ab60f6c0dadddbfd7 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 8 Feb 2023 16:17:34 +0100 Subject: [PATCH 22/41] Rule structure changes --- .../Rules/UnusedStringBuilder.cs | 86 ++++--------------- .../Rules/UnusedStringBuilderBase.cs | 55 ++++++------ .../Rules/UnusedStringBuilder.cs | 58 ++++--------- .../Rules/UnusedStringBuilderTest.cs | 9 ++ 4 files changed, 70 insertions(+), 138 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 6bf207bae54..669c5d58728 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -21,7 +21,7 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => CSharpFacade.Instance; @@ -35,77 +35,27 @@ declaration is } && IsStringBuilderObjectCreation(expression, semanticModel); - protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => - throw new NotImplementedException(); - - protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => - declaration.IsTopLevel() - ? GetTopLevelStatementSyntax(declaration) - : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override IList GetReturnStatements(VariableDeclaratorSyntax declaration) => - declaration.IsTopLevel() - ? new() - : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override IList GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => - declaration.IsTopLevel() - ? GetTopLevelStatementSyntax(declaration) - : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - invocations.Any(x => StringBuilderAccessMethods.Contains(x.Expression.GetName()) && IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => + declarator.IsTopLevel() + ? declarator.Parent.Parent.Parent.Parent + : declarator.Parent.Parent.Parent; - protected override bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.Expression, variableSymbol, semanticModel))); + protected override bool DescendIntoChildren(SyntaxNode node) => true; - protected override bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - - protected override bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); - - protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - GetElementAccessExpressions(declaration).Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + node switch + { + InvocationExpressionSyntax invocation => + (StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, local, model)) + || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.Expression, local, model)), + ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, local, model), + InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, local, model), + ElementAccessExpressionSyntax elementAccess => IsSameReference(elementAccess.Expression, local, model), + MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, local, model), + _ => false, + }; private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => expression.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - - private static bool IsSameVariable(ExpressionSyntax expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) - { - var references = GetLocalReferences(expression, semanticModel); - if (!references.Any() && expression.Ancestors().OfType().Any()) - { - references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); - } - return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); - } - - private static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => - node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); - - private static bool IsLocalReference(IdentifierNameSyntax identifier, SemanticModel semanticModel) => - semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; - - private static bool IsSameVariable(IdentifierNameSyntax identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); - - private static IList GetElementAccessExpressions(VariableDeclaratorSyntax declaration) => - declaration.IsTopLevel() - ? GetTopLevelStatementSyntax(declaration) - : declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - private static IList GetTopLevelStatementSyntax(VariableDeclaratorSyntax declaration) - { - List list = new(); - foreach (var globalStatement in declaration.Parent.Parent.Parent.Parent.DescendantNodes().OfType()) - { - foreach (var interpolation in globalStatement.DescendantNodes().OfType()) - { - list.Add(interpolation); - } - } - return list; - } } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 8d829d88a79..d13004e1ea5 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -20,30 +20,23 @@ namespace SonarAnalyzer.Rules; -public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer +public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer where TSyntaxKind : struct where TVariableDeclarator : SyntaxNode - where TInvocationExpression : SyntaxNode - where TReturnStatement : SyntaxNode - where TInterpolatedString : SyntaxNode + where TIdentifierName : SyntaxNode + where TConditionalExpression : SyntaxNode { private const string DiagnosticId = "S3063"; protected override string MessageFormat => """Remove this "StringBuilder"; ".ToString()" is never called."""; - internal readonly string[] StringBuilderAccessMethods = { "ToString", "CopyTo", "GetChunks" }; + internal readonly string[] StringBuilderAccessInvocations = { "ToString", "CopyTo", "GetChunks" }; + internal readonly string[] StringBuilderAccessExpressions = { "Length", "Capacity", "MaxCapacity" }; protected abstract ILocalSymbol GetSymbol(TVariableDeclarator declaration, SemanticModel semanticModel); protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel); - protected abstract IList GetInvocations(TVariableDeclarator declaration); - protected abstract IList GetReturnStatements(TVariableDeclarator declaration); - protected abstract IList GetInterpolatedStrings(TVariableDeclarator declaration); - protected abstract bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsPropertyReferenced(TVariableDeclarator declaration, IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel); - protected abstract bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node); + protected abstract SyntaxNode GetScope(TVariableDeclarator declarator); + protected abstract bool DescendIntoChildren(SyntaxNode node); protected UnusedStringBuilderBase() : base(DiagnosticId) { } @@ -51,22 +44,30 @@ public abstract class UnusedStringBuilderBase { var variableDeclaration = (TVariableDeclarator)c.Node; - if (!NeedsToTrack(variableDeclaration, c.SemanticModel)) - { - return; - } - var variableSymbol = GetSymbol(variableDeclaration, c.SemanticModel); - var invocations = GetInvocations(variableDeclaration); - - //var wasRead = c.Node.DescendantNodes().Any(node => IsStringBuilderRead(c.SemanticModel, symbol, node)); - 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)) + if (!NeedsToTrack(variableDeclaration, c.SemanticModel) + || GetScope(variableDeclaration).DescendantNodes(DescendIntoChildren).Any(node => IsStringBuilderRead(c.SemanticModel, GetSymbol(variableDeclaration, c.SemanticModel), node))) { return; } c.ReportIssue(Diagnostic.Create(Rule, variableDeclaration.GetLocation())); }, Language.SyntaxKind.VariableDeclarator); + + internal static bool IsSameReference(SyntaxNode expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) + { + var references = GetLocalReferences(expression, semanticModel); + if (!references.Any() && expression.Ancestors().OfType().Any()) + { + references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); + } + return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + } + + internal static bool IsSameVariable(SyntaxNode identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => + variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); + + internal static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => + node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); + + internal static bool IsLocalReference(SyntaxNode identifier, SemanticModel semanticModel) => + semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 5f3a80a3439..5fc7286d28c 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -23,7 +23,7 @@ namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { protected override ILanguageFacade Language => VisualBasicFacade.Instance; @@ -37,49 +37,21 @@ declaration is } && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => - throw new NotImplementedException(); - - protected override IList GetInvocations(VariableDeclaratorSyntax declaration) => - declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override IList GetReturnStatements(VariableDeclaratorSyntax declaration) => - declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override IList GetInterpolatedStrings(VariableDeclaratorSyntax declaration) => - declaration.Parent.Parent.Parent.DescendantNodes().OfType().ToList(); - - protected override bool IsStringBuilderContentRead(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && StringBuilderAccessMethods.Contains(x.GetName())); - - protected override bool IsPassedToMethod(IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - invocations.Any(x => x.ArgumentList.Arguments.Any(y => IsSameVariable(y.GetExpression(), variableSymbol, semanticModel))); - - protected override bool IsReturned(IList returnStatements, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - returnStatements.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => + declarator.Parent.Parent.Parent; - protected override bool IsWithinInterpolatedString(IList interpolations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - interpolations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel)); + protected override bool DescendIntoChildren(SyntaxNode node) => true; - protected override bool IsPropertyReferenced(VariableDeclaratorSyntax declaration, IList invocations, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - invocations.Any(x => IsSameVariable(x.Expression, variableSymbol, semanticModel) && semanticModel.GetOperation(x).Kind is OperationKindEx.PropertyReference); - - private static bool IsSameVariable(ExpressionSyntax expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) - { - var references = GetLocalReferences(expression, semanticModel); - if (!references.Any() && expression.Ancestors().OfType().Any()) + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + node switch { - references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); - } - return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); - } - - private static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => - node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); - - private static bool IsLocalReference(IdentifierNameSyntax identifier, SemanticModel semanticModel) => - semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; - - private static bool IsSameVariable(IdentifierNameSyntax identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); + InvocationExpressionSyntax invocation => + (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, local, model)) + || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), local, model)) + || (IsSameReference(invocation.Expression, local, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference + ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, local, model), + InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, local, model), + MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, local, model), + _ => false, + }; } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 79225494c42..88d7b10ffea 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -56,6 +56,9 @@ public class UnusedStringBuilderTest [DataRow("sb?.ToString().ToLower();", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("var a = sb.Length;", true)] + [DataRow("var a = sb.Capacity;", true)] + [DataRow("var a = sb.MaxCapacity;", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) { @@ -88,6 +91,9 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("sb?.ToString();", true)] [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] + [DataRow("var a = sb.Length;", true)] + [DataRow("var a = sb.Capacity;", true)] + [DataRow("var a = sb.MaxCapacity;", true)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] #if NET @@ -132,6 +138,9 @@ public void MyMethod() [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", true)] + [DataRow("Dim a = sb.MaxCapacity", true)] [DataRow("""SB.ToString()""", true)] #if NET From 7da66e45519013914da63b9460d77bf681a35bca Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 8 Feb 2023 17:38:38 +0100 Subject: [PATCH 23/41] Increased performance checking var names before syntax level check --- .../Rules/UnusedStringBuilder.cs | 30 ++++++++++--------- .../Rules/UnusedStringBuilderBase.cs | 15 +++++----- .../Helpers/VisualBasicSyntaxHelper.cs | 1 + .../Rules/UnusedStringBuilder.cs | 25 ++++++++-------- 4 files changed, 38 insertions(+), 33 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 669c5d58728..8b8e64c7215 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -27,6 +27,13 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration); + protected override string GetName(SyntaxNode declaration) => declaration.GetName(); + + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => + declarator.IsTopLevel() + ? declarator.Parent.Parent.Parent.Parent + : declarator.Parent.Parent.Parent; + protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration is { @@ -35,26 +42,21 @@ declaration is } && IsStringBuilderObjectCreation(expression, semanticModel); - protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => - declarator.IsTopLevel() - ? declarator.Parent.Parent.Parent.Parent - : declarator.Parent.Parent.Parent; - - protected override bool DescendIntoChildren(SyntaxNode node) => true; - - protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => node switch { InvocationExpressionSyntax invocation => - (StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, local, model)) - || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.Expression, local, model)), - ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, local, model), - InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, local, model), - ElementAccessExpressionSyntax elementAccess => IsSameReference(elementAccess.Expression, local, model), - MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, local, model), + (StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, name, symbol, model)) + || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.Expression, name, symbol, model)), + ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), + InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), + ElementAccessExpressionSyntax elementAccess => IsSameReference(elementAccess.Expression, name, symbol, model), + MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), _ => false, }; + protected override bool DescendIntoChildren(SyntaxNode node) => true; + private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => expression.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index d13004e1ea5..288d67ff948 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -33,9 +33,10 @@ public abstract class UnusedStringBuilderBase IsStringBuilderRead(c.SemanticModel, GetSymbol(variableDeclaration, c.SemanticModel), node))) + || GetScope(variableDeclaration).DescendantNodes(DescendIntoChildren).Any(node => IsStringBuilderRead(GetName(variableDeclaration), GetSymbol(variableDeclaration, c.SemanticModel), node, c.SemanticModel))) { return; } c.ReportIssue(Diagnostic.Create(Rule, variableDeclaration.GetLocation())); }, Language.SyntaxKind.VariableDeclarator); - internal static bool IsSameReference(SyntaxNode expression, ILocalSymbol variableSymbol, SemanticModel semanticModel) + internal bool IsSameReference(SyntaxNode expression, string name, ILocalSymbol symbol, SemanticModel semanticModel) { var references = GetLocalReferences(expression, semanticModel); if (!references.Any() && expression.Ancestors().OfType().Any()) { references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); } - return references.Any(x => IsSameVariable(x, variableSymbol, semanticModel)); + return references.Any(x => IsSameVariable(x, name, symbol, semanticModel)); } - internal static bool IsSameVariable(SyntaxNode identifier, ILocalSymbol variableSymbol, SemanticModel semanticModel) => - variableSymbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); + internal bool IsSameVariable(SyntaxNode identifier, string name, ILocalSymbol symbol, SemanticModel semanticModel) => + GetName(identifier).Equals(name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); internal static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs index d392971b41c..4db6a6b9851 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs @@ -179,6 +179,7 @@ private static bool IsOn(this ExpressionSyntax expression, SyntaxKind onKind) SimpleNameSyntax x => x.Identifier, StructureBlockSyntax x => x.StructureStatement.Identifier, QualifiedNameSyntax x => x.Right.Identifier, + VariableDeclaratorSyntax x => x.Names.First().GetIdentifier(), _ => null, }; diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 5fc7286d28c..90979aa8b78 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -29,6 +29,10 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration.Names.First()); + protected override string GetName(SyntaxNode declaration) => declaration.GetName(); + + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => declarator.Parent.Parent.Parent; + protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => declaration is { @@ -37,21 +41,18 @@ declaration is } && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => - declarator.Parent.Parent.Parent; - - protected override bool DescendIntoChildren(SyntaxNode node) => true; - - protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol local, SyntaxNode node) => + protected override bool IsStringBuilderRead(string name, ILocalSymbol local, SyntaxNode node, SemanticModel model) => node switch { InvocationExpressionSyntax invocation => - (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, local, model)) - || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), local, model)) - || (IsSameReference(invocation.Expression, local, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference - ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, local, model), - InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, local, model), - MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, local, model), + (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, name, local, model)) + || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), name, local, model)) + || (IsSameReference(invocation.Expression, name, local, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference + ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, local, model), + InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, local, model), + MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, local, model), _ => false, }; + + protected override bool DescendIntoChildren(SyntaxNode node) => true; } From 7c96800372a04ca37dc939de6e22c81cb295f9c1 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 8 Feb 2023 18:35:16 +0100 Subject: [PATCH 24/41] Increased perf skipping not interesting SyntaxKinds --- .../SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 9 ++++++++- .../Rules/UnusedStringBuilder.cs | 4 +++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 8b8e64c7215..94cac9112d9 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -23,6 +23,11 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class UnusedStringBuilder : UnusedStringBuilderBase { + internal readonly SyntaxKind[] SkipChildren = + { + SyntaxKind.ClassDeclaration, + }; + protected override ILanguageFacade Language => CSharpFacade.Instance; protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration); @@ -55,7 +60,9 @@ node switch _ => false, }; - protected override bool DescendIntoChildren(SyntaxNode node) => true; + 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) => expression.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 90979aa8b78..f34cf3af2ba 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -25,6 +25,8 @@ namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] public sealed class UnusedStringBuilder : UnusedStringBuilderBase { + internal readonly SyntaxKind[] SkipChildren = { }; + protected override ILanguageFacade Language => VisualBasicFacade.Instance; protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration.Names.First()); @@ -54,5 +56,5 @@ node switch _ => false, }; - protected override bool DescendIntoChildren(SyntaxNode node) => true; + protected override bool DescendIntoChildren(SyntaxNode node) => !node.IsAnyKind(SkipChildren); } From a26ea2726e48b4a63d12f5498743f5eb90751472 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 08:55:25 +0100 Subject: [PATCH 25/41] Add coverage amd fix code smell --- .../Rules/UnusedStringBuilderBase.cs | 3 ++- .../Rules/UnusedStringBuilder.cs | 14 +++++++------- .../TestCases/UnusedStringBuilder.CSharp9.cs | 6 ++++++ 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 288d67ff948..a96bb22e82a 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -46,7 +46,8 @@ public abstract class UnusedStringBuilderBase IsStringBuilderRead(GetName(variableDeclaration), GetSymbol(variableDeclaration, c.SemanticModel), node, c.SemanticModel))) + || GetScope(variableDeclaration).DescendantNodes(DescendIntoChildren).Any(node => + IsStringBuilderRead(GetName(variableDeclaration), GetSymbol(variableDeclaration, c.SemanticModel), node, c.SemanticModel))) { return; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index f34cf3af2ba..506c36eb6e3 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -43,16 +43,16 @@ declaration is } && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); - protected override bool IsStringBuilderRead(string name, ILocalSymbol local, SyntaxNode node, SemanticModel model) => + protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => node switch { InvocationExpressionSyntax invocation => - (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, name, local, model)) - || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), name, local, model)) - || (IsSameReference(invocation.Expression, name, local, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference - ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, local, model), - InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, local, model), - MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, local, model), + (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, name, symbol, model)) + || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), name, symbol, model)) + || (IsSameReference(invocation.Expression, name, symbol, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference + ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), + InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), + MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), _ => false, }; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs index 6ee1656569f..981c0ea68db 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.CSharp9.cs @@ -16,5 +16,11 @@ public class MyClass StringBuilder builderInLine1 = new(), builderInLine2 = new(); // Noncompliant // ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 + + static void LocalStaticMethod() + { + StringBuilder builder2 = new StringBuilder(); // Compliant + var a = builder2.Length; + } } } From df5dc9c4366fa776564a64c244afd19086a65178 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 11:05:40 +0100 Subject: [PATCH 26/41] ITs fix --- .../Rules/UnusedStringBuilderBase.cs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index a96bb22e82a..ef9c4013409 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -27,11 +27,12 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; internal readonly string[] StringBuilderAccessInvocations = { "ToString", "CopyTo", "GetChunks" }; internal readonly string[] StringBuilderAccessExpressions = { "Length", "Capacity", "MaxCapacity" }; + protected override string MessageFormat => """Remove this "StringBuilder"; ".ToString()" is never called."""; + protected abstract ILocalSymbol GetSymbol(TVariableDeclarator declaration, SemanticModel semanticModel); protected abstract string GetName(SyntaxNode declaration); protected abstract SyntaxNode GetScope(TVariableDeclarator declarator); @@ -56,10 +57,15 @@ public abstract class UnusedStringBuilderBase().Any()) + if (expression == null) { - references = GetLocalReferences(expression.Ancestors().OfType().First(), semanticModel); + return false; + } + + var references = GetLocalReferences(expression); + if (expression.Ancestors().OfType().Any()) + { + references = GetLocalReferences(expression.Ancestors().OfType().First()); } return references.Any(x => IsSameVariable(x, name, symbol, semanticModel)); } @@ -67,9 +73,6 @@ internal bool IsSameReference(SyntaxNode expression, string name, ILocalSymbol s internal bool IsSameVariable(SyntaxNode identifier, string name, ILocalSymbol symbol, SemanticModel semanticModel) => GetName(identifier).Equals(name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); - internal static IEnumerable GetLocalReferences(SyntaxNode node, SemanticModel semanticModel) => - node.DescendantNodesAndSelf().OfType().Where(x => IsLocalReference(x, semanticModel)); - - internal static bool IsLocalReference(SyntaxNode identifier, SemanticModel semanticModel) => - semanticModel.GetOperation(identifier) is { Kind: OperationKindEx.LocalReference }; + internal static IEnumerable GetLocalReferences(SyntaxNode node) => + node.DescendantNodesAndSelf().OfType(); } From ba3802413044cf7c2f89347898d4d8961fe2e352 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 15:41:49 +0100 Subject: [PATCH 27/41] Address PR comments --- .../VariableDeclaratorSyntaxExtensions.cs | 27 ------------------- .../Rules/UnusedStringBuilder.cs | 19 +++++++------ .../Rules/UnusedStringBuilderBase.cs | 8 +++--- .../Helpers/VisualBasicSyntaxHelper.cs | 1 - .../Rules/UnusedStringBuilder.cs | 21 ++++++++------- 5 files changed, 25 insertions(+), 51 deletions(-) delete mode 100644 analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs deleted file mode 100644 index 4af1add82cc..00000000000 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/VariableDeclaratorSyntaxExtensions.cs +++ /dev/null @@ -1,27 +0,0 @@ -/* - * 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) => - declarator.Parent.Parent is { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax } }; -} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 94cac9112d9..a8bab4b882f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -23,39 +23,38 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class UnusedStringBuilder : UnusedStringBuilderBase { - internal readonly SyntaxKind[] SkipChildren = + private static readonly SyntaxKind[] SkipChildren = { SyntaxKind.ClassDeclaration, }; protected override ILanguageFacade Language => CSharpFacade.Instance; - protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration); - protected override string GetName(SyntaxNode declaration) => declaration.GetName(); protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => - declarator.IsTopLevel() + declarator.Parent.Parent is { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax } } ? declarator.Parent.Parent.Parent.Parent : declarator.Parent.Parent.Parent; - protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => - declaration is + protected override ILocalSymbol RetrieveStringBuilderObject(VariableDeclaratorSyntax declarator, SemanticModel semanticModel) => + declarator is { Parent.Parent: LocalDeclarationStatementSyntax, Initializer.Value: { } expression, } - && IsStringBuilderObjectCreation(expression, semanticModel); + && IsStringBuilderObjectCreation(expression, semanticModel) + ? semanticModel.GetDeclaredSymbol(declarator) as ILocalSymbol + : null; protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => node switch { - InvocationExpressionSyntax invocation => - (StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, name, symbol, model)) - || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.Expression, name, symbol, model)), + InvocationExpressionSyntax invocation => StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, name, symbol, model), ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), ElementAccessExpressionSyntax elementAccess => IsSameReference(elementAccess.Expression, name, symbol, model), + ArgumentSyntax argument => IsSameReference(argument.Expression, name, symbol, model), MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), _ => false, }; diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index ef9c4013409..e982b366560 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -33,10 +33,9 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; - protected abstract ILocalSymbol GetSymbol(TVariableDeclarator declaration, SemanticModel semanticModel); protected abstract string GetName(SyntaxNode declaration); protected abstract SyntaxNode GetScope(TVariableDeclarator declarator); - protected abstract bool NeedsToTrack(TVariableDeclarator declaration, SemanticModel semanticModel); + protected abstract ILocalSymbol RetrieveStringBuilderObject(TVariableDeclarator declaration, SemanticModel semanticModel); protected abstract bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model); protected abstract bool DescendIntoChildren(SyntaxNode node); @@ -46,9 +45,10 @@ public abstract class UnusedStringBuilderBase { var variableDeclaration = (TVariableDeclarator)c.Node; - if (!NeedsToTrack(variableDeclaration, c.SemanticModel) + + if (RetrieveStringBuilderObject(variableDeclaration, c.SemanticModel) is not { } symbol || GetScope(variableDeclaration).DescendantNodes(DescendIntoChildren).Any(node => - IsStringBuilderRead(GetName(variableDeclaration), GetSymbol(variableDeclaration, c.SemanticModel), node, c.SemanticModel))) + IsStringBuilderRead(GetName(variableDeclaration), symbol, node, c.SemanticModel))) { return; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs index 4db6a6b9851..d392971b41c 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs @@ -179,7 +179,6 @@ private static bool IsOn(this ExpressionSyntax expression, SyntaxKind onKind) SimpleNameSyntax x => x.Identifier, StructureBlockSyntax x => x.StructureStatement.Identifier, QualifiedNameSyntax x => x.Right.Identifier, - VariableDeclaratorSyntax x => x.Names.First().GetIdentifier(), _ => null, }; diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 506c36eb6e3..d30127651d4 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -25,33 +25,36 @@ namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] public sealed class UnusedStringBuilder : UnusedStringBuilderBase { - internal readonly SyntaxKind[] SkipChildren = { }; + private static readonly SyntaxKind[] SkipChildren = { }; protected override ILanguageFacade Language => VisualBasicFacade.Instance; - protected override ILocalSymbol GetSymbol(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => (ILocalSymbol)semanticModel.GetDeclaredSymbol(declaration.Names.First()); - - protected override string GetName(SyntaxNode declaration) => declaration.GetName(); + protected override string GetName(SyntaxNode declaration) => + declaration is VariableDeclaratorSyntax variableDeclarator + ? variableDeclarator.Names[0].Identifier.ValueText ?? string.Empty + : declaration.GetName(); protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => declarator.Parent.Parent.Parent; - protected override bool NeedsToTrack(VariableDeclaratorSyntax declaration, SemanticModel semanticModel) => - declaration is + protected override ILocalSymbol RetrieveStringBuilderObject(VariableDeclaratorSyntax declarator, SemanticModel semanticModel) => + declarator is { Parent: LocalDeclarationStatementSyntax, Initializer.Value: ObjectCreationExpressionSyntax { } objectCreation, } - && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel); + && objectCreation.Type.IsKnownType(KnownType.System_Text_StringBuilder, semanticModel) + ? semanticModel.GetDeclaredSymbol(declarator.Names.First()) as ILocalSymbol + : null; protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => node switch { InvocationExpressionSyntax invocation => (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, name, symbol, model)) - || invocation.ArgumentList.Arguments.Any(argument => IsSameReference(argument.GetExpression(), name, symbol, model)) - || (IsSameReference(invocation.Expression, name, symbol, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), // Property reference + || (IsSameReference(invocation.Expression, name, symbol, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), + ArgumentSyntax argument => IsSameReference(argument.GetExpression(), name, symbol, model), MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), _ => false, }; From 76383c17c9e86f31ab6f17f95bd3785110e54ac9 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 15:44:44 +0100 Subject: [PATCH 28/41] Raise for .Capacity and .MaxCapacity --- .../Rules/UnusedStringBuilderBase.cs | 2 +- .../Rules/UnusedStringBuilderTest.cs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index e982b366560..89d93270947 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -29,7 +29,7 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 88d7b10ffea..2e98244a0fe 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -57,8 +57,8 @@ public class UnusedStringBuilderTest [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("var a = sb.Length;", true)] - [DataRow("var a = sb.Capacity;", true)] - [DataRow("var a = sb.MaxCapacity;", true)] + [DataRow("var a = sb.Capacity;", false)] + [DataRow("var a = sb.MaxCapacity;", false)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] public void UnusedStringBuilder_TopLevelStatements(string expression, bool compliant) { @@ -92,8 +92,8 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("var a = sb.Length;", true)] - [DataRow("var a = sb.Capacity;", true)] - [DataRow("var a = sb.MaxCapacity;", true)] + [DataRow("var a = sb.Capacity;", false)] + [DataRow("var a = sb.MaxCapacity;", false)] [DataRow("""var a = $"{sb} is ToStringed here";""", true)] #if NET @@ -139,8 +139,8 @@ public void MyMethod() [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", true)] - [DataRow("Dim a = sb.MaxCapacity", true)] + [DataRow("Dim a = sb.Capacity", false)] + [DataRow("Dim a = sb.MaxCapacity", false)] [DataRow("""SB.ToString()""", true)] #if NET From 2ac2907f53737c4d01599361fc0468acd1ffb1ea Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 16:50:06 +0100 Subject: [PATCH 29/41] Small tweaks --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 7 +++---- .../SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs | 5 +++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index a8bab4b882f..411d3ee5162 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -60,10 +60,9 @@ node switch }; protected override bool DescendIntoChildren(SyntaxNode node) => - !node.IsAnyKind(SkipChildren) - || !(node.IsKind(SyntaxKindEx.LocalFunctionStatement) && ((LocalFunctionStatementSyntaxWrapper)node).Modifiers.Any(SyntaxKind.StaticKeyword)); + !(node.IsAnyKind(SkipChildren) && node.IsKind(SyntaxKindEx.LocalFunctionStatement) && ((LocalFunctionStatementSyntaxWrapper)node).Modifiers.Any(SyntaxKind.StaticKeyword)); private static bool IsStringBuilderObjectCreation(ExpressionSyntax expression, SemanticModel semanticModel) => - expression.IsAnyKind(SyntaxKind.ObjectCreationExpression, SyntaxKindEx.ImplicitObjectCreationExpression) - && ObjectCreationFactory.Create(expression).IsKnownType(KnownType.System_Text_StringBuilder, 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; From 716a82df95633c018a64563e86ce13f7f46bffbd Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 9 Feb 2023 17:17:54 +0100 Subject: [PATCH 30/41] Removed support for Null-conditional operators ?. --- .../Rules/UnusedStringBuilder.cs | 2 +- .../Rules/UnusedStringBuilderBase.cs | 19 +++---------------- .../Rules/UnusedStringBuilder.cs | 2 +- .../Rules/UnusedStringBuilderTest.cs | 12 ++++++------ 4 files changed, 11 insertions(+), 24 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 411d3ee5162..ec61b23879f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -21,7 +21,7 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { private static readonly SyntaxKind[] SkipChildren = { diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index 89d93270947..a205d28f8d7 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -20,11 +20,10 @@ namespace SonarAnalyzer.Rules; -public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer +public abstract class UnusedStringBuilderBase : SonarDiagnosticAnalyzer where TSyntaxKind : struct where TVariableDeclarator : SyntaxNode where TIdentifierName : SyntaxNode - where TConditionalExpression : SyntaxNode { private const string DiagnosticId = "S3063"; @@ -55,20 +54,8 @@ public abstract class UnusedStringBuilderBase().Any()) - { - references = GetLocalReferences(expression.Ancestors().OfType().First()); - } - return references.Any(x => IsSameVariable(x, name, symbol, semanticModel)); - } + internal bool IsSameReference(SyntaxNode expression, string name, ILocalSymbol symbol, SemanticModel semanticModel) => + expression is not null && GetLocalReferences(expression).Any(x => IsSameVariable(x, name, symbol, semanticModel)); internal bool IsSameVariable(SyntaxNode identifier, string name, ILocalSymbol symbol, SemanticModel semanticModel) => GetName(identifier).Equals(name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index d30127651d4..37e186dd50d 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -23,7 +23,7 @@ namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] -public sealed class UnusedStringBuilder : UnusedStringBuilderBase +public sealed class UnusedStringBuilder : UnusedStringBuilderBase { private static readonly SyntaxKind[] SkipChildren = { }; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 2e98244a0fe..b54b49b703c 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -52,8 +52,8 @@ public class UnusedStringBuilderTest [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();""", true)] - [DataRow("sb?.ToString().ToLower();", true)] + [DataRow("""sb?.Append("").ToString().ToLower();""", false)] // FP + [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)] @@ -87,8 +87,8 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [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();""", true)] - [DataRow("sb?.ToString();", true)] + [DataRow("""sb?.Append("").ToString().ToLower();""", false)] // FP + [DataRow("sb?.ToString();", false)] // FP [DataRow("""@sb.Append("").ToString();""", true)] [DataRow("sb.Remove(sb.Length - 1, 1);", true)] [DataRow("var a = sb.Length;", true)] @@ -133,8 +133,8 @@ public void MyMethod() [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()""", true)] - [DataRow("sb?.ToString().ToLower()", true)] + [DataRow("""sb?.Append("").ToString().ToLower()""", false)] // FP + [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)] From 408e5fa2695b9f25c09bc34116c39ad35cc3ed71 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 16:37:38 +0100 Subject: [PATCH 31/41] Addressed more PR comments --- .../Facade/CSharpFacade.cs | 7 +++- .../Rules/UnusedStringBuilder.cs | 32 +++++++++------- .../Facade/ILanguageFacade.cs | 2 + .../Rules/UnusedStringBuilderBase.cs | 38 +++++++++++-------- .../Facade/VisualBasicFacade.cs | 6 +++ .../Rules/UnusedStringBuilder.cs | 34 +++++++++-------- .../Rules/UnusedStringBuilderTest.cs | 10 +++-- .../TestCases/UnusedStringBuilder.cs | 2 +- .../TestCases/UnusedStringBuilder.vb | 2 +- 9 files changed, 81 insertions(+), 52 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs index f05e097d57a..7948319f934 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,10 @@ 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(); + + public SyntaxToken? GetIdentifier(SyntaxNode node) => + node.GetIdentifier(); } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index ec61b23879f..3ec05b7ee75 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -26,18 +26,24 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => CSharpFacade.Instance; - protected override string GetName(SyntaxNode declaration) => declaration.GetName(); - protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => - declarator.Parent.Parent is { Parent: GlobalStatementSyntax { Parent: CompilationUnitSyntax } } - ? declarator.Parent.Parent.Parent.Parent - : declarator.Parent.Parent.Parent; + 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(VariableDeclaratorSyntax declarator, SemanticModel semanticModel) => + protected override ILocalSymbol RetrieveStringBuilderObject(SemanticModel semanticModel, VariableDeclaratorSyntax declarator) => declarator is { Parent.Parent: LocalDeclarationStatementSyntax, @@ -47,15 +53,15 @@ declarator is ? semanticModel.GetDeclaredSymbol(declarator) as ILocalSymbol : null; - protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol symbol, SyntaxNode node) => node switch { - InvocationExpressionSyntax invocation => StringBuilderAccessInvocations.Contains(invocation.Expression.GetName()) && IsSameReference(invocation.Expression, name, symbol, model), - ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), - InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), - ElementAccessExpressionSyntax elementAccess => IsSameReference(elementAccess.Expression, name, symbol, model), - ArgumentSyntax argument => IsSameReference(argument.Expression, name, symbol, model), - MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), + 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), _ => false, }; diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs index 8fd28a001be..d1875d3cb5b 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs @@ -33,6 +33,8 @@ 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); + SyntaxToken? GetIdentifier(SyntaxNode node); } public interface ILanguageFacade : ILanguageFacade diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index a205d28f8d7..ba0848a5f2b 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -27,15 +27,14 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; - protected abstract string GetName(SyntaxNode declaration); protected abstract SyntaxNode GetScope(TVariableDeclarator declarator); - protected abstract ILocalSymbol RetrieveStringBuilderObject(TVariableDeclarator declaration, SemanticModel semanticModel); - protected abstract bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model); + 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) { } @@ -43,23 +42,30 @@ public abstract class UnusedStringBuilderBase context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c => { - var variableDeclaration = (TVariableDeclarator)c.Node; + var variableDeclarator = (TVariableDeclarator)c.Node; - if (RetrieveStringBuilderObject(variableDeclaration, c.SemanticModel) is not { } symbol - || GetScope(variableDeclaration).DescendantNodes(DescendIntoChildren).Any(node => - IsStringBuilderRead(GetName(variableDeclaration), symbol, node, c.SemanticModel))) + if (RetrieveStringBuilderObject(c.SemanticModel, variableDeclarator) is { } symbol + && GetScope(variableDeclarator) is { } scope + && !IsStringBuilderAccessed(c.SemanticModel, symbol, scope)) { - return; + c.ReportIssue(Diagnostic.Create(Rule, variableDeclarator.GetLocation())); } - c.ReportIssue(Diagnostic.Create(Rule, variableDeclaration.GetLocation())); }, Language.SyntaxKind.VariableDeclarator); - internal bool IsSameReference(SyntaxNode expression, string name, ILocalSymbol symbol, SemanticModel semanticModel) => - expression is not null && GetLocalReferences(expression).Any(x => IsSameVariable(x, name, symbol, semanticModel)); + internal bool IsSameReference(SemanticModel semanticModel, ILocalSymbol symbol, SyntaxNode expression) => + expression is not null && GetLocalReferences(expression).Any(x => IsSameVariable(semanticModel, symbol, x)); - internal bool IsSameVariable(SyntaxNode identifier, string name, ILocalSymbol symbol, SemanticModel semanticModel) => - GetName(identifier).Equals(name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); + private bool IsSameVariable(SemanticModel semanticModel, ILocalSymbol symbol, SyntaxNode identifier) => + Language.GetName(identifier).Equals(symbol.Name, Language.NameComparison) && symbol.Equals(semanticModel.GetSymbolInfo(identifier).Symbol); - internal static IEnumerable GetLocalReferences(SyntaxNode node) => + 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..00572aa4faf 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs @@ -58,4 +58,10 @@ 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(); + + public SyntaxToken? GetIdentifier(SyntaxNode node) => + node.GetIdentifier(); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 37e186dd50d..30388208b06 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -29,35 +29,37 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - protected override string GetName(SyntaxNode declaration) => - declaration is VariableDeclaratorSyntax variableDeclarator - ? variableDeclarator.Names[0].Identifier.ValueText ?? string.Empty - : declaration.GetName(); - - protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => declarator.Parent.Parent.Parent; + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => // declarator.Parent.Parent.Parent; + declarator switch + { + { Parent: LocalDeclarationStatementSyntax { Parent: { } block } } => block, + _ => null, + }; - protected override ILocalSymbol RetrieveStringBuilderObject(VariableDeclaratorSyntax declarator, SemanticModel semanticModel) => + 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(declarator.Names.First()) as ILocalSymbol + ? semanticModel.GetDeclaredSymbol(names[0]) as ILocalSymbol : null; - protected override bool IsStringBuilderRead(string name, ILocalSymbol symbol, SyntaxNode node, SemanticModel model) => + protected override bool IsStringBuilderRead(SemanticModel model, ILocalSymbol symbol, SyntaxNode node) => node switch { InvocationExpressionSyntax invocation => - (StringBuilderAccessInvocations.Contains(invocation.GetName()) && IsSameReference(invocation.Expression, name, symbol, model)) - || (IsSameReference(invocation.Expression, name, symbol, model) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), - ReturnStatementSyntax returnStatement => IsSameReference(returnStatement.Expression, name, symbol, model), - InterpolationSyntax interpolation => IsSameReference(interpolation.Expression, name, symbol, model), - ArgumentSyntax argument => IsSameReference(argument.GetExpression(), name, symbol, model), - MemberAccessExpressionSyntax memberAccess => StringBuilderAccessExpressions.Contains(memberAccess.Name.GetName()) && IsSameReference(memberAccess.Expression, name, symbol, model), + (IsAccessInvocation(invocation.GetName()) && IsSameReference(model, symbol, invocation.Expression)) + || (IsSameReference(model, symbol, invocation.Expression) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), + 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), _ => false, }; - protected override bool DescendIntoChildren(SyntaxNode node) => !node.IsAnyKind(SkipChildren); + protected override bool DescendIntoChildren(SyntaxNode node) => + !node.IsAnyKind(SkipChildren); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index b54b49b703c..300b545e96f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -52,7 +52,7 @@ public class UnusedStringBuilderTest [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 + [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)] @@ -87,7 +87,7 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [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 + [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)] @@ -133,7 +133,7 @@ public void MyMethod() [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 + [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)] @@ -141,7 +141,9 @@ public void MyMethod() [DataRow("Dim a = sb.Length", true)] [DataRow("Dim a = sb.Capacity", false)] [DataRow("Dim a = sb.MaxCapacity", false)] - [DataRow("""SB.ToString()""", true)] + [DataRow("Dim a = SB.ToString()", true)] + [DataRow("Dim a = sb.TOSTRING()", true)] + [DataRow("Dim a = sb.LENGTH", true)] #if NET diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 1421eb17319..bf67ae991a5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -45,7 +45,7 @@ public class Program StringBuilder builderInLine1 = new StringBuilder(), builderInLine2 = new StringBuilder(); // Noncompliant // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ // Noncompliant@-1 - StringBuilder builderCfg = new StringBuilder(); // FN (we should use cfg with significant impact on performance) + StringBuilder builderCfg = new StringBuilder(); // FN (requires use of cfg with significant impact on performance) if (false) { builderCfg.ToString(); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 6406c8c1bef..26a36fd6db1 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -21,7 +21,7 @@ Public Class Program 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 builderCfg As StringBuilder = New StringBuilder() ' FN (we should use cfg with significant impact on performance) + Dim builderCfg As StringBuilder = New StringBuilder() ' FN (requires use of cfg with significant impact on performance) If False Then builderCfg.ToString() End If From 5ba0b5308a57520b82d1bdf50f4bd36c94f97b09 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 16:48:03 +0100 Subject: [PATCH 32/41] TryCreate code clean up --- .../SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs | 3 +-- .../Rules/Hotspots/LooseFilePermissions.cs | 2 +- .../SonarAnalyzer.CSharp/Rules/MemberInitializedToDefault.cs | 3 +-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs index 4e95d59b98a..d3d65fce400 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 { } objectCreation => new CSharpMethodParameterLookup(objectCreation.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) { From 232c2162ea73fee0f1ac589a104d76f4e8c5b22c Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 17:12:14 +0100 Subject: [PATCH 33/41] Added support for assignments --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 2 ++ .../SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs | 2 ++ .../SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs | 3 +++ .../SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs | 4 ++++ .../SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb | 4 ++++ 5 files changed, 15 insertions(+) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 3ec05b7ee75..2c8adbb46f4 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -62,6 +62,8 @@ node switch 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), + EqualsValueClauseSyntax { Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + AssignmentExpressionSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), _ => false, }; diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index 30388208b06..ccf5620f914 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -57,6 +57,8 @@ node switch 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), + EqualsValueSyntax { Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + AssignmentStatementSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), _ => false, }; diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs index 300b545e96f..cb2a69612c8 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs @@ -60,6 +60,7 @@ public class UnusedStringBuilderTest [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 = $$""" @@ -95,6 +96,7 @@ public void UnusedStringBuilder_TopLevelStatements(string expression, bool compl [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 @@ -144,6 +146,7 @@ public void MyMethod() [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 diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index bf67ae991a5..52f29e8a9a6 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -23,6 +23,10 @@ public class Program StringBuilder sb7 = new StringBuilder(); // Noncompliant sb7.Append(sb4.ToString()); + StringBuilder sb8 = new StringBuilder(); // Compliant + StringBuilder sb9 = new StringBuilder(); // Noncompliant + sb9 = sb8; + if (true) { if (true) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb index 26a36fd6db1..b8b636d71a8 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb @@ -21,6 +21,10 @@ Public Class Program 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() From add749baffe69a8ae6650b028b201a9c40dc79ac Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 17:24:30 +0100 Subject: [PATCH 34/41] Added FP --- .../TestCases/UnusedStringBuilder.cs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 52f29e8a9a6..4909aba2b85 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -1,5 +1,4 @@ -using System.Collections.Generic; -using System.Text; +using System.Text; public class Program { @@ -27,6 +26,9 @@ public class Program StringBuilder sb9 = new StringBuilder(); // Noncompliant sb9 = sb8; + StringBuilder sb10 = new StringBuilder(); // Noncompliant FP + sb10.ToStringAndFree(); + if (true) { if (true) @@ -86,6 +88,17 @@ public string AnotherMethod() return $"{builder} is ToStringed here"; } + public void Scoping() + { + { + var sb = new StringBuilder(); // Noncompliant + } + { + var sb = new StringBuilder(); + sb.ToString(); + } + } + public string MyProperty { get @@ -103,3 +116,8 @@ public class MyClass { public StringBuilder mySbField = new StringBuilder(); } + +public static class StringBuilderExtensions +{ + public static string ToStringAndFree(this StringBuilder sb) => string.Empty; +} From 424f74329c900ab9bbba77ac1fcd4c0816ffa9a8 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 17:29:50 +0100 Subject: [PATCH 35/41] Add link to the issue --- .../SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs index 4909aba2b85..14c4cd04369 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs @@ -26,7 +26,7 @@ public class Program StringBuilder sb9 = new StringBuilder(); // Noncompliant sb9 = sb8; - StringBuilder sb10 = new StringBuilder(); // Noncompliant FP + StringBuilder sb10 = new StringBuilder(); // Noncompliant FP see https://github.com/SonarSource/sonar-dotnet/issues/6747 sb10.ToStringAndFree(); if (true) From 95e9522f4e4e5332f9e6862a2bc89d0afa67f685 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Fri, 10 Feb 2023 17:55:43 +0100 Subject: [PATCH 36/41] Fix code smell --- .../Rules/UnusedStringBuilderBase.cs | 8 ++++---- .../Rules/UnusedStringBuilder.cs | 10 ++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index ba0848a5f2b..a6934961a03 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -27,8 +27,8 @@ public abstract class UnusedStringBuilderBase """Remove this "StringBuilder"; ".ToString()" is never called."""; @@ -65,7 +65,7 @@ public abstract class UnusedStringBuilderBase IsStringBuilderRead(model, symbol, node)); internal bool IsAccessInvocation(string invocation) => - StringBuilderAccessInvocations.Any(x => x.Equals(invocation, Language.NameComparison)); + stringBuilderAccessInvocations.Any(x => x.Equals(invocation, Language.NameComparison)); internal bool IsAccessExpression(string expression) => - StringBuilderAccessExpressions.Any(x => x.Equals(expression, Language.NameComparison)); + stringBuilderAccessExpressions.Any(x => x.Equals(expression, Language.NameComparison)); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index ccf5620f914..a87d67bc90a 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -29,12 +29,10 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase Language => VisualBasicFacade.Instance; - protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => // declarator.Parent.Parent.Parent; - declarator switch - { - { Parent: LocalDeclarationStatementSyntax { Parent: { } block } } => block, - _ => null, - }; + protected override SyntaxNode GetScope(VariableDeclaratorSyntax declarator) => + declarator is { Parent: LocalDeclarationStatementSyntax { Parent: { } block } } + ? block + : null; protected override ILocalSymbol RetrieveStringBuilderObject(SemanticModel semanticModel, VariableDeclaratorSyntax declarator) => declarator is From a6b6c98b482cc0975e997a3ddc3700e8777b8e6b Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 13 Feb 2023 11:49:37 +0100 Subject: [PATCH 37/41] Address review comments --- .../SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs | 2 +- .../src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs index d3d65fce400..2a39598e0eb 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs @@ -40,7 +40,7 @@ public sealed class CertificateValidationCheck : CertificateValidationCheckBase< argumentListNode switch { InvocationExpressionSyntax invocation => new CSharpMethodParameterLookup(invocation.ArgumentList, method), - _ when ObjectCreationFactory.TryCreate(argumentListNode) is { } objectCreation => new CSharpMethodParameterLookup(objectCreation.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.Common/Rules/UnusedStringBuilderBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs index a6934961a03..8245a808cdf 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs @@ -66,6 +66,7 @@ public abstract class UnusedStringBuilderBase stringBuilderAccessInvocations.Any(x => x.Equals(invocation, Language.NameComparison)); + internal bool IsAccessExpression(string expression) => stringBuilderAccessExpressions.Any(x => x.Equals(expression, Language.NameComparison)); } From 498e6f0647a2dd9c0b3685c3da39ffd02bf073d1 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 13 Feb 2023 16:51:05 +0100 Subject: [PATCH 38/41] More comments --- .../src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 3 ++- .../SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index 2c8adbb46f4..b4dcca92daf 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -29,6 +29,7 @@ public sealed class UnusedStringBuilder : UnusedStringBuilderBase IsSameReference(model, symbol, elementAccess.Expression), ArgumentSyntax argument => IsSameReference(model, symbol, argument.Expression), MemberAccessExpressionSyntax memberAccess => IsAccessExpression(memberAccess.Name.GetName()) && IsSameReference(model, symbol, memberAccess.Expression), - EqualsValueClauseSyntax { Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + VariableDeclaratorSyntax { Initializer.Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), AssignmentExpressionSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), _ => false, }; diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs index a87d67bc90a..7da4aef250c 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs @@ -18,8 +18,6 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -using StyleCop.Analyzers.Lightup; - namespace SonarAnalyzer.Rules.VisualBasic; [DiagnosticAnalyzer(LanguageNames.VisualBasic)] @@ -50,12 +48,12 @@ node switch { InvocationExpressionSyntax invocation => (IsAccessInvocation(invocation.GetName()) && IsSameReference(model, symbol, invocation.Expression)) - || (IsSameReference(model, symbol, invocation.Expression) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), + || (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), - EqualsValueSyntax { Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), + VariableDeclaratorSyntax { Initializer.Value: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), AssignmentStatementSyntax { Right: IdentifierNameSyntax identifier } => IsSameReference(model, symbol, identifier), _ => false, }; From 0d2733e6e3793444c1c8f4946eed71113bcec537 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Mon, 13 Feb 2023 18:34:50 +0100 Subject: [PATCH 39/41] && to || --- analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs index b4dcca92daf..d550b991293 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs @@ -69,7 +69,7 @@ node switch }; protected override bool DescendIntoChildren(SyntaxNode node) => - !(node.IsAnyKind(SkipChildren) && node.IsKind(SyntaxKindEx.LocalFunctionStatement) && ((LocalFunctionStatementSyntaxWrapper)node).Modifiers.Any(SyntaxKind.StaticKeyword)); + !(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 From aaa0754aa750ddb14b946f0489b30b8f8c909e0e Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 14 Feb 2023 09:35:44 +0100 Subject: [PATCH 40/41] Remove GetIdentifier from the Facade (not used in the current rule implementation) --- analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs | 3 --- .../src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs | 3 --- 2 files changed, 6 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs index 7948319f934..c0ecf611a64 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs @@ -63,7 +63,4 @@ invocation switch public string GetName(SyntaxNode expression) => expression.GetName(); - - public SyntaxToken? GetIdentifier(SyntaxNode node) => - node.GetIdentifier(); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs index 00572aa4faf..83d6afc1fab 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs @@ -61,7 +61,4 @@ invocation switch public string GetName(SyntaxNode expression) => expression.GetName(); - - public SyntaxToken? GetIdentifier(SyntaxNode node) => - node.GetIdentifier(); } From d60927cc4cfc995fb85ddf4b58ffdcc8cbd4cb9a Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Tue, 14 Feb 2023 09:46:57 +0100 Subject: [PATCH 41/41] Removed from the interface --- analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs index d1875d3cb5b..4c675baa6d5 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs @@ -34,7 +34,6 @@ public interface ILanguageFacade object FindConstantValue(SemanticModel model, SyntaxNode node); IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol); string GetName(SyntaxNode expression); - SyntaxToken? GetIdentifier(SyntaxNode node); } public interface ILanguageFacade : ILanguageFacade