search

dotnet / roslyn-analyzers

MIT License
1.56k stars 462 forks source link

CA2022: Do not flag well known reliable stream types #7269

Closed mpidash closed 3 months ago

mpidash commented 3 months ago

Fixes #7268.

I only excluded exact type matches of known reliable stream types (i.e. instances of System.IO.MemoryStream or System.IO.UnmanagedMemoryStream). Let me know if we want to exclude child classes as well, but it seems based on the comment in the issue (https://github.com/dotnet/roslyn-analyzers/issues/7268#issuecomment-2021040159) 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

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.48%. Comparing base (0868ca2) to head (d454355).

Additional details and impacted files ```diff @@ 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 ```
buyaa-n commented 3 months ago

Let me know if we want to exclude child classes as well, but it seems based on the comment in the issue (https://github.com/dotnet/roslyn-analyzers/issues/7268#issuecomment-2021040159) that this could lead to false negatives.

Based on that comment I would say no, but as the Stream or MemoryStream are not sealed we might want to make it configurable: https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md#excluded-symbols what you think @mpidash @stephentoub?

stephentoub commented 3 months ago

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.

Based on that comment I would say no, but as the Stream or MemoryStream are not sealed we might want to make it configurable: https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md#excluded-symbols what you think @mpidash @stephentoub?

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.

buyaa-n commented 3 months ago

Good point, thanks!