-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New rule S3063: "StringBuilder" data should be used #6696
Conversation
36749e8
to
063dfb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of comments.
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
7257579
to
c062371
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedStringBuilderTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/UnusedStringBuilderBase.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedStringBuilder.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cosmetic changes are left and there are some cases which need clarification.
analyzers/src/SonarAnalyzer.CSharp/Rules/CertificateValidationCheck.cs
Outdated
Show resolved
Hide resolved
{ | ||
InvocationExpressionSyntax invocation => | ||
(IsAccessInvocation(invocation.GetName()) && IsSameReference(model, symbol, invocation.Expression)) | ||
|| (IsSameReference(model, symbol, invocation.Expression) && model.GetOperation(invocation).Kind is OperationKindEx.PropertyReference), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! Nice find 👍
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Educational: Can you give me an example for EqualsValueClauseSyntax (VB too), please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
I will add coverage for the GetIdentifier method in the facade on a separate PR. |
}; | ||
|
||
protected override bool DescendIntoChildren(SyntaxNode node) => | ||
!(node.IsAnyKind(SkipChildren) && node.IsKind(SyntaxKindEx.LocalFunctionStatement) && ((LocalFunctionStatementSyntaxWrapper)node).Modifiers.Any(SyntaxKind.StaticKeyword)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you increase the code coverage here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
SyntaxKind.StructDeclaration, | ||
SyntaxKind.EnumDeclaration, | ||
SyntaxKind.InterfaceDeclaration, | ||
SyntaxKind.UsingDirective, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of top-level statements we are not going through the children of the using directives (see here).
- The test for top-level statements is called UnusedStringBuilder_TopLevelStatements and it's a parameterized test.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Fixes #6659