Add CA1868: Unnecessary call to `Set.Contains(item)` #6767

mpidash · 2023-07-13T20:54:53Z

This analyzer detects when either Set.Add or Set.Remove is guarded by Set.Contains.
As recommended in dotnet/runtime#85490, a fix is only suggested when the conditional body only has a single Set.Add or Set.Remove.

I tried out the analyzer in dotnet/roslyn, dotnet/roslyn-analyzers and dotnet/runtime and found cases of guarded calls:

Fixes dotnet/runtime#85490

This analyzer detects when either `Set.Add` or `Set.Remove` is guarded by `Set.Contains`. A fix is only suggested when the conditional body has a single statement that is either one of the former two methods. Fix #85490

…call

codecov · 2023-07-13T21:22:18Z

Codecov Report

Merging #6767 (953a052) into main (7fcfa19) will increase coverage by 0.01%.
The diff coverage is 98.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6767      +/-   ##
==========================================
+ Coverage   96.34%   96.36%   +0.01%     
==========================================
  Files        1394     1399       +5     
  Lines      327462   329717    +2255     
  Branches    10788    10841      +53     
==========================================
+ Hits       315507   317727    +2220     
- Misses       9237     9258      +21     
- Partials     2718     2732      +14

...rp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs

...Analyzers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.cs

...ers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.Fixer.cs

This reverts commit 68c210f.

mpidash · 2023-07-14T22:26:06Z

I've added additional tests and corrected the fixer analogous to #6770, PTAL.

buyaa-n · 2023-07-18T17:27:11Z

I tried out the analyzer in dotnet/roslyn, dotnet/roslyn-analyzers and dotnet/runtime and found cases of guarded calls:

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs#1376
runtime/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DgmlWriter.cs#L161
...

Thank you @mpidash I found more hits in runtime, FYI you could build with -warnaserror 0 option to continue building with analyzers warnings, to see all the diagnostics caused by the analyzer.

Found one false positive with the testing, it warns when the elements in the Contains and Add area different, for example:

    public void Test(HashSet<string> runtimeOptions, string[] jiterpreterOptions, bool jiterpreter)
    {
        foreach (var jiterpreterOption in jiterpreterOptions)
        {
            if (jiterpreter == true)
            {
                if (!runtimeOptions.Contains($"--no-{jiterpreterOption}"))
                    runtimeOptions.Add($"--{jiterpreterOption}");
            }
            else
            {
                if (!runtimeOptions.Contains($"--{jiterpreterOption}"))
                    runtimeOptions.Add($"--no-{jiterpreterOption}");
            }
        }
    }

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

mpidash · 2023-07-25T20:02:52Z

Great thank you @mpidash feel free to put up a PR with the fix for valid diagnostics to those repos

I'll prepare one for dotnet/runtime, already submitted for the other two: dotnet/roslyn#69179 and #6793.

Looks great, thank you! Left a few more comment, overall the PR shape looks good.

Thank you so much for your guidance and patience with me! 🎉

...rp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs

sharwell · 2023-07-25T20:32:46Z

...rp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs

+                SyntaxNode newConditionalOperationNode = conditionalExpressionSyntax
+                    .WithCondition((ExpressionSyntax)expression)
+                    .WithWhenTrue(conditionalExpressionSyntax.WhenFalse)
+                    .WithWhenFalse(null!)


❗ This is going to throw ArgumentNullException

Good catch, I thought this is for removing the false part, did not notice the '!'. What can we use instead?

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

SyntaxNode newConditionalOperationNode = conditionalExpressionSyntax

.WithCondition((ExpressionSyntax)negatedExpression)

.WithWhenTrue(conditionalExpressionSyntax.WhenFalse)

.WithWhenFalse(null!)

.WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode);

Thanks for explanation @mpidash, removing this part sounds good to me, what you think @sharwell?

I believe if this is reachable, it would would be hit by:

bool added = set.Contains(value) ? false : set.Add(value);

Maybe start by adding the test and seeing what kind of result it gives?

I've added two tests for ternary operators:

One with a ternary operator as in your example. In this case, no diagnostic is reported.

Another test with a nested ternary operator. No diagnostic is reported for the Add case, but one is reported for the Remove case.

This is because we only check the true part of a conditional for an applicable Add or Remove. So the Add case gets filtered out immediately because it contains no child operations:

var firstOperation = operations.FirstOrDefault(); if (firstOperation is null) { return false; }

For Remove it depends on the content of the true part. In the simple case, it contains a FieldReferenceOperation, which is not covered by the switch in HasApplicableAddOrRemoveMethod so it produces no diagnostic.
The nested case for Remove produces a diagnostic because the first child operation is a IInvocationOperation.

A ternary operator gets filtered out by the fixer in SyntaxSupportedByFixer, because the childStatementSyntax is not a ExpressionStatementSyntax (it is ConditionalExpressionSyntax). So the part where the ArgumentNullException 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 and Remove.
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:

if (MySet.Contains(""Item"")) { throw new Exception(""Item already exists""); } else { MySet.Add(""Item""); }

which could be refactored to:

if (!MySet.Add(""Item"")) { throw new Exception(""Item already exists""); }

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 and Remove.
DoNotGuardDictionaryRemoveByContainsKey should be adapted to treat ternary operators in the same way as this analyzer.

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.

The above has shown a case that is not covered at the moment:

Did you find why it was not covered and could you fix this with the PR?

Did you find why it was not covered and could you fix this with the PR?

Its because atm only the true branch of a condition is checked for Add and Remove.
I will add a fix which also checks the false branch.

ad82982 adds support for Add and Remove in else blocks

f723ac5 adds support for ternary diagnostics (no fixer)

Thank you for the fix @mpidash!

@sharwell requested change is addressed PTAL

buyaa-n · 2023-07-25T20:56:44Z

@mpidash seems need to run msbuild -pack once more

Co-authored-by: Sam Harwell <[email protected]>

sharwell · 2023-07-26T12:53:10Z

...UnitTests/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContainsTests.cs

+        #region Helpers
+        private const string CSUsings = @"using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;";
+
+        private const string CSNamespaceAndClassStart = @"namespace Testopolis
+{
+    public class MyClass
+    {
+";
+
+        private const string CSNamespaceAndClassEnd = @"
+    }
+}";
+
+        private const string VBUsings = @"Imports System
+Imports System.Collections.Generic
+Imports System.Collections.Immutable";


💡 It would be preferable to inline all of these

Just to be sure that I understand correctly:

Change this:

[Fact] public async Task AddWithVariableAssignment_ReportsDiagnostic_CS() { string source = CSUsings + CSNamespaceAndClassStart + @" private readonly HashSet<string> MySet = new HashSet<string>(); public MyClass() { if (![|MySet.Contains(""Item"")|]) { bool result = MySet.Add(""Item""); } } " + CSNamespaceAndClassEnd; await VerifyCS.VerifyCodeFixAsync(source, source); }

To:

[Fact] public async Task AddWithVariableAssignment_ReportsDiagnostic_CS() { string source = """ using System.Collections.Generic; class C { private readonly HashSet<string> MySet = new HashSet<string>(); void M() { if (![|MySet.Contains("Item")|]) { bool result = MySet.Add("Item"); } } } """; await VerifyCS.VerifyCodeFixAsync(source, source); }

Just to be sure that I understand correctly:

Correct, it meant to move CSNamespaceAndClassStart and CSNamespaceAndClassEnd inline.

Changed in 6f69703.

...UnitTests/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContainsTests.cs

mpidash · 2023-07-26T23:41:36Z

I've re-ran the analyzer against dotnet/roslyn, dotnet/roslyn-analyzers and dotnet/runtime and found two new guarded calls, both in else blocks:

Also ran it against dotnet/aspnetcore and found one guarded call: aspnetcore/src/Shared/StaticWebAssets/ManifestStaticWebAssetFileProvider.cs#L103-L110

I found no cases of ternary operators.

mpidash · 2023-07-26T23:46:43Z

Looks like CA1865 was used by another analyzer that was already merged -- I will switch to the next one that is free.

...UnitTests/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContainsTests.cs

buyaa-n

Thank you @mpidash LGTM!

…ance/DoNotGuardSetAddOrRemoveByContainsTests.cs

The requested change addressed

* See dotnet/roslyn-analyzers#6767 * IWalletModel: Unify * Fix prison tests * Redundant `public` keywords * Remove unnecessary cast (error) * `PrisonTests`: Add timeout for safety * ?

* Rename do not guard set analyzer and fixer This is done in a separate commit, so that the actual changes can be seen in the next commit. This is because git would not recognize the rename if the changes were added as is. Note that this commit by itself is broken. * Combine analyzers for guarded calls This combines the analyzers CA1853 and CA1868 to avoid code duplication, make CA1853 support the same range of cases as CA1868 and fix #6781.

mpidash added 5 commits July 12, 2023 01:18

Add analyzer and fixer for CA1865

d5e2607

This analyzer detects when either `Set.Add` or `Set.Remove` is guarded by `Set.Contains`. A fix is only suggested when the conditional body has a single statement that is either one of the former two methods. Fix #85490

Merge branch 'dotnet:main' into issue-85490-analyzer-unnecessary-set-…

4d10f23

…call

Add CA1865 to Microsoft.CodeAnalysis.NetAnalyzers.md

5c6bcdb

Add CA1865 to RulesMissingDocumentation.md

9547162

Add changes from msbuild pack

52e7f91

mpidash requested a review from a team as a code owner July 13, 2023 20:54

github-actions bot added the Community label Jul 13, 2023

Update DoNotGuardSetAddOrRemoveByContainsDescription

fea7a0a

mavasani assigned geeknoid and buyaa-n Jul 14, 2023

Youssef1313 reviewed Jul 14, 2023

View reviewed changes

...rp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 14, 2023

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 14, 2023

View reviewed changes

...ers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 14, 2023

View reviewed changes

...ers/Core/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContains.Fixer.cs Outdated Show resolved Hide resolved

mpidash added 6 commits July 14, 2023 15:49

Add ExportCodeFixProviderAttribute

02fc236

Remove call to First with call to indexer

68c210f

Change equivalenceKey of CodeAction

5a0113f

Check for interface implementation instead of signature match

5e841a6

Revert "Remove call to First with call to indexer"

07943c4

This reverts commit 68c210f.

Remove call to First with call to indexer

9b43e00

mpidash mentioned this pull request Jul 14, 2023

Applying CA1853 produces incorrect code that changes program behavior #6274

Closed

mpidash added 4 commits July 15, 2023 00:03

Add additional tests

a8e4875

Fix ifs with additional statements

29a4630

Fix single line ifs in VB

468274e

Add fixer case for single line ifs in VB

326481c

buyaa-n reviewed Jul 18, 2023

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

buyaa-n reviewed Jul 18, 2023

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

mpidash added 3 commits July 25, 2023 21:18

Make symbol display format static

8a3f6ef

Typo

be9d96e

Update DoNotGuardSetAddOrRemoveByContainsTitle

5fef8d0

sharwell reviewed Jul 25, 2023

View reviewed changes

...rp/Microsoft.NetCore.Analyzers/Performance/CSharpDoNotGuardSetAddOrRemoveByContains.Fixer.cs Outdated Show resolved Hide resolved

sharwell previously requested changes Jul 25, 2023

View reviewed changes

mpidash and others added 2 commits July 25, 2023 23:00

Change condition to pattern matching

1d220c6

Co-authored-by: Sam Harwell <[email protected]>

Add changes from msbuild pack

c5376f3

sharwell reviewed Jul 26, 2023

View reviewed changes

mpidash added 4 commits July 26, 2023 16:09

Add tests for ternary operators

7a9fa60

Use raw strings and inline class and usings

6f69703

Support Add or Remove in else block

ad82982

Support ternary operator (diagnostic only)

f723ac5

mpidash added 2 commits July 27, 2023 01:57

Move from CA1865 to CA1868

915a4ad

Merge branch 'main' into issue-85490-analyzer-unnecessary-set-call

8866f5d

mpidash changed the title ~~Add CA1865: Unnecessary call to Set.Contains(item)~~ Add CA1868: Unnecessary call to Set.Contains(item) Jul 27, 2023

buyaa-n reviewed Jul 27, 2023

View reviewed changes

...UnitTests/Microsoft.NetCore.Analyzers/Performance/DoNotGuardSetAddOrRemoveByContainsTests.cs Outdated Show resolved Hide resolved

buyaa-n approved these changes Jul 27, 2023

View reviewed changes

Update src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Perform…

953a052

…ance/DoNotGuardSetAddOrRemoveByContainsTests.cs

buyaa-n merged commit 1479680 into dotnet:main Jul 28, 2023

mpidash deleted the issue-85490-analyzer-unnecessary-set-call branch July 28, 2023 20:14

mpidash mentioned this pull request Jul 28, 2023

Enable new analyzer CA1868: 'Unnecessary call to 'Contains' for sets' and fix findings dotnet/runtime#89652

Merged

kiminuo added a commit to kiminuo/WalletWasabi that referenced this pull request Jul 31, 2023

See dotnet/roslyn-analyzers#6767

472a353

kiminuo mentioned this pull request Jul 31, 2023

Fix prison tests & a few minor improvements WalletWasabi/WalletWasabi#11165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CA1868: Unnecessary call to `Set.Contains(item)` #6767

Add CA1868: Unnecessary call to `Set.Contains(item)` #6767

mpidash commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

mpidash commented Jul 14, 2023

buyaa-n commented Jul 18, 2023

mpidash commented Jul 25, 2023

sharwell Jul 25, 2023

buyaa-n Jul 25, 2023

mpidash Jul 25, 2023

buyaa-n Jul 25, 2023

sharwell Jul 26, 2023 •

edited

Loading

mpidash Jul 26, 2023

buyaa-n Jul 26, 2023

mpidash Jul 26, 2023

mpidash Jul 26, 2023

buyaa-n Jul 27, 2023

buyaa-n commented Jul 25, 2023

sharwell Jul 26, 2023

mpidash Jul 26, 2023

buyaa-n Jul 26, 2023

mpidash Jul 26, 2023

mpidash commented Jul 26, 2023

mpidash commented Jul 26, 2023

buyaa-n left a comment

	SyntaxNode newConditionalOperationNode = conditionalExpressionSyntax
	.WithCondition((ExpressionSyntax)negatedExpression)
	.WithWhenTrue(conditionalExpressionSyntax.WhenFalse)
	.WithWhenFalse(null!)
	.WithAdditionalAnnotations(Formatter.Annotation).WithTriviaFrom(conditionalOperationNode);

Add CA1868: Unnecessary call to Set.Contains(item) #6767

Add CA1868: Unnecessary call to Set.Contains(item) #6767

Conversation

mpidash commented Jul 13, 2023 • edited Loading

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

mpidash commented Jul 14, 2023

buyaa-n commented Jul 18, 2023

mpidash commented Jul 25, 2023

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

sharwell Jul 26, 2023 • edited Loading

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

buyaa-n commented Jul 25, 2023

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

mpidash commented Jul 26, 2023

mpidash commented Jul 26, 2023

buyaa-n left a comment

Choose a reason for hiding this comment

Add CA1868: Unnecessary call to `Set.Contains(item)` #6767

Add CA1868: Unnecessary call to `Set.Contains(item)` #6767

mpidash commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

sharwell Jul 26, 2023 •

edited

Loading