dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

IDE0031 false positive when `#if DEBUG` is used #65880

Open elachlan opened 2 years ago

elachlan commented 2 years ago

Analyzer

Diagnostic ID: [IDE0031](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0031: Use null propagation

Analyzer source

.NET7

Describe the bug

Stumbled upon in https://github.com/dotnet/winforms/pull/7902

The analyzer doesn't take into account #if DEBUG statements. Inside visual studio it doesn't suggest a code fix. But in CI build it fails.

public void Dispose()
{
    if (_controlToLayout is not null)
    {
        _controlToLayout.ResumeLayout(_resumeLayout);

#if DEBUG
        Debug.Assert(_controlToLayout.LayoutSuspendCount == _layoutSuspendCount, "Suspend/Resume layout mismatch!");
#endif
    }
}

Steps To Reproduce

Enable the analyzer as a warning or error and build a solution with a null check and a #if DEBUG statement.

Expected behavior

No build failure.

Actual behavior

Build Failure.

Additional context

This is blocking winforms uptake of the rule.

elachlan commented 2 years ago

I guess if your a building a release version then #if DEBUG isn't hit so it causes a positive identification. Would Roslyn understand this context? Should we just add a suppression?

Youssef1313 commented 2 years ago

@elachlan IDExxxx diagnostics are in dotnet/roslyn, not dotnet/roslyn-analyzers.

This is generally by-design, and problems similar to this are applicable to all analyzers, not only IDE0031. (for example, consider a "make method static" analyzer that hits a method that can be made static only under a certain configuration)

However, for this specific case, I think a syntactic check for #if might make sense. In general, skipping the analysis for #ifs might have lots of false negatives, but I don't think this is the case for IDE0031.

Tagging @mavasani @CyrusNajmabadi for thoughts and for moving to dotnet/roslyn

Youssef1313 commented 1 year ago

@mavasani @CyrusNajmabadi Could you move to dotnet/roslyn, or close if by-design?

CyrusNajmabadi commented 1 year ago

Moving to roslyn, we should check for this case in that fixer and not offer it.