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

CA2000 Dispose for MemoryStream #4147

Closed
NN--- opened this issue Sep 9, 2020 · 5 comments · Fixed by #5716
Closed

CA2000 Dispose for MemoryStream #4147

NN--- opened this issue Sep 9, 2020 · 5 comments · Fixed by #5716
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@NN---
Copy link

NN--- commented Sep 9, 2020

Analyzer

Diagnostic ID: CA2000

Describe the improvement

MemoryStream doesn't hold anything that needs to be disposed.

var ms = new MemoryStream(myArray, 0, 10); // CA2000

using var msWithUsing = new MemoryStream(myArray, 0, 10); // Analyzer happy but makes redundant Dispose call.

Describe suggestions on how to achieve the rule

The rule CA2000 should not warn on MemoryStream object.

Solution

Add MemoryStream/UnmanagedMemoryStream to exclusion list:

dotnet_code_quality.CA2000.excluded_symbol_names = MemoryStream|UnmanagedMemoryStream

Additional context

@NN---
Copy link
Author

NN--- commented Sep 9, 2020

Added MemoryStream to exclusions.

@mavasani mavasani added Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting labels Feb 9, 2021
@mavasani mavasani added this to the Unknown milestone Feb 9, 2021
@mavasani
Copy link
Member

mavasani commented Feb 9, 2021

We need documentation confirmation for MemoryStream to confirm it is fine to exclude.

@sharwell
Copy link
Member

sharwell commented Feb 9, 2021

@stephentoub The documentation for MemoryStream states that it does not hold any unmanaged resources, but it does not explicitly state that derived types are prohibited from adding them. How should we approach this suggested change?

@TimLovellSmith
Copy link
Contributor

@sharwell How about approaching this change by not warning for MemoryStream when you know it cannot be a derived type because the MemoryStream is instantiated in the local scope and not mutated? I think that would cover about 80% or 90% of the false positives which are bugging me.

@TimLovellSmith
Copy link
Contributor

@sharwell So had a quick look and that actually looks harder than I expected, I think it would need extra work to describe and differentiate known to be the exact (but unsealed) type versus possibly a subtype, in the absence of general type system support for this concept.

How about again we just solve for the most useful case. Never generate this warning for MemoryStream and its subclasses, as long as they are only known to be of type MemoryStream locally.

Rationale: subclassing MemoryStream is pretty uncommon, and so its better to optimize the CA2000 warning (which is just a heuristic), for the common case, which is what provides the most productivity benefit. That way you can pay the lowest costs for actually enabling the warning in your dev environment -- and get the most overall value out of it.

tldr; Its more helpful to add and document this behavior of ignoring MemoryStreams, than to continue to generate useless nags for the whole world "dispose your MemoryStream" -- since they are nearly always being addressed by adding useless using blocks, or adding useless and verbose suppressions, or just disabling or downgrading the whole rule.

TimLovellSmith added a commit to TimLovellSmith/roslyn-analyzers that referenced this issue Nov 11, 2021
…oryStream()'.

#pragma warning disable CA2000 // Dispose objects before losing scope
            var stream = new MemoryStream();
#pragma warning restore CA2000 // Dispose objects before losing scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design False_Positive A diagnostic is reported for non-problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
4 participants