Fix DeclaringSyntaxReferences returning empty for VB catch variables by Copilot · Pull Request #81151 · dotnet/roslyn

Copilot · 2025-11-10T23:41:34Z

Plan: Fix DeclaringSyntaxReferences for Catch Variables ✅

Explore repository and understand the issue
Locate the bug in VariableLocalSymbol.DeclaringSyntaxReferences
Write a test to demonstrate the issue
Fix the bug in LocalSymbol.vb
Verify the fix passes the test
Run broader tests to ensure no regressions
Address code review feedback: Add Debug.Assert for catch variable
Address code review feedback: Remove redundant test comments
Address code review feedback: Simplify test assertions

Summary

Fixed an issue where DeclaringSyntaxReferences returned an empty list for catch statement variables in Visual Basic.

Solution

Modified the VariableLocalSymbol.DeclaringSyntaxReferences property in LocalSymbol.vb to fall back to using _identifierToken.Parent.GetReference() when _modifiedIdentifierOpt is Nothing.

Code Review Response

Investigated all usages of VariableLocalSymbol creation and confirmed that only LocalDeclarationKind.Catch passes Nothing as modifiedIdentifierOpt. Added Debug.Assert(DeclarationKind = LocalDeclarationKind.Catch) to enforce this invariant.
Removed redundant comments from test that restated what assertions were doing.
Simplified test assertions by using .Single() instead of checking length and indexing, and removed redundant NotNull check since IsType already implies not null.

Analysis of all LocalDeclarationKind cases:

Catch: Uses Nothing for modifiedIdentifierOpt ✓
Variable, Constant, Static: Always provide ModifiedIdentifierSyntax
Using: Always provides ModifiedIdentifierSyntax
For, ForEach: Always provide ModifiedIdentifierSyntax
ImplicitVariable, FunctionValue: Use different LocalSymbol types (SourceLocalSymbol), not VariableLocalSymbol

Testing Results ✅

✅ All 15 DeclaringSyntaxNodeTests pass
✅ All 65 catch-related tests pass
✅ Debug assertion does not fire in any tests

Original prompt

This section details on the original issue you should resolve

<issue_title>DeclaringSyntaxReferences returns empty list on the underlying ILocalSymbol of a catch statement's variable</issue_title>
<issue_description>In VB.Net with the following input:
Try
  '...
Catch exc As Exception
  Console.WriteLine(exc.ToString())
End Try
when I query the DeclaringSyntaxReferences of the local symbol of exc, an empty list is returned.

The problem seems to be that VariableLocalSymbol expects to have a ModifiedIdentifierSyntax, but in case of the catch statement it has an IdentifierNameSyntax, so it returns empty list: http://source.roslyn.codeplex.com/#Microsoft.CodeAnalysis.VisualBasic/Symbols/Source/LocalSymbol.vb,44cfe5cfa177deb7,references

If I do the same in C#, I get the reference without a problem.
</issue_description>

<agent_instructions>@copilot Write a test to see if there's still a problem here. If not, the test will suffice to close out the issue. Otherwise, see if there is a simple issue in LocalSymbol.vb and attempt a targeted fix that then makes the test succeed.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes DeclaringSyntaxReferences returns empty list on the underlying ILocalSymbol of a catch statement's variable #6209

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

CyrusNajmabadi · 2025-11-11T00:02:50Z

src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb

                                Return ImmutableArray.Create(Of SyntaxReference)(_modifiedIdentifierOpt.GetReference())
                            Else
-                                Return ImmutableArray(Of SyntaxReference).Empty
+                                ' For catch variables (and potentially other cases), there is no ModifiedIdentifierSyntax.


@copilot determine what other cases there may be that hit this. if there are none, assert that this is a catch variable. If tehre are others, enumerate them here and add tests.

Investigated all usages of VariableLocalSymbol and confirmed that only LocalDeclarationKind.Catch passes Nothing for modifiedIdentifierOpt. Added Debug.Assert(DeclarationKind = LocalDeclarationKind.Catch) in commit 8d731e6.

Analysis:

Catch: Passes Nothing in CatchBlockBinder.vb:55

Variable, Constant, Static, Using: Always provide ModifiedIdentifierSyntax

For, ForEach: Always provide ModifiedIdentifierSyntax

ImplicitVariable, FunctionValue: Use SourceLocalSymbol (different type), not VariableLocalSymbol

All tests pass with the assertion.

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb

CyrusNajmabadi · 2025-11-11T00:31:20Z

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb

+            Dim token = tree.GetCompilationUnitRoot().FindToken(position)
+            Dim identifierName = token.Parent
+
+            ' Get the symbol for the catch variable


@copilot no need to add test comments that just restate what the test is already doing.

Removed redundant comments in commit 8566ad4.

CyrusNajmabadi · 2025-11-11T00:32:45Z

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb

+
+            ' Check that DeclaringSyntaxReferences returns a non-empty list
+            Dim declaringRefs = localSymbol.DeclaringSyntaxReferences
+            Assert.Equal(1, declaringRefs.Length)


@copilot remove unneeded asserts. Instead, use declaringRefs.Single(). That way it will crash if we don't have exactly one item. Similarly, if we have an IsType check, we don't need a NotNull check.

Simplified assertions in commit 6e0ec0c:

Used .Single() instead of checking length and indexing

Removed redundant Assert.NotNull(declaredSyntax) since Assert.IsType already implies not null

…ts.vb

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb

…ts.vb

CyrusNajmabadi · 2025-11-11T20:44:41Z

@dotnet/roslyn-compiler this is ready for review.

AlekseyTs · 2025-11-12T14:56:11Z

src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb

@@ -928,7 +928,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
                            If _modifiedIdentifierOpt IsNot Nothing Then


If _modifiedIdentifierOpt IsNot Nothing Then

It looks like the first case is unreachable and the entire method implementation could be:

If _modifiedIdentifierOpt IsNot Nothing Then Return ImmutableArray.Create(Of SyntaxReference)(_modifiedIdentifierOpt.GetReference()) Else Return MyBase.DeclaringSyntaxReferences End If ``` #Closed

AlekseyTs · 2025-11-12T14:59:36Z

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb

+            Dim code As String = tree.GetText().ToString()
+            Dim position As Integer = code.IndexOf("exc", StringComparison.Ordinal)
+            Dim token = tree.GetCompilationUnitRoot().FindToken(position)
+            Dim identifierName = token.Parent


Dim identifierName = token.Parent

This calculation looks unnecessary complicated. We simply could traverse the syntax tree for the first IdentifierNameSyntax with the right identifier value. #Closed

AlekseyTs · 2025-11-12T15:03:07Z

Done with review pass (commit 7) #Closed

…r-catch-variable-symbol

AlekseyTs · 2025-11-13T14:18:22Z

src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb

+                    If _modifiedIdentifierOpt IsNot Nothing Then
+                        Return ImmutableArray.Create(_modifiedIdentifierOpt.GetReference())
+                    Else
+                        Debug.Assert(DeclarationKind = LocalDeclarationKind.Catch, "Only catch variables should have Nothing for _modifiedIdentifierOpt")


Debug.Assert(DeclarationKind = LocalDeclarationKind.Catch, "Only catch variables should have Nothing for _modifiedIdentifierOpt")

If we think that this assert is valuable to have, I think it belongs in constructor instead. #Closed

@copilot please move this assert to the constructor. Specifically, that either _modifiedIdentifierOpt is non-nothing, or DeclarationKind is LocalDeclarationKind.Catch.

AlekseyTs · 2025-11-13T14:20:15Z

Done with review pass (commit 10) #Closed

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb

AlekseyTs

LGTM (commit 12), assuming CI is passing

Initial plan

4a79f87

Copilot AI assigned Copilot and CyrusNajmabadi Nov 10, 2025

Copilot started work on behalf of CyrusNajmabadi November 10, 2025 23:42 View session

Add test and fix for catch variable DeclaringSyntaxReferences issue

233cdab

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Add test for DeclaringSyntaxReferences in catch statement~~ Fix DeclaringSyntaxReferences returning empty for VB catch variables Nov 10, 2025

Copilot AI requested a review from CyrusNajmabadi November 10, 2025 23:55

Copilot finished work on behalf of CyrusNajmabadi November 10, 2025 23:55

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

Copilot started work on behalf of CyrusNajmabadi November 11, 2025 00:03 View session

Add Debug.Assert for catch variable in DeclaringSyntaxReferences

8d731e6

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

Copilot AI requested a review from CyrusNajmabadi November 11, 2025 00:13

Copilot finished work on behalf of CyrusNajmabadi November 11, 2025 00:13

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

Copilot started work on behalf of CyrusNajmabadi November 11, 2025 00:31 View session

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

CyrusNajmabadi and others added 2 commits November 11, 2025 01:32

Update src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTes…

7cc0087

…ts.vb

Remove redundant comments from test

8566ad4

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

Copilot AI requested a review from CyrusNajmabadi November 11, 2025 00:38

Copilot finished work on behalf of CyrusNajmabadi November 11, 2025 00:38

Copilot started work on behalf of CyrusNajmabadi November 11, 2025 00:39 View session

Simplify test assertions using Single() and removing redundant checks

6e0ec0c

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

Copilot finished work on behalf of CyrusNajmabadi November 11, 2025 00:46

CyrusNajmabadi reviewed Nov 11, 2025

View reviewed changes

src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTests.vb Outdated Show resolved Hide resolved

Update src/Compilers/VisualBasic/Test/Semantic/DeclaringSyntaxNodeTes…

62d2a8d

…ts.vb

CyrusNajmabadi marked this pull request as ready for review November 11, 2025 11:40

CyrusNajmabadi requested a review from a team as a code owner November 11, 2025 11:40

CyrusNajmabadi added the Area-Compilers label Nov 11, 2025

AlekseyTs reviewed Nov 12, 2025

View reviewed changes

CyrusNajmabadi added 3 commits November 12, 2025 17:27

Merge remote-tracking branch 'upstream/main' into copilot/add-test-fo…

9dfef11

…r-catch-variable-symbol

Simplify impl

f8e1b03

Simplify test

c3ecbf0

RikkiGibson approved these changes Nov 13, 2025

View reviewed changes

CyrusNajmabadi requested a review from AlekseyTs November 13, 2025 08:09

AlekseyTs reviewed Nov 13, 2025

View reviewed changes

Copilot started work on behalf of CyrusNajmabadi November 13, 2025 15:20 View session

Changes before error encountered

ebafd74

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>

Copilot AI requested a review from CyrusNajmabadi November 13, 2025 15:35

CyrusNajmabadi reviewed Nov 13, 2025

View reviewed changes

src/Compilers/VisualBasic/Portable/Symbols/Source/LocalSymbol.vb Outdated Show resolved Hide resolved

Apply suggestion from @CyrusNajmabadi

90c69db

AlekseyTs approved these changes Nov 13, 2025

View reviewed changes

CyrusNajmabadi enabled auto-merge (squash) November 13, 2025 16:52

CyrusNajmabadi merged commit fc6092f into main Nov 13, 2025
24 of 25 checks passed

dotnet-policy-service bot added this to the Next milestone Nov 13, 2025

This was referenced Nov 13, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

[android-arm64] The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#6408

Open

dotnet-bot mentioned this pull request Nov 15, 2025

[Automated] PRs inserted in VS build main-11215.03 #81262

Closed

CyrusNajmabadi deleted the copilot/add-test-for-catch-variable-symbol branch December 2, 2025 11:52

dotnet-maestro bot mentioned this pull request Dec 18, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3957

Merged

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026

		@@ -928,7 +928,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
		If _modifiedIdentifierOpt IsNot Nothing Then

Conversation

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

Uh oh!

Plan: Fix DeclaringSyntaxReferences for Catch Variables ✅

Summary

Solution

Code Review Response

Testing Results ✅

Comments on the Issue (you are @copilot in this section)

Uh oh!

CyrusNajmabadi Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Nov 11, 2025

Uh oh!

AlekseyTs Nov 12, 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!

CyrusNajmabadi Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 12, 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!

CyrusNajmabadi Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs Nov 13, 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!

CyrusNajmabadi Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Copilot AI commented Nov 10, 2025 •

edited

Loading

AlekseyTs Nov 12, 2025 •

edited

Loading

AlekseyTs Nov 12, 2025 •

edited

Loading

AlekseyTs commented Nov 12, 2025 •

edited

Loading

AlekseyTs Nov 13, 2025 •

edited

Loading

AlekseyTs commented Nov 13, 2025 •

edited

Loading