-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 arrow expressions in conditional expressions, take N+1 #49531
Fix arrow expressions in conditional expressions, take N+1 #49531
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@typescript-bot test this |
Heya @weswigham, I've started to run the extended test suite on this PR at 7ac56c3. You can monitor the build here. |
Heya @weswigham, I've started to run the tarball bundle task on this PR at 7ac56c3. You can monitor the build here. |
Heya @weswigham, I've started to run the perf test suite on this PR at 7ac56c3. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the diff-based user code test suite on this PR at 7ac56c3. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the abridged perf test suite on this PR at 7ac56c3. You can monitor the build here. Update: The results are in! |
@weswigham |
Heya @weswigham, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@weswigham Here they are:Comparison Report - main..49531
System
Hosts
Scenarios
Developer Information: |
@typescript-bot run dt |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 7ac56c3. You can monitor the build here. |
@typescript-bot pack this |
Heya @DanielRosenwasser, I'm starting to run the extended test suite on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. |
Heya @DanielRosenwasser, I'm starting to run the perf test suite on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @DanielRosenwasser, I'm starting to run the diff-based user code test suite on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. |
Heya @DanielRosenwasser, I'm starting to run the tarball bundle task on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. |
Heya @DanielRosenwasser, I'm starting to run the abridged perf test suite on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. |
Heya @DanielRosenwasser, I'm starting to run the parallelized Definitely Typed test suite on this PR at 8675dc1. Hold tight - I'll update this comment with the log link once the build has been queued. |
colonToken = parseExpectedToken(SyntaxKind.ColonToken), | ||
nodeIsPresent(colonToken) | ||
? parseAssignmentExpressionOrHigher() | ||
? parseAssignmentExpressionOrHigher(allowReturnTypeInArrowFunction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion over at #49531 (comment) about whether or we should have consistency between the true/false branches, or whether backwards compatibility should win and we propagate outer context for the false case.
In other words, should the following continue to parse?
a ? b : (c): Type => c
While we can propagate the flag here, having the two branches differ is confusing, and could make naive refactorings fail. I have a soft vote for disallowing in conditional types, but explicitly document it.
What do you think @ahejlsberg @sandersn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pro-backwards-compatibility; this was an oversight in that I had it this way in a previous change, but while working on one of the newer examples from the wild, I switched it back until I found the "real" fix as the token === ":" check in the arrow function parsing function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant test case is e76fe4c
(#49531), which I fixed in 8675dc1
(#49531) before I had a test case.
@@ -1,8 +0,0 @@ | |||
//// [parserArrowFunctionExpression10.ts] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this is a rename, not a deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's why I wanted to have my first commit be only test changes, so the following commits were clear as to what changed.
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
||
|
||
==== tests/cases/conformance/parser/ecmascript5/ArrowFunctionExpressions/fileJs.js (7 errors) ==== | ||
a() ? (b: number, c?: string): void => d() : e; // Not legal JS; "Unexpected token ':'" at first colon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment supposed to reflect an example runtime's error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the parser error I get from node or a browser. I can drop these if they are confusing, I only had them in there to convince myself and reviewers that in every case, the only ones with TS-y meanings were ones that were not valid JavaScript.
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
RWC and DT look good, perf is fine as well. Going to merge this to get this in for the beta. |
@DanielRosenwasser Here they are:Comparison Report - main..49531
System
Hosts
Scenarios
Developer Information: |
Fixes #16241
See also: #48733
This is a revert of PR #48940 with #48788 applied.
I previously described #48788 as "permissive"; this is accurate in the sense that all of the existing TS code that we found in the wild is parsed correctly without a change in meaning. However, this does not compromise on the ideal that if it's legal JS, then we should parse it as such; the effect of this PR should be to get everything working as expected.
I recommend ignoring the first commit for sake of viewing the diff, as it's just setting up the tests in such a way to make sure we can view the JS and TS code side by side with the output.
If it's legal JS, we want to parse it as such. This means that in cases where the code is legal JS, we should see that:
If it's a syntax error in JS, then we are free to interpret it how we need, i.e. to support TS features. Many of the tests are not legal JS.