Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDE0031 false positive when #if DEBUG is used #65880

Open
elachlan opened this issue Oct 6, 2022 · 4 comments
Open

IDE0031 false positive when #if DEBUG is used #65880

elachlan opened this issue Oct 6, 2022 · 4 comments
Assignees
Milestone

Comments

@elachlan
Copy link
Contributor

elachlan commented Oct 6, 2022

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 dotnet/winforms#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
Copy link
Contributor Author

elachlan commented Oct 6, 2022

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
Copy link
Member

Youssef1313 commented Oct 10, 2022

@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
Copy link
Member

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

@CyrusNajmabadi
Copy link
Member

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

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 8, 2022
@CyrusNajmabadi CyrusNajmabadi transferred this issue from dotnet/roslyn-analyzers Dec 8, 2022
@arkalyanms arkalyanms added this to the 18.0 milestone Aug 15, 2023
@arkalyanms arkalyanms removed the untriaged Issues and PRs which have not yet been triaged by a lead label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants