dotnet / roslyn-analyzers

MIT License
1.6k stars 467 forks source link

CA2000 false positive when using fluent API's #5709

Open lucastheisen opened 3 years ago

lucastheisen commented 3 years ago

Analyzer

Diagnostic ID: CA2000: Dispose objects before losing scope

Analyzer source

SDK: Built-in CA analyzers in .NET 5 SDK or later

Version: SDK 5.0.401

Describe the bug

I created a Stream wrapper for adding IProgress support. It has fluent style registration for the reporters:

        /// <summary>Registers a progress callback for calls to <c>Read</c>.</summary>
        /// <param name="progress">The callback, provided with the total bytes read to this
        /// point.</param>
        /// <returns>This stream for chaining.</returns>
        public ProgressStream AddReadProgressReporter(IProgress<long> progress)
        {
            this.readProgressCallbacks.Add(progress);
            return this;
        }

        /// <summary>Registers a progress callback for calls to <c>Write</c>.</summary>
        /// <param name="progress">The callback, provided with the total bytes written to this
        /// point.</param>
        /// <returns>This stream for chaining.</returns>
        public ProgressStream AddWriteProgressReporter(IProgress<long> progress)
        {
            this.writeProgressCallbacks.Add(progress);
            return this;
        }

However, when consumed using the fluent approach:

using var stream = new ProgressStream(File.OpenRead("file.txt"))
    .AddWriteProgressReporter(reporter);

It will trigger this finding because it is not detecting that the stream returned by .Add[Read|Write]ProgressReporter is the same object.

I can work around it by separating into two commands:

using var stream = new ProgressStream(File.OpenRead("file.txt"));
stream.AddWriteProgressReporter(reporter);

But this has other downsides. I could ignore the warning at every consumption site, but that also has a huge downside to usability for consumers.

Steps To Reproduce

  1. Create a class that wraps a stream
  2. Create a fluent API to modify the class and return this
  3. Attempt to use in a fluent fashion

Expected behavior

No warning

Actual behavior

CA2000

Additional context

ltalasil24 commented 3 months ago

Is there any update on this issue? Would appreciate if any update can be provided regarding this. Is this issue fixed but the NuGet package isn't pushed yet as this seem to be open since 2021?

bwilliams1 commented 3 months ago

I am also interested in an update on this issue. we have many false positives in our app that we would like cleaned up with the fix to this issue.

ltalasil24 commented 2 months ago

Any update on this issue is appreciated. Thank you!

sharwell commented 2 months ago

I don't believe any fix is currently planned here. The current CA2000 behavior was largely derived from legacy FxCop. We've had a hard time making any alterations to the behavior without causing regressions (performance or functional) for some of our larger enterprise customers. For better or worse, the request we kept getting is essentially "we understand it has limitations, but those limitations are predictable so please don't make any changes to them". It should be possible to implement a targeted DiagnosticSuppressor specific to your project which understands how to automatically suppress CA2000 when it gets reported on one of your known fluent APIs.