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 expressions vs conditional access expressions #68756

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 113 additions & 19 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10664,30 +10664,98 @@ private ExpressionSyntax ParseExpressionContinued(ExpressionSyntax leftOperand,
// null-coalescing-expression ? expression : expression
//
// Only take the conditional if we're at or below its precedence.
if (CurrentToken.Kind == SyntaxKind.QuestionToken && precedence <= Precedence.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.

i recommend reviewing with whitespae disabled.

if (CurrentToken.Kind != SyntaxKind.QuestionToken || precedence > Precedence.Conditional)
return leftOperand;

// Complex ambiguity with `?` and collection-expressions. Specifically: b?[c]:d
//
// On its own, we want that to be a conditional expression with a collection expression in it. However, for
// back compat, we need to make sure that `a ? b?[c] : d` sees the inner `b?[c]` as a
// conditional-access-expression. So, if after consuming the portion after the initial `?` if we do not
// have the `:` we need, and we can see a `?[` in that portion of the parse, then we retry consuming the
// when-true portion, but this time forcing the prior way of handling `?[`.
var questionToken = this.EatToken();

using var afterQuestionToken = this.GetDisposableResetPoint(resetOnDispose: false);
var whenTrue = this.ParsePossibleRefExpression();

if (this.CurrentToken.Kind != SyntaxKind.ColonToken &&
!this.ForceConditionalAccessExpression &&
containsTernaryCollectionToReinterpret(whenTrue))
{
var questionToken = this.EatToken();
var colonLeft = this.ParsePossibleRefExpression();
if (this.CurrentToken.Kind == SyntaxKind.EndOfFileToken && this.lexer.InterpolationFollowedByColon)
// Keep track of where we are right now in case the new parse doesn't make things better.
using var originalAfterWhenTrue = this.GetDisposableResetPoint(resetOnDispose: false);

// Go back to right after the `?`
afterQuestionToken.Reset();

// try reparsing with `?[` as a conditional access, not a ternary+collection
this.ForceConditionalAccessExpression = true;
var newWhenTrue = this.ParsePossibleRefExpression();
this.ForceConditionalAccessExpression = false;

if (this.CurrentToken.Kind == SyntaxKind.ColonToken)
{
// We have an interpolated string with an interpolation that contains a conditional expression.
// Unfortunately, the precedence demands that the colon is considered to signal the start of the
// format string. Without this code, the compiler would complain about a missing colon, and point
// to the colon that is present, which would be confusing. We aim to give a better error message.
var colon = SyntaxFactory.MissingToken(SyntaxKind.ColonToken);
var colonRight = _syntaxFactory.IdentifierName(SyntaxFactory.MissingToken(SyntaxKind.IdentifierToken));
leftOperand = _syntaxFactory.ConditionalExpression(leftOperand, questionToken, colonLeft, colon, colonRight);
leftOperand = this.AddError(leftOperand, ErrorCode.ERR_ConditionalInInterpolation);
// if we now are at a colon, this was preferred parse.
whenTrue = newWhenTrue;
}
else
{
var colon = this.EatToken(SyntaxKind.ColonToken);
var colonRight = this.ParsePossibleRefExpression();
leftOperand = _syntaxFactory.ConditionalExpression(leftOperand, questionToken, colonLeft, colon, colonRight);
// retrying the parse didn't help. Use the original interpretation.
originalAfterWhenTrue.Reset();
}
}

return leftOperand;
if (this.CurrentToken.Kind == SyntaxKind.EndOfFileToken && this.lexer.InterpolationFollowedByColon)
{
// We have an interpolated string with an interpolation that contains a conditional expression.
// Unfortunately, the precedence demands that the colon is considered to signal the start of the
// format string. Without this code, the compiler would complain about a missing colon, and point
// to the colon that is present, which would be confusing. We aim to give a better error message.
leftOperand = _syntaxFactory.ConditionalExpression(
leftOperand,
questionToken,
whenTrue,
SyntaxFactory.MissingToken(SyntaxKind.ColonToken),
_syntaxFactory.IdentifierName(SyntaxFactory.MissingToken(SyntaxKind.IdentifierToken)));
return this.AddError(leftOperand, ErrorCode.ERR_ConditionalInInterpolation);
}
else
{
return _syntaxFactory.ConditionalExpression(
leftOperand,
questionToken,
whenTrue,
this.EatToken(SyntaxKind.ColonToken),
this.ParsePossibleRefExpression());
}

static bool containsTernaryCollectionToReinterpret(ExpressionSyntax expression)
{
var stack = ArrayBuilder<GreenNode>.GetInstance();
stack.Push(expression);

while (stack.Count > 0)
{
var current = stack.Pop();
if (current is ConditionalExpressionSyntax conditionalExpression &&
conditionalExpression.WhenTrue.GetFirstToken().Kind == SyntaxKind.OpenBracketToken)
{
stack.Free();
return true;
}

// Note: we could consider not recursing into anonymous-methods/lambdas (since we reset the
// ForceConditionalAccessExpression flag when we go into that). However, that adds a bit of
// fragile coupling between these different code blocks that i'd prefer to avoid. In practice
// the extra cost here will almost never occur, so the simplicity is worth it.
foreach (var child in current.ChildNodesAndTokens())
333fred marked this conversation as resolved.
Show resolved Hide resolved
stack.Push(child);
}

stack.Free();
return false;
}
}

private DeclarationExpressionSyntax ParseDeclarationExpression(ParseTypeMode mode, bool isScoped)
Expand Down Expand Up @@ -11057,13 +11125,19 @@ private bool CanStartConsequenceExpression()
if (nextTokenKind == SyntaxKind.OpenBracketToken)
{
// could simply be `x?[0]`, or could be `x ? [0] : [1]`.

// Caller only wants us to parse ?[ how it was originally parsed before collection expressions.
if (this.ForceConditionalAccessExpression)
return true;

using var _ = GetDisposableResetPoint(resetOnDispose: true);

// Move past the '?'. Parse what comes next the same way that conditional expressions are parsed.
this.EatToken();
var collectionExpression = this.ParseCollectionExpression();
this.ParsePossibleRefExpression();

// PROTOTYPE: Def back compat concern here. What if the user has `x ? y?[0] : z` this would be legal,
// but will now change to `y?[0] : z` being a ternary. Have to decide if this is acceptable break.
// If we see a colon, then do not parse this as a conditional-access-expression, pop up to the caller
// and have it reparse this as a conditional-expression instead.
return this.CurrentToken.Kind != SyntaxKind.ColonToken;
}

Expand Down Expand Up @@ -12477,8 +12551,15 @@ private ExpressionSyntax ParseRegularStackAllocExpression()
private AnonymousMethodExpressionSyntax ParseAnonymousMethodExpression()
{
var parentScopeIsInAsync = this.IsInAsync;

var parentScopeForceConditionalAccess = this.ForceConditionalAccessExpression;
this.ForceConditionalAccessExpression = false;

var result = parseAnonymousMethodExpressionWorker();

this.ForceConditionalAccessExpression = parentScopeForceConditionalAccess;
this.IsInAsync = parentScopeIsInAsync;

return result;

AnonymousMethodExpressionSyntax parseAnonymousMethodExpressionWorker()
Expand Down Expand Up @@ -12596,8 +12677,15 @@ private LambdaExpressionSyntax ParseLambdaExpression()
{
var attributes = ParseAttributeDeclarations(inExpressionContext: true);
var parentScopeIsInAsync = this.IsInAsync;

var parentScopeForceConditionalAccess = this.ForceConditionalAccessExpression;
this.ForceConditionalAccessExpression = false;

var result = parseLambdaExpressionWorker();

this.ForceConditionalAccessExpression = parentScopeForceConditionalAccess;
this.IsInAsync = parentScopeIsInAsync;

return result;

LambdaExpressionSyntax parseLambdaExpressionWorker()
Expand Down Expand Up @@ -13169,6 +13257,12 @@ private bool IsInAsync
}
}

private bool ForceConditionalAccessExpression
{
get => _syntaxFactoryContext.ForceConditionalAccessExpression;
set => _syntaxFactoryContext.ForceConditionalAccessExpression = value;
}
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: i went this route (surrounding context) because trying to actually use parameters to pass in this context inwards was extremely complex and viral. Virtually all paths in expression parsing needed to pass this information along. It both clutered things enormously, and made me worried about extra stack frame sizes.

The current approach is nice because it allows each level of ternary parsing to effectively retry the lower level with the old parsing, preferring that if it keeps the original parsing semantics.


private bool IsInQuery
{
get { return _syntaxFactoryContext.IsInQuery; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ internal class SyntaxFactoryContext
/// </summary>
internal bool IsInAsync;

/// <summary>
/// If we are forcing that ?[ is parsed as a conditional-access-expression, and not a conditional-expression
/// with a collection-expression in it.
/// </summary>
internal bool ForceConditionalAccessExpression;
Copy link
Member

Choose a reason for hiding this comment

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

ForceConditionalAccessExpression

Do we need incremental parsing tests, given that we have new context state?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 27, 2023

Choose a reason for hiding this comment

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

I'll try to formulate one. It would be very contrived though. Esp. because we do not reuse expressions or do incremental parsing at the expression level.

That's already because expressions already have a staggering amount of contextual parsing. This is both because of things like speculative lookahead, and because of non-tracked context (like 'current expression precedence'). :-)

I may be able to get something working where a nested local function becomes top level. That should demonstrate things though.


internal int QueryDepth;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,54 @@ void M()
WalkTreeAndVerify(tree.GetCompilationUnitRoot(), fullTree.GetCompilationUnitRoot());
}

[Fact]
public void TestLocalFunctionCollectionVsAccessParsing()
{
var source = """
using System;

class C
{
void M()
{
var v = a ? b?[() =>
{
var v = whatever();
int LocalFunc()
{
var v = a ? [b] : c;
}
var v = whatever();
}] : d;
}
}
""";
var tree = SyntaxFactory.ParseSyntaxTree(source);
Assert.Empty(tree.GetDiagnostics());

var localFunc1 = tree.GetRoot().DescendantNodesAndSelf().Single(n => n is LocalFunctionStatementSyntax);
var innerConditionalExpr1 = localFunc1.DescendantNodesAndSelf().Single(n => n is ConditionalExpressionSyntax);

var text = tree.GetText();

var prefix = "var v = a ? b?[() =>";
var suffix = "] : d;";

var prefixSpan = new TextSpan(source.IndexOf(prefix), length: prefix.Length);
var suffixSpan = new TextSpan(source.IndexOf(suffix), length: suffix.Length);
text = text.WithChanges(new TextChange(prefixSpan, ""), new TextChange(suffixSpan, ""));
tree = tree.WithChangedText(text);
Assert.Empty(tree.GetDiagnostics());

var fullTree = SyntaxFactory.ParseSyntaxTree(text.ToString());
Assert.Empty(fullTree.GetDiagnostics());

var localFunc2 = tree.GetRoot().DescendantNodesAndSelf().Single(n => n is LocalFunctionStatementSyntax);
var innerConditionalExpr2 = localFunc2.DescendantNodesAndSelf().Single(n => n is ConditionalExpressionSyntax);

WalkTreeAndVerify(tree.GetCompilationUnitRoot(), fullTree.GetCompilationUnitRoot());
}

#region "Regression"

#if false
Expand Down
Loading