Fix ambiguity parsing collection expressions vs conditional access expressions #68756
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -10664,30 +10665,92 @@ 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) | |||
There was a problem hiding this comment.
i recommend reviewing with whitespae disabled.
| UsingExpression("x ? [A] () => { } : z", TestOptions.RegularPreview, | ||
| // (1,1): error CS1073: Unexpected token '=>' | ||
| // x ? [A] () => { } : z | ||
| Diagnostic(ErrorCode.ERR_UnexpectedToken, "x ? [A] ()").WithArguments("=>").WithLocation(1, 1)); |
There was a problem hiding this comment.
note: this error appears to be incorrect (and the new test ConditionalExpression_01_A shows why that is). It looks like we missed this when we added attribute-lambdas (or just accepted that it didn't work). With the updated logic, this now properly parses.
| [Fact] | ||
| public void ConditionalExpression_01_A() | ||
| { | ||
| UsingExpression("x ? () => { } : z", TestOptions.RegularPreview); |
There was a problem hiding this comment.
this shows that we should be parsing this properly. And the addition of attributes to the lambda shouldn't change anything.
| { | ||
| get => _syntaxFactoryContext.ForceConditionalAccessExpression; | ||
| set => _syntaxFactoryContext.ForceConditionalAccessExpression = value; | ||
| } |
There was a problem hiding this comment.
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.
| /// 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; |
There was a problem hiding this comment.
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.
|
@RikkiGibson ptal. Will also be adding tests as I work more in this space. |
| [Fact] | ||
| public void ConditionalAmbiguity11() | ||
| { | ||
| UsingExpression("a ? b?[c] : d ? e?[f] : g"); |
There was a problem hiding this comment.
Woe be upon any who would write this code.
Would def appreciate help with getting lots of test cases to throw at the situation here :)