Fix 'Compare symbols correctly' false positive with custom comparers #5807
Conversation
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Outdated
Show resolved
Hide resolved
…eSymbolsCorrectlyAnalyzer.cs
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.Analyzers/UnitTests/MetaAnalyzers/CompareSymbolsCorrectlyTests.cs
Show resolved
Hide resolved
|
@Evangelink In case you want to review |
Codecov Report
@@ Coverage Diff @@
## main #5807 +/- ##
========================================
Coverage 95.58% 95.58%
========================================
Files 1284 1284
Lines 296834 296980 +146
Branches 18101 18105 +4
========================================
+ Hits 283725 283868 +143
- Misses 10670 10674 +4
+ Partials 2439 2438 -1 |
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Outdated
Show resolved
Hide resolved
| @@ -1248,9 +1248,156 @@ public int GetHashCode(object o1, object o2) | |||
| }.RunAsync(); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
Could you add the various vbnet parts?
There was a problem hiding this comment.
Will fix, probably in few days :(
Feel free to re-ping me if I forgot.
src/Microsoft.CodeAnalysis.Analyzers/UnitTests/MetaAnalyzers/CompareSymbolsCorrectlyTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.Analyzers/UnitTests/MetaAnalyzers/CompareSymbolsCorrectlyTests.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public void M(IEnumerable<ITypeSymbol> symbols) | ||
| { | ||
| _ = symbols.ToDictionary(s => s, s => s.ToDisplayString(), SymbolNameComparer.Instance); |
There was a problem hiding this comment.
I would move this line (and the method arg) to the 1st test. We could even consider copying all the methods declared in the test CollectionMethodsKnownToRequireComparer_DiagnosticAsync.
| @@ -1248,9 +1248,156 @@ public int GetHashCode(object o1, object o2) | |||
| }.RunAsync(); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task RS1024_CustomComparer() | |||
There was a problem hiding this comment.
I don't know when this was done but it seems that all tests were renamed to follow the Async suffix pattern, could you please update your tests?
There was a problem hiding this comment.
That looks weird. We shouldn't follow Async suffix for tests.
There was a problem hiding this comment.
I agree, don't know why it was done.
| { | ||
| public void M(IEnumerable<ITypeSymbol> symbols) | ||
| { | ||
| _ = symbols.ToDictionary(s => s, s => s.ToDisplayString(), SymbolNameComparer.Instance); |
There was a problem hiding this comment.
Same remark about moving this to test 2 this time.
Co-authored-by: Amaury Levé <[email protected]>
|
@Youssef1313 @Evangelink - do we need any more iterations on this PR? |
|
@mavasani I think that's ok. |
Fixes #5715