Fix IDE0031 false positive when preprocessor directives are used in if statements #80878
Conversation
- Make AnalyzeIfStatement virtual in AbstractUseNullPropagationDiagnosticAnalyzer - Override in CSharpUseNullPropagationDiagnosticAnalyzer to check for directives on block close brace - Add tests for preprocessor directive scenarios Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
This is more conservative and handles cases where directives appear anywhere in the block, not just at the end. Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| """, | ||
| languageVersion: LanguageVersion.CSharp14); | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77777")] |
| if (ifStatement.Statement is BlockSyntax block) | ||
| { | ||
| if (block.ContainsDirectives) | ||
| return null; | ||
| } |
There was a problem hiding this comment.
@copilot actually you can just check if it .ContainsDirectives, regardless of whether or not it is a block. And that means this check can move into the base class, and you can avoid needing the virtual/override.
Also, please add a test where the if-statement doesn't have a block, but just a single-statement with a #if/#else/#endif..
There was a problem hiding this comment.
Moved the directive check to the base class and simplified the implementation in commit 2654915. The check now uses ifStatement.ContainsDirectives directly without needing virtual/override. Also added a test for single-statement if with #if/#else/#endif.
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
- Remove virtual/override approach - Check ContainsDirectives on if-statement directly in base class - Add test for single-statement if with preprocessor directives Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
@copilot looks like this broke a bunch of tests in UseNullPropagationTests. Take a look at what broke and what we were expecting to work from before. And use that to redo this PR with a correct fix that doesn't break existing working scenarios, btu blocks the new scenarios we want blocked. |
…tement The previous check was too broad - checking ifStatement.ContainsDirectives would fail for directives anywhere in the if-statement including the condition. Now we check only the statements that would be transformed (trueStatement and nullAssignmentOpt). Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Fixed in commit ad5bc14. The issue was that the check was too broad - Now the check only examines the statements that would actually be transformed ( |
* upstream/main: (332 commits) Cache lambdas in analyzer driver (dotnet#80759) Add information for NuGet package version 4.14 (dotnet#80870) Add missing search keywords to VB Advanced options page Fix IDE0031 false positive when preprocessor directives are used in if statements (dotnet#80878) Use core compiler on netfx hosts with toolset package (dotnet#80631) Make string concat assert more precise (dotnet#80619) Extensions: address some diagnostic quality issues (dotnet#80827) Add note on traversal order for bound nodes (dotnet#80872) Ensure that locals at the top level of a constructor have the same safe-context as parameters (dotnet#80807) Fix handling of SymbolDisplayCompilerInternalOptions.UseArityForGenericTypes option for non-native symbol implementations (dotnet#80826) Update src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests.cs Add IsValidContainingStatement check to prevent collection initializers in using declarations Add back old DocumentSpan constructor (dotnet#80864) Add tests verifying pointer types in type parameters require unsafe context (dotnet#80776) Add regression test for Interlocked.Exchange with nullable types (dotnet#80796) Add regression test for ParseAttributeArgumentList with invalid input (fixes dotnet#8699) (dotnet#80705) Add regression test for compiler crash with syntax error in indexer declaration (dotnet#80772) Add runtime NullReferenceException validation to foreach null iteration tests (dotnet#80839) Update MicrosoftBuildTasksCoreVersionForMetrics to 17.11.48 (dotnet#80812) Mark CS4009 error as a "build only" error. (dotnet#80698) ...
Fix IDE0031 false positive when
#if DEBUGis usedAnalyzeIfStatementvirtual inAbstractUseNullPropagationDiagnosticAnalyzer_IfStatement.csAnalyzeIfStatementinCSharpUseNullPropagationDiagnosticAnalyzer.csto check for directives in block#if DEBUGstatementsSummary
This PR fixes IDE0031 (Use null propagation) false positive when preprocessor directives like
#if DEBUGare present in the body of an if statement.The analyzer now checks if the statements that would be transformed contain any preprocessor directives. If directives are found in the statement body, the transformation is skipped to avoid incorrect code transformations where conditional compilation directives would be removed or execute unconditionally.
Changes
AnalyzeIfStatementmethodContainsDirectiveson the actual statements being transformed (not the entire if-statement)Fixes #65880
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.