Don't treat a colon in a conditional expression branch as part of an arrow function #47550
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
src/compiler/parser.ts
Outdated
| @@ -4147,7 +4147,7 @@ namespace ts { | |||
| return parseOptional(SyntaxKind.EqualsToken) ? parseAssignmentExpressionOrHigher() : undefined; | |||
| } | |||
|
|
|||
| function parseAssignmentExpressionOrHigher(): Expression { | |||
| function parseAssignmentExpressionOrHigher(parsingConditionalTrueSide = false): Expression { | |||
There was a problem hiding this comment.
I am not attached to this parameter name. Please suggest something better if you think of one!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I didn't create a context flag for this as I wasn't sure that it would be clearer to go and figure out all of the places where I want to exit the context. The parameter naturally unsets itself when it's no longer needed, at the cost of some plumbing and maybe the potential to forget to pass it down if the code changes. I can try and switch it to a context flag if that's more desirable, but I'm not entirely convinced it's the better of the two options. |
src/compiler/parser.ts
Outdated
| colonToken = parseExpectedToken(SyntaxKind.ColonToken), | ||
| nodeIsPresent(colonToken) | ||
| ? parseAssignmentExpressionOrHigher() | ||
| ? parseAssignmentExpressionOrHigher(/*parsingConditionalTrueSide*/ false) |
There was a problem hiding this comment.
This is false because I believe this to be unambiguous after the first colon, and one of the original test cases exhibits this.
(I could have omitted the parameter, but I'm being explicit in this one place for clarity.)
There was a problem hiding this comment.
This might be problematic in a ? b ? c : (d) : e => f
There was a problem hiding this comment.
Thanks for the test case. This should be parsed the same as:
a ? (b ? (c) : (d)) : (e => f)I guess it's still ambiguous in the right side.
jakebailey
changed the title
|
An alternative approach - one idea @ahejlsberg had was to disallow return-annotated function expressions in conditionals today. In that case, we would continue parsing it as we do today, but issue an error that the arrow function has to be parenthesized. |
|
Unless I'm misunderstanding, that's exactly what my PR does; you can't stick a return type in an unparenthesized branch of a conditional expression, as it'll always assume it's terminating. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/compiler/parser.ts
Outdated
| // false side, we may still be within the true side of a parent conditional | ||
| // expression, so don't allow it to be a return type either. | ||
| if (parsingConditionalExpression && token() === SyntaxKind.ColonToken) { | ||
| Debug.assert(!allowAmbiguity); |
There was a problem hiding this comment.
can you issue an error in the allowAmbiguity case? At least I think that's what @DanielRosenwasser was proposing.
Edit: I'm not sure an error here will be (1) always correct (2) sensible even if correct. If it's not both, then making this happen is probably difficult.
There was a problem hiding this comment.
It can never be the case that we're parsing a conditional and allowing ambiguity, as the conditional only tryParse's these to see if they are, then accepts them, otherwise tries some other expression type, hence the assert.
src/compiler/parser.ts
Outdated
| colonToken = parseExpectedToken(SyntaxKind.ColonToken), | ||
| nodeIsPresent(colonToken) | ||
| ? parseAssignmentExpressionOrHigher() | ||
| ? parseAssignmentExpressionOrHigher(/*parsingConditionalExpression*/ true) |
There was a problem hiding this comment.
I agree, I think a context flag would be easier to read and possibly faster. These are currently the only two places that are needed, right?
Actually, why are they both true? Where is false ever provided?
Edit: Oh, the parameter is optional. Yeah, I'd much prefer a context flag to a lot of identical optional parameters.
There was a problem hiding this comment.
I was worried you'd say that, darn.
I guess my main annoyance was how many "okay, it's time to not be in this context" now calls I would have to add to all of the calls that parseAssignmentExpressionOrHigher (and all of the function it calls), but that's no better than the for in thing I'm already not a fan of...
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 4d44f07. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 74a5786. You can monitor the build here. Update: The results are in! |
|
@jakebailey |
|
One option that would minimize breaking existing code would be leaving the handling as-is in TS files, and parsing it compliantly only in JS files -- like how the ambiguity with regard to generics is handled. |
|
@jakebailey Here they are:Comparison Report - main..47550
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 2958b46. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Pushed a change that is explicitly #16241 (comment). |
|
Wow, passes on my machine because I had renamed the test to something with different casing. Oops. |
…t of an arrow function (microsoft#47550)" This reverts commit 2dede20.
|
FYI, this PR has been reverted in #48940, pending a "real" fix. |
Fixes #16241
The easiest way to see this is by reading the emitted JS files.
DEMO: https://www.staging-typescript.org/play?target=0&ts=4.6.0-pr-47550-11#code/CYUwxgNghgTiAEA3W8oBp4CMNg8DIGAZhgB4YCeGAXgNwCwAUEwPQvwCMTp8A-PBXgBeAHzwAFAG8B8AL4BKeAC541YWKmq58prsZt4AJiaY+EsIpXB18EHtbsAzEyhnxmS-DA3xwTwlF4Int9dgAWFzNTfm8VX38bYOZkgwBWSP53RUDxCysbECA