Skip to content

Commit

Permalink
Improve parser recovery around nullable types in patterns
Browse files Browse the repository at this point in the history
  • Loading branch information
@DoctorKrolic
DoctorKrolic committed Mar 29, 2024
1 parent 6f7f627 commit 41d70bf
Show file tree
Hide file tree
Showing 9 changed files with 1,975 additions and 147 deletions.
60 changes: 48 additions & 12 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6916,20 +6916,20 @@ private enum ParseTypeMode
FirstElementOfPossibleTupleLiteral,
}

private TypeSyntax ParseType(ParseTypeMode mode = ParseTypeMode.Normal)
private TypeSyntax ParseType(ParseTypeMode mode = ParseTypeMode.Normal, bool whenIsKeyword = false)
{
if (this.CurrentToken.Kind == SyntaxKind.RefKeyword)
{
return _syntaxFactory.RefType(
this.EatToken(),
this.CurrentToken.Kind == SyntaxKind.ReadOnlyKeyword ? this.EatToken() : null,
ParseTypeCore(ParseTypeMode.AfterRef));
ParseTypeCore(ParseTypeMode.AfterRef, whenIsKeyword));
}

return ParseTypeCore(mode);
return ParseTypeCore(mode, whenIsKeyword);
}

private TypeSyntax ParseTypeCore(ParseTypeMode mode)
private TypeSyntax ParseTypeCore(ParseTypeMode mode, bool whenIsKeyword)
{
NameOptions nameOptions;
switch (mode)
Expand Down Expand Up @@ -6970,7 +6970,7 @@ private TypeSyntax ParseTypeCore(ParseTypeMode mode)
{
case SyntaxKind.QuestionToken when canBeNullableType():
{
var question = EatNullableQualifierIfApplicable(mode);
var question = EatNullableQualifierIfApplicable(mode, whenIsKeyword);
if (question != null)
{
type = _syntaxFactory.NullableType(type, question);
Expand Down Expand Up @@ -7043,7 +7043,7 @@ bool canBeNullableType()
return type;
}

private SyntaxToken EatNullableQualifierIfApplicable(ParseTypeMode mode)
private SyntaxToken EatNullableQualifierIfApplicable(ParseTypeMode mode, bool whenIsKeyword)
{
Debug.Assert(this.CurrentToken.Kind == SyntaxKind.QuestionToken);
using var resetPoint = this.GetDisposableResetPoint(resetOnDispose: false);
Expand All @@ -7065,12 +7065,48 @@ bool canFollowNullableType()
case ParseTypeMode.AfterIs:
case ParseTypeMode.DefinitePattern:
case ParseTypeMode.AsExpression:
// These contexts might be a type that is at the end of an expression. In these contexts we only
// permit the nullable qualifier if it is followed by a token that could not start an
// expression, because for backward compatibility we want to consider a `?` token as part of the
// `?:` operator if possible.
//
// Similarly, if we have `T?[]` we do want to treat that as an array of nullables (following
// These contexts might be a type that is at the end of an expression.
// For backward compatibility we want to consider a `?` token as part of the `?:` operator if possible.
// However, if current token is an identifier, it might be beneficial to allow `?`
// and treat this identifier as a designation. Nullable types in patterns are semantically invalid,
// so we will get a nice error about that during binding.
if (this.CurrentToken.Kind == SyntaxKind.IdentifierToken)
{
// Given that we are deciding whether we take `?:` operator or not while looking at state after `?`,
// our focus is on `:` token as well. But `:` have special meaning in context of switch statements
// and expressions (since `:` in switch expressions is recovered to `=>`). So consider the following case:
// `case T? t:`. On its own `T ? t :` can be treated as a conditional expression with condition `T`,
// 'whenTrue' `t` and 'whenFalse' not yet parsed. But it is way better to take `:` as case label end and
// thus parse label condition as a declaration pattern `T? t`. `whenIsKeyword` flag can conveniently tell
// whether we are in a 'top-level' state of a label, so if condition get parenthesized like `case (a ? b : c):`
// we don't take the first `:` as label end and parse label condition as a parenthesized conditional expression
if ((_termState.HasFlag(TerminatorState.IsExpressionOrPatternInCaseLabelOfSwitchStatement) ||
_termState.HasFlag(TerminatorState.IsPatternInSwitchExpressionArm)) && whenIsKeyword)
{
return true;
}

using var _ = GetDisposableResetPoint(resetOnDispose: true);
this.EatToken();

// If token after identifier starts an expression it is probably an error case, e.g. missing a `,` in a pattern
if (CanStartExpression())
{
return true;
}

// These token either 100% end a pattern or start a new one (again, in cases with missing `,` and so on).
// Note, that some cases of 'token starts a new pattern' are handled by condition above, e.g. starting of type patterns
return this.CurrentToken.Kind is SyntaxKind.OpenParenToken or SyntaxKind.CloseParenToken
or SyntaxKind.OpenBraceToken or SyntaxKind.CloseBraceToken
or SyntaxKind.OpenBracketToken or SyntaxKind.CloseBracketToken
or SyntaxKind.CommaToken
or SyntaxKind.EndOfFileToken;
}

// 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:
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private PatternSyntax ParsePrimaryPattern(Precedence precedence, bool afterIs, b
TypeSyntax? type = null;
if (LooksLikeTypeOfPattern())
{
type = this.ParseType(afterIs ? ParseTypeMode.AfterIs : ParseTypeMode.DefinitePattern);
type = this.ParseType(afterIs ? ParseTypeMode.AfterIs : ParseTypeMode.DefinitePattern, whenIsKeyword);
if (type.IsMissing || !CanTokenFollowTypeInPattern(precedence))
{
// either it is not shaped like a type, or it is a constant expression.
Expand Down
45 changes: 19 additions & 26 deletions src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationExpressionTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#nullable disable

using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -134,45 +135,37 @@ public void NullableTypeTest_02()
EOF();
}

[Fact]
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72720")]
public void NullableTypeTest_03()
{
UsingStatement("if (e is int? x) {}",
// (1,16): error CS1003: Syntax error, ':' expected
// if (e is int? x) {}
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(":").WithLocation(1, 16),
// (1,16): error CS1525: Invalid expression term ')'
// if (e is int? x) {}
Diagnostic(ErrorCode.ERR_InvalidExprTerm, ")").WithArguments(")").WithLocation(1, 16)
);
UsingStatement("if (e is int? x) {}");

N(SyntaxKind.IfStatement);
{
N(SyntaxKind.IfKeyword);
N(SyntaxKind.OpenParenToken);
N(SyntaxKind.ConditionalExpression);
N(SyntaxKind.IsPatternExpression);
{
N(SyntaxKind.IsExpression);
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierName);
N(SyntaxKind.IdentifierToken, "e");
}
N(SyntaxKind.IsKeyword);
N(SyntaxKind.DeclarationPattern);
{
N(SyntaxKind.NullableType);
{
N(SyntaxKind.IdentifierToken, "e");
N(SyntaxKind.PredefinedType);
{
N(SyntaxKind.IntKeyword);
}
N(SyntaxKind.QuestionToken);
}
N(SyntaxKind.IsKeyword);
N(SyntaxKind.PredefinedType);
N(SyntaxKind.SingleVariableDesignation);
{
N(SyntaxKind.IntKeyword);
N(SyntaxKind.IdentifierToken, "x");
}
}
N(SyntaxKind.QuestionToken);
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, "x");
}
M(SyntaxKind.ColonToken);
M(SyntaxKind.IdentifierName);
{
M(SyntaxKind.IdentifierToken);
}
}
N(SyntaxKind.CloseParenToken);
N(SyntaxKind.Block);
Expand Down
Loading

0 comments on commit 41d70bf

Please sign in to comment.