-
Notifications
You must be signed in to change notification settings - Fork 470
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
Add CA1868: Unnecessary call to Set.Contains(item)
#6767
Merged
buyaa-n
merged 46 commits into
dotnet:main
from
mpidash:issue-85490-analyzer-unnecessary-set-call
Jul 28, 2023
Merged
Changes from 37 commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
d5e2607
Add analyzer and fixer for CA1865
mpidash 4d10f23
Merge branch 'dotnet:main' into issue-85490-analyzer-unnecessary-set-…
mpidash 5c6bcdb
Add CA1865 to Microsoft.CodeAnalysis.NetAnalyzers.md
mpidash 9547162
Add CA1865 to RulesMissingDocumentation.md
mpidash 52e7f91
Add changes from msbuild pack
mpidash fea7a0a
Update DoNotGuardSetAddOrRemoveByContainsDescription
mpidash 02fc236
Add ExportCodeFixProviderAttribute
mpidash 68c210f
Remove call to First with call to indexer
mpidash 5a0113f
Change equivalenceKey of CodeAction
mpidash 5e841a6
Check for interface implementation instead of signature match
mpidash 07943c4
Revert "Remove call to First with call to indexer"
mpidash 9b43e00
Remove call to First with call to indexer
mpidash a8e4875
Add additional tests
mpidash 29a4630
Fix ifs with additional statements
mpidash 468274e
Fix single line ifs in VB
mpidash 326481c
Add fixer case for single line ifs in VB
mpidash 533cd78
Apply suggestions from code review
mpidash cf49578
CA1865: Add tests for other set types
mpidash d063408
CA1865: Check descendants instead of switch
mpidash d7c7c54
CA1865: Do not fire when arguments are different
mpidash 4c71d4c
CA1865: Change fixer title
mpidash ed09398
CA1865: Update resources
mpidash 4f62beb
WIP: Try to handle IImmutableSet Add and Remove
mpidash 315e2c5
Apply suggestions from code review
mpidash a0f8d26
CA1865: Use original definitions to filter methods
mpidash f430939
Merge branch 'main' into issue-85490-analyzer-unnecessary-set-call
mpidash f22e2dc
Check parameter length before using them
mpidash efe2993
Only check child operations instead of descendants
mpidash 7c68257
Fix diagnostic message
mpidash 860f217
Also flag when interface types are used
mpidash 8b7a007
Use helper class for required symbols
mpidash 1facbcf
Merge branch 'dotnet:main' into issue-85490-analyzer-unnecessary-set-…
mpidash ba59f85
Add changes from msbuild pack
mpidash 9513d9d
Only compare first argument value
mpidash 8a3f6ef
Make symbol display format static
mpidash be9d96e
Typo
mpidash 5fef8d0
Update DoNotGuardSetAddOrRemoveByContainsTitle
mpidash 1d220c6
Change condition to pattern matching
mpidash c5376f3
Add changes from msbuild pack
mpidash 7a9fa60
Add tests for ternary operators
mpidash 6f69703
Use raw strings and inline class and usings
mpidash ad82982
Support Add or Remove in else block
mpidash f723ac5
Support ternary operator (diagnostic only)
mpidash 915a4ad
Move from CA1865 to CA1868
mpidash 8866f5d
Merge branch 'main' into issue-85490-analyzer-unnecessary-set-call
mpidash 953a052
Update src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Perform…
buyaa-n File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
79 changes: 79 additions & 0 deletions
79
...Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Composition; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.Editing; | ||
using Microsoft.CodeAnalysis.Formatting; | ||
using Microsoft.NetCore.Analyzers.Performance; | ||
|
||
namespace Microsoft.NetCore.CSharp.Analyzers.Performance | ||
{ | ||
[ExportCodeFixProvider(LanguageNames.CSharp), Shared] | ||
public sealed class CSharpDoNotGuardSetAddOrRemoveByContainsFixer : DoNotGuardSetAddOrRemoveByContainsFixer | ||
{ | ||
protected override bool SyntaxSupportedByFixer(SyntaxNode conditionalSyntax, SyntaxNode childStatementSyntax) | ||
{ | ||
if (childStatementSyntax is not ExpressionStatementSyntax) | ||
{ | ||
return false; | ||
} | ||
|
||
if (conditionalSyntax is IfStatementSyntax ifStatementSyntax) | ||
{ | ||
return ifStatementSyntax.Statement.ChildNodes().Count() == 1; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
protected override Document ReplaceConditionWithChild(Document document, SyntaxNode root, SyntaxNode conditionalOperationNode, SyntaxNode childOperationNode) | ||
{ | ||
SyntaxNode newRoot; | ||
|
||
if (conditionalOperationNode is ConditionalExpressionSyntax conditionalExpressionSyntax && | ||
conditionalExpressionSyntax.WhenFalse.ChildNodes().Any()) | ||
{ | ||
var expression = GetNegatedExpression(document, childOperationNode); | ||
|
||
SyntaxNode newConditionalOperationNode = conditionalExpressionSyntax | ||
.WithCondition((ExpressionSyntax)expression) | ||
.WithWhenTrue(conditionalExpressionSyntax.WhenFalse) | ||
.WithWhenFalse(null!) | ||
.WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode); | ||
|
||
newRoot = root.ReplaceNode(conditionalOperationNode, newConditionalOperationNode); | ||
} | ||
else if (conditionalOperationNode is IfStatementSyntax ifStatementSyntax && ifStatementSyntax.Else != null) | ||
mpidash marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
var expression = GetNegatedExpression(document, childOperationNode); | ||
|
||
SyntaxNode newConditionalOperationNode = ifStatementSyntax | ||
.WithCondition((ExpressionSyntax)expression) | ||
.WithStatement(ifStatementSyntax.Else.Statement) | ||
.WithElse(null) | ||
.WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode); | ||
|
||
newRoot = root.ReplaceNode(conditionalOperationNode, newConditionalOperationNode); | ||
} | ||
else | ||
{ | ||
SyntaxNode newConditionNode = childOperationNode | ||
.WithAdditionalAnnotations(Formatter.Annotation) | ||
.WithTriviaFrom(conditionalOperationNode); | ||
|
||
newRoot = root.ReplaceNode(conditionalOperationNode, newConditionNode); | ||
} | ||
|
||
return document.WithSyntaxRoot(newRoot); | ||
} | ||
|
||
private static SyntaxNode GetNegatedExpression(Document document, SyntaxNode newConditionNode) | ||
{ | ||
var generator = SyntaxGenerator.GetGenerator(document); | ||
return generator.LogicalNotExpression(((ExpressionStatementSyntax)newConditionNode).Expression.WithoutTrivia()); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61 changes: 61 additions & 0 deletions
61
.../Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.Fixer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Immutable; | ||
using System.Threading.Tasks; | ||
using Analyzer.Utilities; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CodeActions; | ||
using Microsoft.CodeAnalysis.CodeFixes; | ||
|
||
namespace Microsoft.NetCore.Analyzers.Performance | ||
{ | ||
public abstract class DoNotGuardSetAddOrRemoveByContainsFixer : CodeFixProvider | ||
{ | ||
public override ImmutableArray<string> FixableDiagnosticIds { get; } = | ||
ImmutableArray.Create(DoNotGuardSetAddOrRemoveByContains.RuleId); | ||
|
||
public sealed override FixAllProvider GetFixAllProvider() | ||
{ | ||
return WellKnownFixAllProviders.BatchFixer; | ||
} | ||
|
||
public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var root = await context.Document.GetRequiredSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
var node = root.FindNode(context.Span, getInnermostNodeForTie: true); | ||
|
||
if (node is null) | ||
{ | ||
return; | ||
} | ||
|
||
var diagnostic = context.Diagnostics[0]; | ||
var conditionalLocation = diagnostic.AdditionalLocations[0]; | ||
var childLocation = diagnostic.AdditionalLocations[1]; | ||
|
||
if (root.FindNode(conditionalLocation.SourceSpan) is not SyntaxNode conditionalSyntax || | ||
root.FindNode(childLocation.SourceSpan) is not SyntaxNode childStatementSyntax) | ||
{ | ||
return; | ||
} | ||
|
||
// We only offer a fixer if the conditional true branch has a single statement, either 'Add' or 'Delete' | ||
if (!SyntaxSupportedByFixer(conditionalSyntax, childStatementSyntax)) | ||
{ | ||
return; | ||
} | ||
|
||
var codeAction = CodeAction.Create(MicrosoftNetCoreAnalyzersResources.RemoveRedundantGuardCallCodeFixTitle, | ||
ct => Task.FromResult(ReplaceConditionWithChild(context.Document, root, conditionalSyntax, childStatementSyntax)), | ||
nameof(MicrosoftNetCoreAnalyzersResources.DoNotGuardSetAddOrRemoveByContainsTitle)); | ||
|
||
context.RegisterCodeFix(codeAction, diagnostic); | ||
} | ||
|
||
protected abstract bool SyntaxSupportedByFixer(SyntaxNode conditionalSyntax, SyntaxNode childStatementSyntax); | ||
|
||
protected abstract Document ReplaceConditionWithChild(Document document, SyntaxNode root, | ||
SyntaxNode conditionalOperationNode, | ||
SyntaxNode childOperationNode); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is going to throw ArgumentNullException
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.
Good catch, I thought this is for removing the false part, did not notice the '!'. What can we use instead?
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 originally copied this section from the similar fixer for dictionaries (
CSharpDoNotGuardDictionaryRemoveByContainsKeyFixer
).As part of a bugfix for the dictionary analyzer, it was discovered that the case of the ternary operator was never flagged by the analyzer (see #6770 (comment)).
So the code in the fixer that would throw an ArgumentNullException is never executed.
That part should probably be removed from the dictionary fixer as well (I missed that in the PR above):
roslyn-analyzers/src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardDictionaryRemoveByContainsKeyFixer.cs
Lines 44 to 48 in 47edf21
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.
Thanks for explanation @mpidash, removing this part sounds good to me, what you think @sharwell?
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 believe if this is reachable, it would would be hit by:
Maybe start by adding the test and seeing what kind of result it gives?
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've added two tests for ternary operators:
Add
case, but one is reported for theRemove
case.This is because we only check the true part of a conditional for an applicable
Add
orRemove
. So theAdd
case gets filtered out immediately because it contains no child operations:For
Remove
it depends on the content of the true part. In the simple case, it contains aFieldReferenceOperation
, which is not covered by the switch inHasApplicableAddOrRemoveMethod
so it produces no diagnostic.The nested case for
Remove
produces a diagnostic because the first child operation is aIInvocationOperation
.A ternary operator gets filtered out by the fixer in
SyntaxSupportedByFixer
, because thechildStatementSyntax
is not aExpressionStatementSyntax
(it isConditionalExpressionSyntax
). So the part where theArgumentNullException
would be thrown is not reachable.I think we should filter out ternary operators for now. I am not sure how likely it is that someone would write code like in the test cases, as they already use the return value of
Add
andRemove
.DoNotGuardDictionaryRemoveByContainsKey
should be adapted to treat ternary operators in the same way as this analyzer.The above has shown a case that is not covered at the moment:
which could be refactored to:
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.
Agree that it would be rare, though it could happen and it fixable. I am OK with filtering this out for now and create an issue for future fix.
Did you find why it was not covered and could you fix this with the PR?
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.
Its because atm only the true branch of a condition is checked for
Add
andRemove
.I will add a fix which also checks the false branch.
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.
Add
andRemove
in else blocksThere 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.
Thank you for the fix @mpidash!
@sharwell requested change is addressed PTAL