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

cristian-ambrosini-sonarsource · 2023-02-01T13:54:12Z

Fixes #6659

martin-strecker-sonarsource

First round of comments.

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

analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs

martin-strecker-sonarsource

Cool stuff, great test coverage, and a really nice find with the interpolation 👍

We need to find a better way to detect the reads but you can read more about this below.

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

analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs

analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.vb

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

analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs

analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs

martin-strecker-sonarsource

I found a couple of things that could be improved but nothing is blocking the rule from being merged.

Feel free to prioritize and cut branches by creating tickets or leaving a comment. I'm on a hardening sprint with Tim next week and we can pick up from there.

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

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

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs

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

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

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs

martin-strecker-sonarsource

Some cosmetic changes are left and there are some cases which need clarification.

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

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

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

martin-strecker-sonarsource · 2023-02-13T08:44:03Z

analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs

+        {
+            InvocationExpressionSyntax invocation =>
+                (IsAccessInvocation(invocation.GetName()) && IsSameReference(model, symbol, invocation.Expression))
+                || (IsSameReference(model, symbol, invocation.Expression) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference),


The or part looks wrong to me. Can it be removed without breaking things?

If not make the "or" part it's own pattern matching branch (this reads better IMO).

Try to get rid of the model.GetOperation call if possible.

This line is meant to support indexers:
Dim a = sb(0)

Unfortunately, I wasn't able to find any other way to detect a property reference in VB other than looking at the operation kind. In CS indexers are treated differently and I can register for them with ElementAccessExpressionSyntax, thus avoiding the check on the operation.

You are right about the syntax check: There is no way to detect whether the invocation is a method or an indexer. You can use IPropertySymbol.IsIndexer instead of IOperation though.

You are right! Nice find 👍

martin-strecker-sonarsource · 2023-02-13T08:51:12Z

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

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


Educational: Can you give me an example for EqualsValueClauseSyntax (VB too), please?

Sure!
Here for CS and here for VB.

I see. There is propably no problem with this but there other places the syntax is used (VB and CS):

EnumMemberDeclarationSyntax (Initilaizer)

PropertyDeclarationSyntax (Initilaizer)

ParameterSyntax (Default)

You better check for { Parent: VariableDeclaratorSyntax } or even better VariableDeclaratorSyntax { Initializer.Value: { .. } }

(The only way I'm aware of to check for something like this is to checkout Roslyn and use "FindAllReferences" on the SyntaxNode.)

cristian-ambrosini-sonarsource · 2023-02-13T11:16:01Z

I will add coverage for the GetIdentifier method in the facade on a separate PR.

martin-strecker-sonarsource · 2023-02-13T14:27:54Z

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

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


Can you increase the code coverage here?

martin-strecker-sonarsource

LGTM. There is one fishy looking syntax kind. Also if you have time

Increase the coverage a bit so we can merge without admin approval
There seems to be no test for top-level statements

martin-strecker-sonarsource · 2023-02-14T07:51:10Z

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

+        SyntaxKind.StructDeclaration,
+        SyntaxKind.EnumDeclaration,
+        SyntaxKind.InterfaceDeclaration,
+        SyntaxKind.UsingDirective,


Why is this?

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

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

…plementation)

sonarcloud · 2023-02-14T08:56:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-02-14T09:00:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

95.9% Coverage
0.0% Duplication

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 2, 2023 14:15

cristian-ambrosini-sonarsource marked this pull request as ready for review February 2, 2023 14:16

cristian-ambrosini-sonarsource force-pushed the cristian/S3063 branch from 36749e8 to 063dfb2 Compare February 2, 2023 15:40

martin-strecker-sonarsource requested changes Feb 2, 2023

View reviewed changes

CristianAmbrosini added 15 commits February 7, 2023 14:46

Base

c99b62c

Add test cases

08f0ba3

First implementation CS/VB

b7d0d30

Fix UTs

e487913

Fixed code smells + added test cases

7ddcd31

More test cases

1dee716

Fixed CS test cases

b4c1029

Fixed VB test cases

97a2421

Semplifications

d2554e6

Add support for TopLevelStatements and StringBuilder access methods

740a16e

Add support for null-conditional operator

85e22ee

Add support for interpolated strings and VB null conditional operators

e6ce8bd

Avoid raising for field declarations

59f1967

Replaced name comparison with semantic model symbol checks

f0003ba

Add support for indexer access

c062371

cristian-ambrosini-sonarsource force-pushed the cristian/S3063 branch from 7257579 to c062371 Compare February 7, 2023 14:03

CristianAmbrosini added 5 commits February 7, 2023 15:15

Add syntax kind

f44dc07

Fix UTs

c207c32

Rewritten IsSameVariable to improve performance

226bfdf

Removed comments

73ca43b

Improved perf on IsSameVariable (VB)

344e472

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 7, 2023 17:21

martin-strecker-sonarsource requested changes Feb 7, 2023

View reviewed changes

CristianAmbrosini added 3 commits February 8, 2023 11:29

Addressed PR comments

0830d34

Rule structure changes

77a23a1

Increased performance checking var names before syntax level check

7da66e4

CristianAmbrosini added 5 commits February 9, 2023 11:05

ITs fix

df5dc9c

Address PR comments

ba38024

Raise for .Capacity and .MaxCapacity

76383c1

Small tweaks

2ac2907

Removed support for Null-conditional operators ?.

716a82d

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 9, 2023 16:18

cristian-ambrosini-sonarsource commented Feb 9, 2023

View reviewed changes

analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs Outdated Show resolved Hide resolved

martin-strecker-sonarsource requested changes Feb 10, 2023

View reviewed changes

CristianAmbrosini added 4 commits February 10, 2023 16:37

Addressed more PR comments

408e5fa

TryCreate code clean up

5ba0b53

Added support for assignments

232c216

Added FP

add749b

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 10, 2023 16:24

CristianAmbrosini added 2 commits February 10, 2023 17:29

Add link to the issue

424f743

Fix code smell

95e9522

martin-strecker-sonarsource requested changes Feb 13, 2023

View reviewed changes

Address review comments

a6b6c98

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 13, 2023 10:50

martin-strecker-sonarsource reviewed Feb 13, 2023

View reviewed changes

More comments

498e6f0

cristian-ambrosini-sonarsource requested a review from martin-strecker-sonarsource February 13, 2023 15:52

&& to ||

0d2733e

martin-strecker-sonarsource approved these changes Feb 14, 2023

View reviewed changes

CristianAmbrosini added 2 commits February 14, 2023 09:35

Remove GetIdentifier from the Facade (not used in the current rule im…

aaa0754

…plementation)

Removed from the interface

d60927c

cristian-ambrosini-sonarsource merged commit a2d988a into master Feb 14, 2023

cristian-ambrosini-sonarsource deleted the cristian/S3063 branch February 14, 2023 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cristian-ambrosini-sonarsource commented Feb 1, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Feb 13, 2023

cristian-ambrosini-sonarsource Feb 13, 2023

martin-strecker-sonarsource Feb 13, 2023

cristian-ambrosini-sonarsource Feb 13, 2023

martin-strecker-sonarsource Feb 13, 2023

cristian-ambrosini-sonarsource Feb 13, 2023

martin-strecker-sonarsource Feb 13, 2023 •

edited

cristian-ambrosini-sonarsource commented Feb 13, 2023

martin-strecker-sonarsource Feb 13, 2023

martin-strecker-sonarsource left a comment

martin-strecker-sonarsource Feb 14, 2023

cristian-ambrosini-sonarsource Feb 14, 2023

sonarcloud bot commented Feb 14, 2023

sonarcloud bot commented Feb 14, 2023

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

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

Conversation

cristian-ambrosini-sonarsource commented Feb 1, 2023

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-strecker-sonarsource Feb 13, 2023 • edited

Choose a reason for hiding this comment

cristian-ambrosini-sonarsource commented Feb 13, 2023

Choose a reason for hiding this comment

martin-strecker-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 14, 2023

sonarcloud bot commented Feb 14, 2023

martin-strecker-sonarsource Feb 13, 2023 •

edited