-
Notifications
You must be signed in to change notification settings - Fork 463
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
CA2022: Do not flag well known reliable stream types #7269
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7269 +/- ##
==========================================
- Coverage 96.48% 96.48% -0.01%
==========================================
Files 1442 1442
Lines 345212 345257 +45
Branches 11345 11346 +1
==========================================
+ Hits 333081 333120 +39
- Misses 9256 9263 +7
+ Partials 2875 2874 -1 |
Based on that comment I would say no, but as the |
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 fix LGTM, thank you, I'll wait for opinions for the open question for making it configurable
We could, but I'd be fine waiting until we have real user cases for it. MemoryStream is one that comes up a lot, especially in tests, and so it's worth reducing noise about. |
Good point, thanks! |
@@ -130,6 +141,11 @@ public bool IsAnyStreamReadAsyncMethod(IMethodSymbol method) | |||
SymbolEqualityComparer.Default.Equals(method, m) || IsOverrideOf(method, m)); | |||
} | |||
|
|||
public bool IsKnownReliableStreamType(ITypeSymbol? type) | |||
{ | |||
return _knownReliableStreamTypes.Any(t => SymbolEqualityComparer.Default.Equals(type, t)); |
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 the set be created with this comparer so the lookup is O(1) rather than O(N)?
Fixes #7268.
I only excluded exact type matches of known reliable stream types (i.e. instances of
System.IO.MemoryStream
orSystem.IO.UnmanagedMemoryStream
).Let me know if we want to exclude child classes as well, but it seems based on the comment in the issue (#7268 (comment)) that this could lead to false negatives.
We could detect more cases with flow analysis, but this would mean that the rule is disabled by default (AFAIK).
cc @buyaa-n @stephentoub