Improve parser recovery around nullable types in patterns #72805
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7015,20 +7015,20 @@ private enum ParseTypeMode | |
| FirstElementOfPossibleTupleLiteral, | ||
| } | ||
|
|
||
| private TypeSyntax ParseType(ParseTypeMode mode = ParseTypeMode.Normal, bool inSwitchArmPattern = false) | ||
| { | ||
| if (this.CurrentToken.Kind == SyntaxKind.RefKeyword) | ||
| { | ||
| return _syntaxFactory.RefType( | ||
| this.EatToken(), | ||
| this.CurrentToken.Kind == SyntaxKind.ReadOnlyKeyword ? this.EatToken() : null, | ||
| ParseTypeCore(ParseTypeMode.AfterRef, inSwitchArmPattern)); | ||
| } | ||
|
|
||
| return ParseTypeCore(mode, inSwitchArmPattern); | ||
| } | ||
|
|
||
| private TypeSyntax ParseTypeCore(ParseTypeMode mode, bool inSwitchArmPattern) | ||
| { | ||
| NameOptions nameOptions; | ||
| switch (mode) | ||
|
|
@@ -7069,7 +7069,7 @@ private TypeSyntax ParseTypeCore(ParseTypeMode mode) | |
| { | ||
| case SyntaxKind.QuestionToken when canBeNullableType(): | ||
| { | ||
| var question = EatNullableQualifierIfApplicable(mode, inSwitchArmPattern); | ||
| if (question != null) | ||
| { | ||
| type = _syntaxFactory.NullableType(type, question); | ||
|
|
@@ -7142,7 +7142,9 @@ bool canBeNullableType() | |
| return type; | ||
| } | ||
|
|
||
| /// <param name="inSwitchArmPattern">If this is in the patternsection of a switch-expression arm or a | ||
| /// switch-statement case-clause.</param> | ||
| private SyntaxToken EatNullableQualifierIfApplicable(ParseTypeMode mode, bool inSwitchArmPattern) | ||
| { | ||
| Debug.Assert(this.CurrentToken.Kind == SyntaxKind.QuestionToken); | ||
| using var resetPoint = this.GetDisposableResetPoint(resetOnDispose: false); | ||
|
|
@@ -7164,18 +7166,88 @@ bool canFollowNullableType() | |
| case ParseTypeMode.AfterIs: | ||
| case ParseTypeMode.DefinitePattern: | ||
| case ParseTypeMode.AsExpression: | ||
| // We are currently after `?` token after a nullable type pattern and need to decide how to | ||
| // parse what we see next. In the case of an identifier (e.g. `x ? a` there are two ways we can | ||
| // see things | ||
| // | ||
| // 1. As a start of conditional expression, e.g. `var a = obj is string ? a : b` | ||
| // 2. As a designation of a nullable-typed pattern, e.g. `if (obj is string? str)` | ||
| // | ||
| // Since nullable types (no matter reference of value types) are not valid in patterns by | ||
DoctorKrolic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // default we are biased towards the first option and consider case 2 only for error recovery | ||
| // purposes (if we parse here as nullable type pattern an error will be reported during | ||
| // binding). This condition checks for simple cases, where we better use option 2 and parse a | ||
| // nullable-typed pattern | ||
| if (IsTrueIdentifier(this.CurrentToken)) | ||
| { | ||
| // If we're in a cast statement/expression arm, then we have: | ||
| // | ||
| // `switch { case X ? y` or | ||
|
There was a problem hiding this comment. This assumption is not correct. The following is valid code: switch(i3) {
case b ? i1 : i2: break;
}This comment needs to be corrected, and we should re-examine this approach to ensure that we're not breaking anything. There was a problem hiding this comment. The "why on Earth this is legal" question should have probably been asked N years ago when switch statements were designed) I was sure that conditional expressions in case labels aren't semantically legal and thus there is no difference whether this is parsed as a conditional or a nullable pattern + label statement. There is already a test in the codebase for that case and I initially wrote a comment explaining how these 2 parse strategies both lead to a compiler error thus are identical for our purposes, had to revert that change. And I'm somewhat surprised that only a very few new test cases now are back to bad state after removing this assumption, so I don't think we should do additional work to restore them There was a problem hiding this comment.
So, this is legal because cases allow for patterns. And one sort of pattern is the 'constant pattern' (that's how |
||
| // | ||
| // e switch { X ? y ` | ||
|
There was a problem hiding this comment. Similarly, this assumption is closer to correct, but isn't always: _ = i3 switch {
(b ? i1 : i2) => 3
};You do need the parentheses to force parse in this manner, but I'm concerned that we'll still end up with bad behavior here. There was a problem hiding this comment. Given that this requires parentheses, it wasn't affected even with the wrong assumption. Added a test for this case |
||
| // | ||
| // In both these cases, this isn't a conditional expression and should be treated as a | ||
| // nullable type pattern (with a a good binding error to be reported later). | ||
| if (inSwitchArmPattern) | ||
| return true; | ||
|
|
||
| // In a non-async method, `await` is a simple identifier. However, if we see `x ? await` | ||
| // it's almost certainly the start of an `await expression` in a conditional expression | ||
| // (e.g. `x is Y ? await ...`), not a nullable type pattern (since users would not use | ||
| // 'await' as the name of a variable). So just treat this as a conditional expression. | ||
| if (this.CurrentToken.ContextualKind == SyntaxKind.AwaitKeyword) | ||
DoctorKrolic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return false; | ||
|
|
||
| var nextTokenKind = PeekToken(1).Kind; | ||
|
|
||
| // These token either 100% end a pattern or start a new one: | ||
|
|
||
| // A literal token starts a new pattern. Can occur in list pattern with missing separation | ||
| // `,`. For example, in `x is [int[]? arr 5]` we'd prefer to parse this as a missing `,` | ||
| // after `arr` | ||
| if (SyntaxFacts.IsLiteral(nextTokenKind)) | ||
| return true; | ||
|
|
||
| // A predefined type is basically the same case: `x is [string[]? slice char ch]`. We'd | ||
| // prefer to parse this as a missing `,` after `slice`. | ||
| if (SyntaxFacts.IsPredefinedType(nextTokenKind)) | ||
| return true; | ||
|
|
||
| // `)`, `]` and `}` obviously end a pattern. For example: | ||
| // `if (x is int? i)`, `indexable[x is string? s]`, `x is { Prop: Type? typeVar }` | ||
| if (nextTokenKind is SyntaxKind.CloseParenToken or SyntaxKind.CloseBracketToken or SyntaxKind.CloseBraceToken) | ||
| return true; | ||
|
|
||
| // `{` starts a new pattern. For example: `x is A? { ...`. Note, that `[` and `(` are not | ||
| // in the list because they can start an invocation/indexer | ||
| if (nextTokenKind == SyntaxKind.OpenBraceToken) | ||
| return true; | ||
|
|
||
| // `,` ends a pattern in list/property pattern. For example `x is { Prop1: Type1? type, Prop2: Type2 }` or | ||
| // `x is [Type1? type, ...]` | ||
| if (nextTokenKind == SyntaxKind.CommaToken) | ||
| return true; | ||
|
|
||
| // `;` end a pattern if it finishes an expression statement: var y = x is bool? b; | ||
| if (nextTokenKind == SyntaxKind.SemicolonToken) | ||
| return true; | ||
|
|
||
| // EndOfFileToken is obviously the end of parsing. We are better parsing a pattern rather | ||
| // than an unfinished conditional expression | ||
| if (nextTokenKind == SyntaxKind.EndOfFileToken) | ||
| return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // If nothing from above worked permit the nullable qualifier if it is followed by a token that | ||
| // could not start an expression If we have `T?[]` we do want to treat that as an array of | ||
| // nullables (following existing parsing), not a conditional that returns a list. | ||
| return !CanStartExpression() || this.CurrentToken.Kind is SyntaxKind.OpenBracketToken; | ||
| case ParseTypeMode.NewExpression: | ||
| // A nullable qualifier is permitted as part of the type in a `new` expression. e.g. `new | ||
| // int?()` is allowed. It creates a null value of type `Nullable<int>`. Similarly `new int? {}` | ||
| // is allowed. | ||
| return | ||
| this.CurrentToken.Kind is SyntaxKind.OpenParenToken or // ctor parameters | ||
| SyntaxKind.OpenBracketToken or // array type | ||
|
|
||
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.
whenIsKeywordgot renamed toinSwitchArmPatternto try to be clearer. We pass 'true' in here when parsign the pattern for a switch-statement or switch-expression pattern. And it controls how we view 'where' and a few other error recovery rules (including new ones in this pr).