Bug 4/30: Disposable Analyzer Code Fix

Posted on Sep 28, 2020

Bug 4 is DotNetAnalyzers/IDisposableAnalyzers/Wrong code generated in fix for IDSP002.

For many years now I’ve been huge fan of StyleCop, which is an ex-Microsoft tool that enforces a well-thought-out coding standard on a C# codebase. When combined with its R# plug-in it allowed a whole team of developers to maintain a rigorous coding standard across a large codebase with almost no overhead. It also has the advantage of that standard being good.

The C# language grew considerably after StyleCop was made public, and although its rules remained valid, StyleCop itself seemed to no longer be under active development. This caused frustration for a lot of teams because, for those that had adopted it, it was a critical tool.

Then, soon after the release of the Roslyn compiler, a new project was started to replicate and extend StyleCop as a collection of Roslyn Analyzers: DotNetAnalyzers/StyleCopAnalyzers. This no longer requires R# to integrate into Visual Studio, and the success of the project has completely dwarfed that of the original StyleCop. The Analyzer’s NuGet package has been downloaded almost 30M times.

More recently this project has spawned many new Analyzer libraries. These Analyzer libraries find issues in code files (before compilation) and in many cases provide fixes for these issues. This bug concerns an incorrect code fix for one of the rules in the analyzer for IDisposable and IAsyncDisposable symbols.

The defect here is that when a class member can be cast to IDisposable or IAsyncDisposable, and a code fix is triggered to dispose that member, the cast that is generated is not wrapped in parentheses, so the code no longer compiles. This is better demonstrated by example.

Before triggering the code fix the code looks like this:

1
2
3
4
5
6
7
8
public sealed class C : IDisposable
{
    public object D { get; } = new Disposable(); // trigger the code fix on this line.

    public void Dispose()
    {
    }
}

After triggering the code fix it currently looks like this:

1
2
3
4
5
6
7
8
9
public sealed class C : IDisposable
{
    public object D { get; } = new Disposable();

    public void Dispose()
    {
        (IDisposable)P.Dispose();
    }
}

Whereas it should look like this:

1
2
3
4
5
6
7
8
9
public sealed class C : IDisposable
{
    public object D { get; } = new Disposable();

    public void Dispose()
    {
        ((IDisposable)P).Dispose(); // Note the extra parentheses.
    }
}

There is an equivalent issue with IAsyncDisposable.

I’ve worked with Roslyn in the past and have a lot of experience of working with syntax trees (I designed a fairly decent auto-complete engine for IronPython a few years ago and spent some time working on the IronPython parser itself to help upgrade it to Python 3.4). So not only was I interested in the project in general, but I like this kind of programming.

I started by writing a failing test case and this allowed me to quickly find the defective piece of code. The fix was straightforward, all I needed to do was tell Roslyn to encase the CastExpression with a ParenthesisExpression.

Having done this the code fix worked, but the test output did not exactly match the expectation due to whitespace issues. I spent the next half hour messing around with the tests to get them working (it was something to do with the strings containing the expected code that I never quite got to the bottom of) and then added a couple of tests for the IAsyncDisposable case.

The PR can be found here

Update

Boom! The pull request got accepted first time.

Three down, 27 to go 😄.