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

[Youssef1313]

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

[Youssef1313]

@PathogenDavid Opened it here based on @mavasani's comment https://github.com/dotnet/roslyn-analyzers/issues/4899#issuecomment-790801753.

[PathogenDavid]

@Youssef1313 I would've appreciated the opportunity to move the issue and/or respond to @mavasani myself.

---

I didn't comment on the original issue to avoid derailing it into a style debate, but:

In these cases I think it's clearer to leave an empty line before the opening brace. This should satisfy the analyzer, keep the intentional braces as them, and also make the code more clear.

Personally speaking, if this is how the analyzer is implemented it's going to be disabled in my codebases. As I mentioned in the original issue, it conflicts a pretty common pattern in codebases I work in and would lead to too many false-positives.

I feel like analyzers are best suited for things which appear to be a mistake, but this design shifts things into code style preference territory. (Which I suppose is why Manish wanted the issue moved here, although that was not made explicitly clear.)

[CyrusNajmabadi]

I believe the compiler already warns about thsi for other types of constructs (maybe if/while loops?) I'd just make thsi a compiler warning (wave?)

[Youssef1313]

@PathogenDavid If this is going to be an analyzer, it could be made configurable to fulfill different needs.

@CyrusNajmabadi That sounds reasonable to extend the cases of CS0642 in a warning wave.

[Youssef1313]

Not sure if this should be moved to Area-Compilers to look at this as a warning wave candidate as suggested by @CyrusNajmabadi ?

cc: @jaredpar