Fix returning valid pp symbol info on unrelated preprocessor directives #72910

Rekkonnect · 2024-04-06T22:26:10Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>

AlekseyTs · 2024-04-12T15:51:59Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

            return PreprocessingSymbolInfo.None;
+
+            static bool isPreprocessorDirectiveAcceptingPreprocessingSymbols(SyntaxKind kind)
+                => kind is SyntaxKind.IfDirectiveTrivia or SyntaxKind.ElifDirectiveTrivia or SyntaxKind.DefineDirectiveTrivia or SyntaxKind.UndefDirectiveTrivia;


SyntaxKind.DefineDirectiveTrivia or SyntaxKind.UndefDirectiveTrivia

Are these using IdentifierNameSyntax to refer to preprocessing symbol? #Closed

No, these contain a SyntaxToken Name as the name of the preprocessing symbol

AlekseyTs · 2024-04-12T15:54:34Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

            CheckSyntaxNode(node);

-            if (node.Ancestors().Any(n => SyntaxFacts.IsPreprocessorDirective(n.Kind())))
+            if (node.Ancestors().Any(n => isPreprocessorDirectiveAcceptingPreprocessingSymbols(n.Kind())))


node.Ancestors().Any(n => isPreprocessorDirectiveAcceptingPreprocessingSymbols(n.Kind()))

I think it would be good to make this condition stronger and verify that the IdentifierNameSyntax is actually coming from expression part of the directive. #Closed

This should come with additional testing, of course.

I'm not sure on how to test this additional condition, the directive trivia only have Condition as the only expression that may contain the IdentifierNameSyntax that is evaluated.

I'm not sure on how to test this additional condition,

I am thinking of a check like the following:

Get node's parent, but do not ascend out of trivia

if parent is IfDirectiveTriviaSyntax _if , return node == _if.Condition

if parent is ElifDirectiveTrivia _elif , return node == _elif.Condition

If parent is ExpressionSyntax, let node be parent and repeat from step 1.

return false

If it is valid for IfDirectiveTriviaSyntax.Condition/ ElifDirectiveTriviaSyntax.Condition to contain non expression nodes, which are not trivia themselves, then perhaps the check could be something like:

Ascend to the first directive, but do not ascend out of trivia

Return true if the directive is IfDirectiveTriviaSyntax/ ElifDirectiveTriviaSyntax and the starting node is within IfDirectiveTriviaSyntax.Condition/ ElifDirectiveTriviaSyntax.Condition.

src/Compilers/CSharp/Test/Symbol/Compilation/GetSemanticInfoTests.cs

AlekseyTs · 2024-04-12T16:00:25Z

Done with review pass (commit 3) #Closed

Rekkonnect · 2025-10-30T09:15:29Z

@AlekseyTs could you ptal?

AlekseyTs · 2025-10-30T18:48:18Z

src/Compilers/CSharp/Test/Symbol/Compilation/GetSemanticInfoTests.cs

+                var position = sourceCode.IndexOf(substringToMatch, StringComparison.Ordinal);
+                var nameSyntaxToBind = tree.GetRoot().FindToken(position, findInsideTrivia: true).Parent as IdentifierNameSyntax;
+
+                if (nameSyntaxToBind is null)


if (nameSyntaxToBind is null)

This makes the test fragile. Someone can make a mistake on the previous statement and the test will silently stop testing the target scenario. Since the API takes IdentifierNameSyntax, we are not really interested in calling this method for other situations. #Closed

Changed to only test against IdentifierNameSyntax

AlekseyTs · 2025-10-30T18:59:16Z

src/Compilers/CSharp/Test/Symbol/Compilation/GetSemanticInfoTests.cs

        }

+        [Fact, WorkItem(72907, "https://github.com/dotnet/roslyn/issues/72907")]
+        public void GetPreprocessingSymbolInfoOnUnrelatedDirective()


GetPreprocessingSymbolInfoOnUnrelatedDirective

It looks like no tests were added for positive scenarios #Closed

Other tests already cover the positive scenarios, that's why this test didn't focus on them. I will add the assertion for #define which we expect to return a non-null symbol

AlekseyTs · 2025-10-30T19:46:46Z

Done with review pass (commit 5) #Closed

Rekkonnect · 2025-10-31T19:42:27Z

@AlekseyTs changed the test, ptal

AlekseyTs · 2025-11-03T16:10:06Z

Done with review pass (commit 6). It looks like #72910 (comment) thread is still active. #Closed

Rekkonnect · 2025-11-03T16:34:47Z

It looks to me like it's outdated, the code currently actually checks whether the expression node is within the condition of the if/elif statements

AlekseyTs · 2025-11-03T16:55:56Z

It looks to me like it's outdated, the code currently actually checks whether the expression node is within the condition of the if/elif statements

I added comments to that thread several days ago, and I think they are still relevant. For example, the current check doesn't stop at the immediate containing directive. The behavior around ascending out of trivia is not clear as well. #Closed

Rekkonnect · 2025-11-04T23:05:44Z

Changed the code, could you ptal? @AlekseyTs

AlekseyTs · 2025-11-05T15:59:10Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+                    return false;
+                }
+
+                return isPreprocessingSymbolIdentifierContainer(parentNode.Parent);


return isPreprocessingSymbolIdentifierContainer(parentNode.Parent);

It looks like we could use a while loop instead of a recursion. #Closed

AlekseyTs · 2025-11-05T16:04:23Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs


            return PreprocessingSymbolInfo.None;
+
+            bool isPreprocessingSymbolIdentifierContainer(SyntaxNode parentNode)


isPreprocessingSymbolIdentifierContainer

"isPossiblePreprocessingSymbolReference"? #Closed

AlekseyTs · 2025-11-05T16:06:08Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs


            return PreprocessingSymbolInfo.None;
+
+            bool isPreprocessingSymbolIdentifierContainer(SyntaxNode parentNode)


SyntaxNode parentNode

Consider passing IdentifierNameSyntax node instead and using a loop through the Parent chain instead of a recursion. #Closed

AlekseyTs · 2025-11-05T16:07:22Z

Done with review pass (commit 7) #Closed

Rekkonnect · 2025-11-06T09:22:31Z

Applied suggestions, ptal

AlekseyTs

LGTM (commit 8)

AlekseyTs · 2025-11-06T14:13:46Z

@dotnet/roslyn-compiler For a second review

AlekseyTs · 2025-11-07T13:50:07Z

@dotnet/roslyn-compiler For a second review on a community PR

AlekseyTs · 2025-11-10T17:16:33Z

@dotnet/roslyn-compiler For a second review on a community PR

AlekseyTs · 2025-11-11T14:16:44Z

@dotnet/roslyn-compiler For a second review on a community PR

CyrusNajmabadi · 2025-11-11T14:27:11Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+                var parentNode = node.Parent;
+                while (parentNode is not null)
+                {
+                    var kind = parentNode.Kind();
+                    switch (kind)
+                    {
+                        case SyntaxKind.IfDirectiveTrivia:
+                            {
+                                var parentIf = (IfDirectiveTriviaSyntax)parentNode;
+                                return parentIf.Condition.FullSpan.Contains(node.FullSpan);
+                            }
+                        case SyntaxKind.ElifDirectiveTrivia:
+                            {
+                                var parentElif = (ElifDirectiveTriviaSyntax)parentNode;
+                                return parentElif.Condition.FullSpan.Contains(node.FullSpan);
+                            }
+                    }
+
+                    if (SyntaxFacts.IsPreprocessorDirective(kind))
+                    {
+                        return false;
+                    }
+
+                    parentNode = parentNode.Parent;
+                }
+                return false;


Suggested change

var parentNode = node.Parent;

while (parentNode is not null)

{

var kind = parentNode.Kind();

switch (kind)

{

case SyntaxKind.IfDirectiveTrivia:

{

var parentIf = (IfDirectiveTriviaSyntax)parentNode;

return parentIf.Condition.FullSpan.Contains(node.FullSpan);

}

case SyntaxKind.ElifDirectiveTrivia:

{

var parentElif = (ElifDirectiveTriviaSyntax)parentNode;

return parentElif.Condition.FullSpan.Contains(node.FullSpan);

}

}

if (SyntaxFacts.IsPreprocessorDirective(kind))

{

return false;

}

parentNode = parentNode.Parent;

}

return false;

for (var parentNode = node.Parent; parentNode != null; parentNode = parentNode.Parent)

{

var kind = parentNode.Kind();

switch (kind)

{

case SyntaxKind.IfDirectiveTrivia:

{

var parentIf = (IfDirectiveTriviaSyntax)parentNode;

return parentIf.Condition.FullSpan.Contains(node.FullSpan);

}

case SyntaxKind.ElifDirectiveTrivia:

{

var parentElif = (ElifDirectiveTriviaSyntax)parentNode;

return parentElif.Condition.FullSpan.Contains(node.FullSpan);

}

}

if (SyntaxFacts.IsPreprocessorDirective(kind))

{

return false;

}

}

return false;

CyrusNajmabadi

approving. seems simpelr as a for-loop.

AlekseyTs · 2025-11-11T19:36:38Z

@Rekkonnect Thanks for the contribution.

Fix returning pp symbol info on invalid directive

e55af0a

Rekkonnect requested a review from a team as a code owner April 6, 2024 22:26

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 6, 2024

dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 6, 2024

CyrusNajmabadi reviewed Apr 7, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs Show resolved Hide resolved

Rekkonnect and others added 2 commits April 7, 2024 13:52

Apply styling suggestion

e1a0252

Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>

Fix broken suggestion

91b37b1

AlekseyTs reviewed Apr 12, 2024

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Compilation/GetSemanticInfoTests.cs Outdated Show resolved Hide resolved

Apply suggestions

40380ba

Rekkonnect requested a review from AlekseyTs April 12, 2024 23:14

jaredpar added this to the Backlog milestone Jul 9, 2024

Rekkonnect mentioned this pull request Oct 19, 2024

Implement find all references to preprocessing symbols #66425

Merged

Merge branch 'main' into fix/72907

e4bb78b

AlekseyTs reviewed Oct 30, 2025

View reviewed changes

focus tests on identifier names

1bccefa

apply suggestions

fbcc66d

AlekseyTs reviewed Nov 5, 2025

View reviewed changes

apply suggestions

760e82b

AlekseyTs approved these changes Nov 6, 2025

View reviewed changes

AlekseyTs requested a review from a team November 6, 2025 14:14

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

CyrusNajmabadi approved these changes Nov 11, 2025

View reviewed changes

AlekseyTs merged commit 5afb7a1 into dotnet:main Nov 11, 2025
25 checks passed

dotnet-policy-service bot modified the milestones: Backlog, Next Nov 11, 2025

dotnet-bot mentioned this pull request Nov 14, 2025

[Automated] PRs inserted in VS build main-11213.22 #81230

Closed


		return PreprocessingSymbolInfo.None;

		bool isPreprocessingSymbolIdentifierContainer(SyntaxNode parentNode)

Fix returning valid pp symbol info on unrelated preprocessor directives #72910

Fix returning valid pp symbol info on unrelated preprocessor directives #72910

Conversation

Rekkonnect commented Apr 6, 2024

Uh oh!

Uh oh!

AlekseyTs Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rekkonnect Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

Rekkonnect Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlekseyTs commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rekkonnect commented Oct 30, 2025

Uh oh!

AlekseyTs Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rekkonnect Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rekkonnect Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rekkonnect commented Oct 31, 2025

Uh oh!

AlekseyTs commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rekkonnect commented Nov 3, 2025

Uh oh!

AlekseyTs commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rekkonnect commented Nov 4, 2025

Uh oh!

AlekseyTs Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 12, 2024 •

edited

Loading

AlekseyTs Apr 12, 2024 •

edited

Loading

AlekseyTs Oct 30, 2025 •

edited

Loading

AlekseyTs commented Apr 12, 2024 •

edited

Loading

AlekseyTs Oct 30, 2025 •

edited

Loading

AlekseyTs Oct 30, 2025 •

edited

Loading

AlekseyTs commented Oct 30, 2025 •

edited

Loading

AlekseyTs commented Nov 3, 2025 •

edited

Loading

AlekseyTs commented Nov 3, 2025 •

edited

Loading

AlekseyTs Nov 5, 2025 •

edited

Loading

AlekseyTs Nov 5, 2025 •

edited

Loading

AlekseyTs Nov 5, 2025 •

edited

Loading

AlekseyTs commented Nov 5, 2025 •

edited

Loading