dotnet/roslyn

Rule suggestion: Warn if a using declaration is followed immediately by a loose block

Open

#51,668 opened on Mar 4, 2021

View on GitHub
 (5 comments) (1 reaction) (0 assignees)C# (4,257 forks)batch import
Area-AnalyzersConcept-Continuous Improvementhelp wanted

Repository metrics

Stars
 (20,414 stars)
PR merge metrics
 (Avg merge 6d 17h) (256 merged PRs in 30d)

Description

This rule is inspired by a dumb mistake I made recently that I thought might make a good candidate for an analyzer.

Describe the problem you are trying to solve

At a glance, the following (semi-contrived) example looks correct and sensible. The intent is that the StreamWriter is disposed on the line marked with <--.

static void TestFunction()
{
    using StreamWriter f = new("HelloWorld.txt");
    {
        f.WriteLine("Hello, world!");
    } // <--

    Console.WriteLine(File.ReadAllText("HelloWorld.txt"));
}

However, the author accidentally used a using declaration where they clearly intended to use a using statement. As such the StreamWriter is disposed at the end of TestFunction, which causes the ReadAllText to fail at runtime due to the file still being locked.

They should've written this instead:

static void TestFunction()
{
    using (StreamWriter f = new("HelloWorld.txt"))
    {
        f.WriteLine("Hello, world!");
    } // <--

    Console.WriteLine(File.ReadAllText("HelloWorld.txt"));
}

Describe suggestions on how to achieve the rule

If one or more sequence of LocalDeclarationStatements representing using declarations are followed immediately by a Block, the rule fails. (Potentially with a heuristic involving the trivia involved to avoid cases where the block is more likely for code organization and just happens to follow a using declaration.)

The suggested code fix would be to rewrite the using declarations + block into a UsingStatement (as shown above.)

Additional context

Obviously the "wrong" example above is technically valid C#, but I couldn't think of a real scenario where someone would want code shaped that way without the intent being to use a using statement.

I also felt like this would be really easy to overlook in a code review in a scenario where the important of the using scope was less important and wouldn't necessarily blow up immediately at runtime.

In my case, it was also a non-trivial custom type rather than StreamWriter so I accidentally went down a rabbit hole of trying to debug my Dispose implementation. (I probably would've noticed the issue sooner if it was just StreamWriter.)

Definitely open to hear I'm just bad though 😅

Originally posted by @PathogenDavid in https://github.com/dotnet/roslyn-analyzers/issues/4899#issue-816115290

Contributor guide