Skip to content
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

fix CA2021 false positives #7183

Merged
merged 3 commits into from
Feb 15, 2024
Merged

fix CA2021 false positives #7183

merged 3 commits into from
Feb 15, 2024

Conversation

fowl2
Copy link
Contributor

@fowl2 fowl2 commented Feb 8, 2024

Can't say I really understand this that well, but adds the test case and fixes #7153 without breaking anything else as far I can tell.


The code was previously using ITypeSymbol property AllInterfaces which says the seemly relevantly:

// This is not quite the same as "all interfaces of which this type
// is a proper subtype"
However, I couldn't find any sort of "proper subtype" API.

So this change sprinkles some OriginalDefinition calls around, and switches to using DerivesFrom - which calls OriginalDefinition on all the interface symbols before checking equality.

@fowl2 fowl2 requested a review from a team as a code owner February 8, 2024 07:52
@fowl2 fowl2 force-pushed the enumerablecasts-fix2 branch from 8404737 to de9cac1 Compare February 8, 2024 08:12
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d0311fb) 96.44% compared to head (b87e72e) 96.44%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7183      +/-   ##
==========================================
- Coverage   96.44%   96.44%   -0.01%     
==========================================
  Files        1415     1415              
  Lines      338191   338229      +38     
  Branches    11191    11191              
==========================================
+ Hits       326166   326201      +35     
- Misses       9205     9209       +4     
+ Partials     2820     2819       -1     

@buyaa-n
Copy link
Contributor

buyaa-n commented Feb 15, 2024

Can't say I really understand this that well, but adds the test case and fixes #7153 without breaking anything else as far I can tell.

The code was previously using ITypeSymbol property AllInterfaces which says the seemly relevantly:

// This is not quite the same as "all interfaces of which this type
// is a proper subtype"
However, I couldn't find any sort of "proper subtype" API.

So this change sprinkles some OriginalDefinition calls around, and switches to using DerivesFrom - which calls OriginalDefinition on all the interface symbols before checking equality.

I think DerivesFrom works because it checks constraints, not only all interfaces

…/DoNotCallEnumerableCastOrOfTypeWithIncompatibleTypesAnalyzerTests.cs
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @fowl2 for quick fix

@buyaa-n buyaa-n enabled auto-merge (squash) February 15, 2024 22:36
…/DoNotCallEnumerableCastOrOfTypeWithIncompatibleTypesAnalyzerTests.cs
@buyaa-n buyaa-n merged commit 35c0109 into dotnet:main Feb 15, 2024
11 checks passed
@fowl2 fowl2 deleted the enumerablecasts-fix2 branch March 28, 2024 00:15
JoeRobich added a commit that referenced this pull request Dec 3, 2024
Fix for #7357.

#7183 changed the CastWillAlwaysFail(ITypeSymbol castFrom, ITypeSymbol castTo) to work with castFrom.OriginalDefinition/castTo.OriginalDefinition, eliding the generic type information in order to fix issues with interfaces, but that appears to have broken the check for class types. This reverts just the class type case to use the castFrom/castTo types passed in instead of the .OriginalDefinition. I'm not sure I quite understand why that's the only place this matters, but I tried a few variations with structs and interfaces and couldn't get any other false positives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CA2021 false positive on Enumerable.OfType<T>extension
2 participants