Skip to content
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

Fix ambiguity parsing collection in conditional expressions #75328

Merged
merged 12 commits into from
Oct 2, 2024

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #75318

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 1, 2024 19:58
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 1, 2024
@CyrusNajmabadi
Copy link
Member Author

@cston @RikkiGibson ptal :)

UsingExpression(source,
// (1,19): error CS1003: Syntax error, ':' expected
// x is string ? [A] y
Diagnostic(ErrorCode.ERR_SyntaxError, "y").WithArguments(":").WithLocation(1, 19));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this change is appropriate. it's not legal to say x is string?[A]. string?[A] is not a type. While you can instantiate a new string?[A], it's type is now string?[]. So this now properly becomes a case where this is a conditional that creates a collection, but is missing a colon.

// (1,1): error CS1073: Unexpected token '=>'
// _ = x is string ? [A] y => y : z
Diagnostic(ErrorCode.ERR_UnexpectedToken, "_ = x is string ? [A] y").WithArguments("=>").WithLocation(1, 1));
UsingExpression(source);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old parsing was just wrong. This should have parsed according to the rules of the language. The precedence rules say: https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#1115-conditional-operator

conditional_expression
    : null_coalescing_expression
    | null_coalescing_expression '?' expression ':' expression
    ;

So the true/false parts of a conditional can be full expressions (which includes lambdas). So this should have correctly parsed before. This is a fix.

@@ -2360,13 +2368,7 @@ public void NullableType_Is_04()
public void NullableType_Is_05()
{
string source = "_ = x is string ? [return: A] y => y : z";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

@RikkiGibson RikkiGibson self-assigned this Oct 2, 2024
// see if it is followed by `:` to find out. If there is a colon, it's a
// conditional.
case SyntaxKind.CloseBracketToken:
return this.PeekToken(3).Kind != SyntaxKind.ColonToken;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If collection-exprs become usable as a primary-expression, we'll have to revisit this heuristic, right?
e.g. for x is T ? [].Method() : ...

Also, are there any potentially valid expression forms today, which don't have the collection immediately followed by :, like x is T ? [] == GetCollection() : ...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If collection-exprs become usable as a primary-expression, we'll have to revisit this heuristic, right?

Yes. We'd need to revisit that based on what can legally follow. Fun times.

Also, are there any potentially valid expression forms today, which don't have the collection immediately followed by :, like x is T ? [] == GetCollection() : ...?

THis is definitely an interesting case. I'm thinking we may actually have to speculatively parse here to see what is happening.

@@ -11254,13 +11287,16 @@ private ExpressionSyntax ParseExpressionContinued(ExpressionSyntax leftOperand,
}
}

// From the language spec:
// https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md?rgh-link-date=2024-10-02T19%3A11%3A19Z#1115-conditional-operator:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the rgh-link-date=... in this url do something useful? If not, consider removing it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ick. will do.

{
var question = EatNullableQualifierIfApplicable(mode);
var question = TryEatNullableQualifierIfApplicable(type, mode);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. the prior logic was very confusing. We had 'canBeNullable' here, and then EatNullableQualifierIfApplicable (which also did a bunch more canBeNullable checks). I've merged these all into the renamed TryEatNullableQualifierIfApplicable method now.

// These are the fast tests for (in)applicability.
// More expensive tests are in `EatNullableQualifierIfApplicable`
if (type.Kind == SyntaxKind.NullableType || type.Kind == SyntaxKind.PointerType)
return false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic moved into the new method.

if (type.Kind == SyntaxKind.NullableType || type.Kind == SyntaxKind.PointerType)
return false;
if (this.PeekToken(1).Kind == SyntaxKind.OpenBracketToken)
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the logic that had to be expanded on majorly to handle the new parsing.

if (this.PeekToken(1).Kind == SyntaxKind.OpenBracketToken)
return true;
if (mode == ParseTypeMode.DefinitePattern)
return true; // Permit nullable type parsing and report while binding for a better error message
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need this check anymore, as this was jsut the bool that determined if we moved onto TryEatNullableQualifierIfApplicable

this.PeekToken(1).Kind is not SyntaxKind.OpenParenToken and not SyntaxKind.OpenBraceToken)
{
return false; // Permit `new (int, int)?(t)` (creation) and `new (int, int) ? x : y` (conditional)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this falls out from existing code in the existing method.

// If we already have `x?` or `x*` then do not parse out a nullable type if we see `x??` or `x*?`. These
// are never legal as types in the language, so we can fast bail out.
if (typeParsedSoFar.Kind is SyntaxKind.NullableType or SyntaxKind.PointerType)
return null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a move, and i added comments to explain it.

return null;
}

return questionToken;

bool canFollowNullableType()
{
if (mode == ParseTypeMode.AfterIs && this.CurrentToken.Kind is SyntaxKind.OpenBracketToken)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the new logic for figuring out what to do with x?[


// Now see if we have a ':' following. If so, this is a conditional. If not, it's a nullable type.
return this.CurrentToken.Kind != SyntaxKind.ColonToken;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added based on what Rikki noticed.

@@ -7523,7 +7558,10 @@ bool canFollowNullableType()
// 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;
if (this.CurrentToken.Kind is SyntaxKind.OpenBracketToken)
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulled simple check up.

// _ = x is string ? [] y => y : z
Diagnostic(ErrorCode.ERR_UnexpectedToken, "_ = x is string ? [] y").WithArguments("=>").WithLocation(1, 1));
Diagnostic(ErrorCode.ERR_IdentifierExpected, "]").WithLocation(1, 20));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this parsing matches teh parsing of hte test below this one ("_ = x is string ? [A] y => y : z"). Specifically, we now see this appropriately as a conditional that returns a lambda. But we report an issue on the lambda that it's attribute-list is missing attributes. This is better parse than before, more in line with the parse once you add the attribute.

@CyrusNajmabadi CyrusNajmabadi merged commit 948ccac into dotnet:main Oct 2, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Oct 2, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the collectionParsingAmbiguity branch October 3, 2024 17:56
@akhera99 akhera99 modified the milestones: Next, 17.13 P1 Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
4 participants