-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve error recovery around 'scoped' modifier parsing #81636
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
Changes from all commits
aa4a9a9
6e7a144
70b8e1a
3e9d93b
0be99eb
26b6685
332babb
686ad93
b3e35fb
179a870
6e0779a
742fdb7
4aa09cf
2a7b3a1
85d0a2d
3d8d9fc
14db92b
29b92fb
fb0a724
e00ae77
c2ad0e9
d80f672
b87389e
4897d48
78793a0
845fba9
5ff993b
92947dc
05282bf
077cfd2
991bf4e
a216706
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4834,7 +4834,9 @@ private bool IsPossibleParameter() | |
| return this.IsTrueIdentifier(); | ||
|
|
||
| default: | ||
| return IsParameterModifierExcludingScoped(this.CurrentToken) || IsPossibleScopedKeyword(isFunctionPointerParameter: false) || IsPredefinedType(this.CurrentToken.Kind); | ||
| return IsParameterModifierExcludingScoped(this.CurrentToken) || | ||
| IsDefiniteScopedModifier(isFunctionPointerParameter: false, isLambdaParameter: false) || | ||
| IsPredefinedType(this.CurrentToken.Kind); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -4961,32 +4963,50 @@ private static bool IsParameterModifierExcludingScoped(SyntaxToken token) | |
|
|
||
| private void ParseParameterModifiers(SyntaxListBuilder modifiers, bool isFunctionPointerParameter, bool isLambdaParameter) | ||
| { | ||
| bool tryScoped = true; | ||
| Debug.Assert(!(isFunctionPointerParameter && isLambdaParameter), "Can't be parsing parameters for both a function pointer and a lambda at the same time"); | ||
|
|
||
| while (IsParameterModifierExcludingScoped(this.CurrentToken)) | ||
| var seenScoped = false; | ||
| while (true) | ||
| { | ||
| if (this.CurrentToken.Kind is SyntaxKind.RefKeyword or SyntaxKind.OutKeyword or SyntaxKind.InKeyword or SyntaxKind.ReadOnlyKeyword) | ||
| // Normal keyword-modifier (in/out/ref/readonly/params/this). Always safe to consume. | ||
| if (IsParameterModifierExcludingScoped(this.CurrentToken)) | ||
| { | ||
| tryScoped = false; | ||
| modifiers.Add(this.EatToken()); | ||
| continue; | ||
| } | ||
|
|
||
| modifiers.Add(this.EatToken()); | ||
| } | ||
|
|
||
| if (tryScoped) | ||
| { | ||
| SyntaxToken scopedKeyword = ParsePossibleScopedKeyword(isFunctionPointerParameter, isLambdaParameter); | ||
|
|
||
| if (scopedKeyword != null) | ||
| // 'scoped' modifier. May be ambiguous with a type/identifier. And has changed parsing rules between | ||
| // C#13/14 inside a lambda parameter list. | ||
| if (this.IsDefiniteScopedModifier(isFunctionPointerParameter, isLambdaParameter)) | ||
| { | ||
| modifiers.Add(scopedKeyword); | ||
|
|
||
| // Look if ref/out/in/readonly are next | ||
| while (this.CurrentToken.Kind is SyntaxKind.RefKeyword or SyntaxKind.OutKeyword or SyntaxKind.InKeyword or SyntaxKind.ReadOnlyKeyword) | ||
| // First scoped-modifier is always considered the modifier. | ||
| if (!seenScoped) | ||
| { | ||
| modifiers.Add(this.EatToken()); | ||
| seenScoped = true; | ||
| modifiers.Add(this.EatContextualToken(SyntaxKind.ScopedKeyword)); | ||
| continue; | ||
| } | ||
| else | ||
| { | ||
| // If we've already seen `scoped` then we may have a situation like `scoped scoped`. This could | ||
| // be duplicated modifier, or it could be that the second `scoped` is actually the identifier of | ||
| // a parameter. | ||
| // | ||
| // Places where it is an identifier are: | ||
| // | ||
| // `(scoped scoped) =>` | ||
| // `(scoped scoped, ...) =>` | ||
| // `(scoped scoped = ...) =>` | ||
| if (this.PeekToken(1).Kind is not (SyntaxKind.CloseParenToken or SyntaxKind.CommaToken or SyntaxKind.EqualsToken)) | ||
| { | ||
| modifiers.Add(this.EatContextualToken(SyntaxKind.ScopedKeyword)); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Not a modifier. We're done. | ||
| return; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -8464,7 +8484,7 @@ private bool IsPossibleLocalDeclarationStatement(bool isGlobalScriptLevel) | |
| return true; | ||
| } | ||
|
|
||
| if (IsPossibleScopedKeyword(isFunctionPointerParameter: false)) | ||
| if (IsDefiniteScopedModifier(isFunctionPointerParameter: false, isLambdaParameter: false)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
@@ -8482,12 +8502,6 @@ private bool IsPossibleLocalDeclarationStatement(bool isGlobalScriptLevel) | |
| return IsPossibleFirstTypedIdentifierInLocalDeclarationStatement(isGlobalScriptLevel); | ||
| } | ||
|
|
||
| private bool IsPossibleScopedKeyword(bool isFunctionPointerParameter) | ||
| { | ||
| using var _ = this.GetDisposableResetPoint(resetOnDispose: true); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for those keeping count, this made for a triple nested reset point. We now just have one reset point. |
||
| return ParsePossibleScopedKeyword(isFunctionPointerParameter, isLambdaParameter: false) != null; | ||
| } | ||
|
|
||
| private bool IsPossibleFirstTypedIdentifierInLocalDeclarationStatement(bool isGlobalScriptLevel) | ||
| { | ||
| bool? typedIdentifier = IsPossibleTypedIdentifierStart(this.CurrentToken, this.PeekToken(1), allowThisKeyword: false); | ||
|
|
@@ -8623,7 +8637,7 @@ private bool IsPossibleTopLevelUsingLocalDeclarationStatement() | |
| // Skip 'using' keyword | ||
| EatToken(); | ||
|
|
||
| if (IsPossibleScopedKeyword(isFunctionPointerParameter: false)) | ||
| if (IsDefiniteScopedModifier(isFunctionPointerParameter: false, isLambdaParameter: false)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
@@ -10518,41 +10532,34 @@ private StatementSyntax ParseLocalDeclarationStatement(SyntaxList<AttributeListS | |
| } | ||
| } | ||
|
|
||
| private SyntaxToken ParsePossibleScopedKeyword( | ||
| private bool IsDefiniteScopedModifier( | ||
| bool isFunctionPointerParameter, | ||
| bool isLambdaParameter) | ||
| { | ||
| if (this.CurrentToken.ContextualKind != SyntaxKind.ScopedKeyword) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this got much easier as we just need to return a bool saying if this should be considered a modifier. We don't actually have to do all the setting-up/rewinding if we change our minds. This means we just need a single reset point in here, and the caller can just eat depending on what we return. |
||
| return null; | ||
| 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. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // so `scoped scoped` is `modifier-scoped identifier-scoped` not `type-scoped identifier-scoped`. | ||
| // Note: this only applies the modifier/type portion. We still allow the identifier of a lambda | ||
| // to be named 'scoped'. | ||
| if (isLambdaParameter && IsFeatureEnabled(MessageID.IDS_FeatureSimpleLambdaParameterModifiers)) | ||
| return this.EatContextualToken(SyntaxKind.ScopedKeyword); | ||
| return true; | ||
|
|
||
| using var beforeScopedResetPoint = this.GetDisposableResetPoint(resetOnDispose: false); | ||
| using var beforeScopedResetPoint = this.GetDisposableResetPoint(resetOnDispose: true); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| var scopedKeyword = this.EatContextualToken(SyntaxKind.ScopedKeyword); | ||
|
|
||
| // trivial case. scoped ref/out/in is definitely the scoped keyword. | ||
| if (this.CurrentToken.Kind is (SyntaxKind.RefKeyword or SyntaxKind.OutKeyword or SyntaxKind.InKeyword)) | ||
| return scopedKeyword; | ||
| // 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)) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we intentionally allow a broader set than before. so you can say things like |
||
| return true; | ||
|
|
||
| // More complex cases. We have to check for `scoped Type ...` now. | ||
| using var afterScopedResetPoint = this.GetDisposableResetPoint(resetOnDispose: false); | ||
|
|
||
| if (ScanType() is ScanTypeFlags.NotType || | ||
| !isValidScopedTypeCase()) | ||
| { | ||
| // We didn't see a type, or it wasn't a legal usage of a type. This is not a scoped-keyword. Rollback to | ||
| // before the keyword so the caller has to handle it. | ||
| beforeScopedResetPoint.Reset(); | ||
| return null; | ||
| } | ||
|
|
||
| // We had a Type syntax in a supported production. Roll back to just after the scoped-keyword and | ||
| // return it successfully. | ||
| afterScopedResetPoint.Reset(); | ||
| return scopedKeyword; | ||
| // | ||
| // Note that `scoped scoped` can be valid here as a type called scoped and a variable called scoped. | ||
| return ScanType() is not ScanTypeFlags.NotType && isValidScopedTypeCase(); | ||
|
|
||
| bool isValidScopedTypeCase() | ||
| { | ||
|
|
@@ -10574,6 +10581,15 @@ bool isValidScopedTypeCase() | |
| } | ||
| } | ||
|
|
||
| private SyntaxToken ParsePossibleScopedKeyword( | ||
| bool isFunctionPointerParameter, | ||
| bool isLambdaParameter) | ||
| { | ||
| return IsDefiniteScopedModifier(isFunctionPointerParameter, isLambdaParameter) | ||
| ? this.EatContextualToken(SyntaxKind.ScopedKeyword) | ||
| : null; | ||
| } | ||
|
|
||
| private VariableDesignationSyntax ParseDesignation(bool forPattern) | ||
| { | ||
| // the two forms of designation are | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this only happen when
isLambdaParameteris true?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We also hit this in cases like
ref scoped scoped X xin normal method parameter cases. In this case, the firstscopedis considered a modifier (sincescoped Xlooks like atype id). Then the second 'scoped' is also a modifier because it's also ontype identifier.So, we treat the first as a scoped modifier. Then, when we see the second, since we prove it's not an param-identifier, we make it a modifier as well.