dotnet / roslyn-analyzers

MIT License
1.59k stars 465 forks source link

CA2000 false positive when disposable class is in another assembly, using method chaining and old using syntax is used #3022

Open prosser opened 4 years ago

prosser commented 4 years ago

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.7 (Latest)

Diagnostic ID

CA2000

Repro steps

See attached minimum repro solution. CATest.zip

  1. Create C# library project "CATestLib " (does not matter the fx, repro solution uses .NET Core 3.0)
  2. Add the following class to CATestLib project:
    
    using System;
    using System.Diagnostics.CodeAnalysis;

namespace CATestLib {

public abstract class MyDisposableBase<T> : IDisposable
    where T : MyDisposableBase<T>
{
    [SuppressMessage("Globalization", "CA1303:Do not pass literals as localized parameters", Justification = "Test code")]
    public T Chained()
    {
        Console.WriteLine("Chaining...");
        return (T)this;
    }

    public void Dispose()
    {
        this.Dispose(true);
    }

    [SuppressMessage("Globalization", "CA1303:Do not pass literals as localized parameters", Justification = "Test code")]
    public void Print()
    {
        Console.WriteLine("Hello World!");
    }

    protected virtual void Dispose(bool disposing)
    { }
}

}

3. Create C# console app project "CATest"
4. Add reference on CATest project to CATestLib project
5. Add package reference to Microsoft.CodeAnalysis.FxCopAnalyzers 2.9.7 to App project
6. Replace to App's Program.cs content with:

namespace CATest { using CATestLib;

public class MyDisposable : MyDisposableBase<MyDisposable>
{
}

internal class Program
{
    private static void Main()
    {
        var program = new Program();
        program.BuildsClean();
        program.BuildsCleanToo();
        program.BuildsWithWarningCa2000();
    }

    private void BuildsClean()
    {
        using var disposableNoWarning = new MyDisposable().Chained();
        disposableNoWarning.Print();
    }

    private void BuildsCleanToo()
    {
        using (var disposableHasWarning = new MyDisposable().Chained())
        {
            disposableHasWarning.Print();
        }

        using var disposableNoWarning = new MyDisposable().Chained();
        disposableNoWarning.Print();
    }

    private void BuildsWithWarningCa2000()
    {
        using (var disposableHasWarning = new MyDisposable().Chained())
        {
            disposableHasWarning.Print();
        }
    }
}

}


#### Expected behavior
No CA2000 warning on either WarningCa2000 or NoWarnings methods.

#### Actual behavior
CA2000 warning on WarningCa2000 method, but not on NoWarnings method.
mavasani commented 4 years ago

We used to skip analysis of code with C# using 8 declarations, as the compiler did not expose an API for analyzing those until recently. If you move to VS2019 16.4, you would see identical behavior regardless of the whether you have a using statement or a using declaration.

The core issue here is new MyDisposable().Chained() here returns the same object as new MyDisposable() due to chaining. We are able to analyze this correctly if the type and chaining is in the same project. However, interprocedural analysis across assembly boundaries is not supported, so we do not recognize the chaining and aggressively assume that these are distinct objects. I think the best option for us would be to provide you a configurable option that would not flag any instances of a specific type to avoid false positives. Will that work?