Improve error recovery around 'scoped' modifier parsing by CyrusNajmabadi · Pull Request #81636 · dotnet/roslyn

CyrusNajmabadi · 2025-12-10T13:26:23Z

We now allow a misplacced 'scoped' keyword to be treated as a normal modifier, and move teh reporting of issues to the binding phase. Today we get entirely thrown off and enter very bad error recovery that often gets thrown entirely off, creating tons of cascading errors.

Note: the net effect of this is much fewer errors, and much better error recovery. The added length in this PR is the addition of new tests, or updating existing tests to report binding errors to demonstrate that errors are still produced semantically where they would have previously been syntactic.

CyrusNajmabadi · 2025-12-10T14:35:23Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                return true;

-            using var beforeScopedResetPoint = this.GetDisposableResetPoint(resetOnDispose: false);
+            using var beforeScopedResetPoint = this.GetDisposableResetPoint(resetOnDispose: true);


no need to for dual reset points. and we can always trivially clean up. we just let the caller then eat the token if this function says it is a keyword.

CyrusNajmabadi · 2025-12-10T14:36:27Z

src/Compilers/CSharp/Test/Emit3/RefReadonlyParameterTests.cs

-            Diagnostic(ErrorCode.ERR_IdentifierExpected, "readonly").WithLocation(3, 37),
-            // (3,37): error CS1003: Syntax error, ',' expected
-            //     public static void M(ref scoped readonly int p) => throw null;
-            Diagnostic(ErrorCode.ERR_SyntaxError, "readonly").WithArguments(",").WithLocation(3, 37),


lots of the PR will just be syntax errors going away, and better errors about misplaced 'scopes' modifiers.

CyrusNajmabadi · 2025-12-10T14:36:45Z

src/Compilers/CSharp/Test/Emit3/RefReadonlyParameterTests.cs

-            Diagnostic(ErrorCode.ERR_RefReadOnlyWrongOrdering, "readonly").WithLocation(3, 26),
-            // (3,35): error CS0246: The type or namespace name 'scoped' could not be found (are you missing a using directive or an assembly reference?)
-            //     public static void M(readonly scoped ref int p) => throw null;
-            Diagnostic(ErrorCode.ERR_SingleTypeNameNotFound, "scoped").WithArguments("scoped").WithLocation(3, 35),


or removing errors where we considered it a type name, but now realie it should be treated as a modifier.

CyrusNajmabadi · 2025-12-10T14:37:59Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

+                    Diagnostic(ErrorCode.ERR_ScopedAfterInOutRefReadonly, "scoped").WithLocation(1, 29),
+                    // (1,47): error CS8936: Feature 'ref fields' is not available in C# 10.0. Please use language version 11.0 or greater.
+                    // void F(ref scoped int b, in scoped int c, out scoped int d) { }
+                    Diagnostic(ErrorCode.ERR_ScopedAfterInOutRefReadonly, "scoped").WithLocation(1, 47)]);


the difference between these two is just the errors about a feature not being available. is tehre a helper to filter those out appropriately to make the test cleaner?

CyrusNajmabadi · 2025-12-10T14:38:26Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

-                // (1,54): error CS1003: Syntax error, ',' expected
-                // void F(ref scoped int b, in scoped int c, out scoped int d) { }
-                Diagnostic(ErrorCode.ERR_SyntaxError, "int").WithArguments(",").WithLocation(1, 54)
-                );


in some tests all syntax diagnostics went away. as such, i added semantic diagnostics to show we still report problems with the scoped modifier.

CyrusNajmabadi · 2025-12-10T14:38:47Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

-                    M(SyntaxKind.CommaToken);
-                    N(SyntaxKind.Parameter);
-                    {
+                        N(SyntaxKind.ScopedKeyword);


these tests demonstrate better understanding, with 'scoped' properly being treated as a modifier.

CyrusNajmabadi · 2025-12-10T14:39:30Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

+                    Diagnostic(ErrorCode.ERR_SemicolonExpected, "").WithLocation(1, 26)]);

-            N(SyntaxKind.ParenthesizedExpression);
+            N(SyntaxKind.ParenthesizedLambdaExpression);


we didn't even realize this was a lambda before...

CyrusNajmabadi · 2025-12-10T14:44:04Z

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

CyrusNajmabadi · 2025-12-10T14:45:00Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs


-        private bool IsPossibleScopedKeyword(bool isFunctionPointerParameter)
-        {
-            using var _ = this.GetDisposableResetPoint(resetOnDispose: true);


for those keeping count, this made for a triple nested reset point. We now just have one reset point.

CyrusNajmabadi · 2025-12-10T14:45:53Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+            // trivial case.  scoped ref/out/in/this  is definitely the scoped keyword. Note: the only actual legal
+            // cases are `scoped ref`, `scoped out`, and `scoped in`.  But we detect and allow `scoped this`, `scoped
+            // params` and `scoped readonly` as well.  These will be reported as errors later in binding.
+            if (IsParameterModifierExcludingScoped(this.CurrentToken))


we intentionally allow a broader set than before. so you can say things like scoped this. This is caught later in the binder.

CyrusNajmabadi · 2025-12-10T17:43:42Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                return false;

-            // In C# 14 we decided that within a lambda 'scoped' would *always* be a keyword.
+            // In C# 14 we decided that within a lambda 'scoped' would *always* be a modifier, not a type.


refining this comment. It wasn't 100% accurate. It's not always a keyword. But it is a modifier if it previous would have been allowed as a type.

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaParameterParsingTests.cs

CyrusNajmabadi · 2025-12-23T22:21:11Z

@jcouv @jjonescz for another pair of eyes.

CyrusNajmabadi · 2025-12-28T23:16:31Z

@jcouv ptal :-)

jcouv · 2025-12-29T23:05:11Z

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

+                        }
+                        else if (seenIn || seenOut || seenRef || seenReadonly)
+                        {
+                            // Matches original parsing logic that disallowed parsing out 'scoped' once in/out/ref/readonly had been seen.


Matches original parsing logic

nit: "original" is relative. "Disallow parsing ..." would be enough.
Same comment applies below (line 791) #Closed

Updated both comments.

jcouv · 2025-12-29T23:07:25Z

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

+                        else if (i < n - 1)
+                        {
+                            // Matches original parsing logic that only allowed 'scoped' to be followed by ref/out/in to
+                            // actually be considered the modifier.


I didn't understand this comment. The logic below merely reports a diagnostic, how does it make it not considered a modifier? #Closed

instead of treating it as a modifier (no error), it errors. meaning we don't accept this as a modifier in this order.

updated comment to now say: Only allow 'scoped' followed by ref/out/in to actually be considered a valid modifier.

into scopedParsing

jcouv · 2025-12-29T23:15:16Z

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationScopeParsingTests.cs

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using System.Linq;


nit: We have another open issue related to scoped ordering: #78556
Could you include a test for scoped in this int and this scoped in int to show if the PR affects those scenarios?

jcouv

LGTM Thanks (commit 30)

* upstream/main: (1373 commits) Add docs Use new test plan for dictionary expressions (dotnet#81861) Update dartlab pipeline setup (dotnet#81807) [main] Update dependencies from dotnet/roslyn (dotnet#81649) Fix LSP, everything but handlers Reapply "Update methods to be `async`." (dotnet#81808) Properly await calls to ensure exception handling works for sync and async scenarios Let Razor fill in project information on diagnostics (dotnet#81822) only analyzers Reapply "Update methods to be `async`." (dotnet#81808) only editorfeatures Reapply "Update methods to be `async`." (dotnet#81808) Enable GetTypeInfo for type of object creation syntax (dotnet#81802) Improve error recovery around 'scoped' modifier parsing (dotnet#81636) Only VisualStudio Only features Only analyzers Only editor features Reapply "Update methods to be `async`." (dotnet#81808) ...

Improve error recovery around 'scoped' modifier parsing

aa4a9a9

CyrusNajmabadi requested a review from a team as a code owner December 10, 2025 13:26

github-actions bot added the Area-Compilers label Dec 10, 2025

Delete

6e7a144

CyrusNajmabadi marked this pull request as draft December 10, 2025 13:27

CyrusNajmabadi added 9 commits December 10, 2025 14:32

Update test

70b8e1a

Preserve modifier/identifier logic from before

3e9d93b

Update tests

0be99eb

Update tsts

26b6685

Update tests

332babb

Update tests

686ad93

Update tests

b3e35fb

delete

179a870

delete

6e0779a

CyrusNajmabadi commented Dec 10, 2025

View reviewed changes

Update tests

742fdb7

CyrusNajmabadi marked this pull request as ready for review December 10, 2025 14:43

CyrusNajmabadi commented Dec 10, 2025

View reviewed changes

Simplify tests

4aa09cf

CyrusNajmabadi commented Dec 10, 2025

View reviewed changes

CyrusNajmabadi added 2 commits December 10, 2025 18:54

Add test

2a7b3a1

better error for 'scoped scoped'

85d0a2d

CyrusNajmabadi added 2 commits December 23, 2025 15:45

Add triple scoped tests

4897d48

lint

78793a0

333fred approved these changes Dec 23, 2025

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaParameterParsingTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi commented Dec 23, 2025

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaParameterParsingTests.cs Outdated Show resolved Hide resolved

Apply suggestion from @CyrusNajmabadi

845fba9

jcouv reviewed Dec 29, 2025

View reviewed changes

CyrusNajmabadi added 3 commits December 29, 2025 18:14

Update comments

5ff993b

Merge branch 'scopedParsing' of https://github.com/CyrusNajmabadi/roslyn

92947dc

into scopedParsing

Tweak comment

05282bf

jcouv reviewed Dec 29, 2025

View reviewed changes

Tweak comment

077cfd2

jcouv approved these changes Dec 29, 2025

View reviewed changes

Ad tests

991bf4e

CyrusNajmabadi enabled auto-merge (squash) December 29, 2025 23:21

CyrusNajmabadi disabled auto-merge December 30, 2025 01:58

CyrusNajmabadi enabled auto-merge (squash) December 30, 2025 01:59

Merge remote-tracking branch 'upstream/main' into scopedParsing

a216706

CyrusNajmabadi merged commit 96c60b5 into dotnet:main Dec 30, 2025
24 of 25 checks passed

CyrusNajmabadi deleted the scopedParsing branch December 30, 2025 17:57

dotnet-policy-service bot added this to the Next milestone Dec 30, 2025

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

[release/10.0.2xx] Source code updates from dotnet/roslyn dotnet/dotnet#4023

Merged

dotnet-bot mentioned this pull request Jan 1, 2026

[Automated] PRs inserted in VS build main-11331.49 #81829

Closed

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

dotnet-maestro bot mentioned this pull request Jan 7, 2026

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

Merged

davidwengier mentioned this pull request Jan 12, 2026

Update Razor and Roslyn dotnet/vscode-csharp#8877

Merged

Comments

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Dec 23, 2025

Uh oh!

CyrusNajmabadi commented Dec 28, 2025

Uh oh!

jcouv Dec 29, 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Dec 10, 2025 •

edited

Loading

jcouv Dec 29, 2025 •

edited

Loading

jcouv Dec 29, 2025 •

edited

Loading