CA2213 code analysis rule and auto-implemented properties

Question

I use static code analysis in our projects to check for code violations. One of extensively used rules is CA2213, which checks for correct disposing of disposable fields.

I noticed CA2213 does not check disposing of auto implemented properties.

Furthermore CA2213 does not check disposing of neither fields nor auto implemented properties if the class inherits from class which implements IDisposable and does not override the Dispose method.

Practical example:

public sealed class Good : IDisposable {
    private Font font;
    public Font Font {
        get { return font; }
        set { font = value; }
    }
    public Good() { font = new Font("Arial", 9); }
    public void Dispose() { /* Do nothing */ }       // CA2213
}

public sealed class Bad : IDisposable {
    public Font Font { get; set; }
    public Bad() { Font = new Font("Arial", 9); }
    public void Dispose() { /* Do nothing */ }       // No warning
}

Has anyone else encountered this behavior? Is this by design or bug in CA2213 rule?

score 5 · Answer 1 · answered Jul 10 '15 at 17:59

Code analyzers do have limitations, the one present in this code is that it needs to generate an actionable warning. One that the programmer can actually follow up on to fix his code. A key problem with the current code analyzer is that it does not analyze source code, it works from the assembly that's generated by the compiler. Which is nice, it works for any .NET language. But nobody will like this warning:

CA2213 Disposable fields should be disposed
'Bad' contains field <Font>k__BackingField that is of IDisposable type: 'Font'. Change the Dispose method on 'Bad' to call Dispose or Close on this field.

Yikes, of course there is no such field in the program. To generate a better message, the analyzer would have to figure out that the <Font>k__BackingField field in the metadata is in fact associated with the auto-implemented Font property. There is nothing available in the metadata that makes that connection unambiguously. The field only carries the [CompilerGenerated] attribute and the auto-generated field name is a compiler implementation detail. Different language compilers generate different names.

It is the kind of problem that requires source-code analysis, not IL analysis as currently implemented. Opportunity is knocking on the door, source-code analysis is going to be a lot easier to implement with Roslyn available today. VS2015 is the first VS version that supports "Live Code" analysis. I don't know yet whether it looks for CA2213 style errors, soon.

Is there a solution that can work for VS2012? Such as a StyleCop rule which triggers on AutoProperties which inherit from IDisposable? — Nate Diamond, Jul 10 '15 at 18:43
I'm marking this answer for bounty as it explains the difficulty for the Code Analysis engine with respect to IL analysis. The answer seems to be that by default you can't cover the issues that I and Zvonko have outlined. If in the future I find a method of creating a StyleCop rule to handle this, or we move to Roslyn and I have time to add a custom code analysis rule, then I will make sure to update. — Nate Diamond, Jul 14 '15 at 17:14
I agree, the solution is not straight forward. To cover as much cases as possible when analyzing the source code (and to prevent bad code going to our VCS), I combine different approaches: source code analysis (StyleCop), IL analysis (FxCop), unit tests with reflection (to force rules on specific types), additional program code (which executes only in debug build or when debugger is attached) and custom PowerShell scripts. Of course gated check-ins are enabled and do all that. — Zvonko, Jul 22 '15 at 08:14
FYI, there is already an official project in a pretty advanced state for doing FxCop with Roslyn: https://github.com/dotnet/roslyn-analyzers. They implemented CA2213, then they cut it out on grounds of difficulty to remove the excessive number of false positives: https://github.com/dotnet/roslyn-analyzers/issues/695, but it will probably be reconsidered again in the future. So do vote there! — Ignacio Calvo, Nov 04 '16 at 15:46

score 0 · Answer 2 · answered Jul 10 '15 at 17:11

Static analysis tools are tuned for a good trade-off between generating too many false-positives (i.e. emitting issues on perfectly fine code) and too many false-negatives (i.e. missing real issues in the code).

Let's take a simple example:

// CA2213 detects an issue
class OwnsTheField : IDisposable
{
    private MemoryStream f = new MemoryStream();
    public void Dispose() {}
}

It is clear in this case that an issue should be reported, and that is indeed the case.

Now, let's make things slightly more complicated:

// CA2213 does not detect
class FieldIsInjected : IDisposable
{
    private MemoryStream f;
    public FieldIsInjected(MemoryStream p)
    {
        f = p;
    }
    public void Dispose() { }
}

In this case, there might not be an issue with not disposing f. If pis properly disposed by the caller, then everything works as expected:

using (var p = new MemoryStream())
{
    using (var x = FieldIsInjected(p)) { /* ... */ }
} // p is properly disposed

More often than not, the exact behavior (including the handling of all corner cases) will not be documented, as an exhaustive documentation would be too costly to maintain over time. For example, this exception is explicitly mentioned in https://msdn.microsoft.com/en-us/library/ms182328.aspx

In the case you list, Code Analysis still requires you to handle `f` in some way, usually by suppressing manually and noting in the justification that the injected field is not owned by the current class. This is a fine solution for me. What is not a fine solution is not showing the code analysis issue at all, simply because `f` is an autoproperty or because FieldIsInjected derives from an IDisposable class and doesn't override Dispose(bool) — Nate Diamond, Jul 10 '15 at 17:14

score 0 · Answer 3 · edited May 23 '17 at 12:08

The rule is from .net framework 2, and auto properties are from C# 3, which was added to the language specification after the rule was created. The rule is defined in terms of fields not properties, so it seems like it is due for an update but working as currently specified.

This Question on disposing properties seems to show that properties that implement IDisposable aren't consistent in the framework.

score 0 · Answer 4 · answered Aug 17 '15 at 21:16

The analysis engine is perfectly capable of walking between the property and its backing field. It just doesn't have the logic to do it, and so misses an opportunity for a warning. Worse, it emits a false warning if a properly disposed property is get-only.

CA2213 code analysis rule and auto-implemented properties

4 Answers4