[ts] Check if param is assignable when parsing arrow return type #13581

nicolo-ribaudo · 2021-07-20T22:15:39Z

Q	A
Fixed Issues?	Fixes #11038
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This is the third attempt at fixing #11038:

Fix: check if param is assignable when parsing arrow return type annotation #11992, reverted by Revert "Fix: check if param is assignable when parsing arrow return type annotation" #12173 because the tests were failing
Reland "Fix: check if param is assignable when parsing arrow return type annotation" #12183, reverted by Revert "Reland "Fix: check if param is assignable when parsing arrow return type annotation"" #12198 because of [@babel/parser@7.12.1]: unexpected SyntaxError omitted from function params destructuring #12195 (test for the regression added by test: add test case for babel-parser: fixtures/typescript/arrow-function/destructuring-with-annotation-newline #12203)

nicolo-ribaudo · 2021-07-20T22:16:17Z

packages/babel-parser/src/parser/lval.js

+        return node.properties.every((prop, i) => {
+          return (
+            prop.type !== "ObjectMethod" &&
+            (i === last || prop.type !== "SpreadElement") &&


The bug that caused the regression was that this was using === "SpreadElement" instead of !==.

Why use 'every' ? It's slow vs a native for-loop

babel-bot · 2021-07-20T22:18:44Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47484/

codesandbox-ci · 2021-07-20T22:19:55Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 24c017a:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

nicolo-ribaudo · 2021-07-21T07:40:03Z

The failure is a regression.

packages/babel-parser/src/parser/expression.js

JLHwung · 2021-07-21T01:26:09Z

packages/babel-parser/src/parser/lval.js

@@ -60,7 +60,7 @@ export default class LValParser extends NodeUtils {
   - RestElement is not the last element
   - Missing `=` in assignment pattern

-   NOTE: There is a corresponding "isAssignable" method in flow.js.
+   NOTE: There is a corresponding "isAssignable" method.
   When this one is updated, please check if also that one needs to be updated.


This note can be removed then since it is moved to base parser.

I actually like this comment: even if they are in the same file, it's still easy to miss isAssignable when updating toAssignable.

KFlash · 2021-07-22T06:05:08Z

packages/babel-parser/src/parser/lval.js

@@ -235,6 +236,7 @@ export default class LValParser extends NodeUtils {
      case "ObjectPattern":
      case "ArrayPattern":
      case "AssignmentPattern":
+      case "RestElement":


@nicolo-ribaudo What about a lookup table? This switch is slow because of 1) string comparison 2) many cases 3) hard to optimize in v8

I don't think this switch relies on string comparisons: these strings are always constants in our code so V8 will atomize them and just compare them as pointers (but @JLHwung knows more than me about this)

v8 will convert this switch to a lookup, so it's faster if you do it.

KFlash · 2021-07-26T18:40:16Z

Merged, but performance not improved. What's the reason?

nicolo-ribaudo · 2021-07-26T20:50:50Z

That change doesn't have an observable impact on the normal @babel/parser usage.

…l#13581)

cederom · 2021-08-23T03:50:04Z

Thank you guys :-) Then this release will land into npm? 7.15.0 is still the latest one over there :-)

JLHwung · 2021-08-23T14:39:24Z

It is fixed in @babel/parser 7.15.3, @babel/parser is a dependency of @babel/core.

[ts] Check if param is assignable when parsing arrow return type

8a8c465

nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: typescript labels Jul 20, 2021

nicolo-ribaudo commented Jul 20, 2021

View reviewed changes

Fix flow

857c382

sosukesuzuki reviewed Jul 21, 2021

View reviewed changes

packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved

JLHwung reviewed Jul 21, 2021

View reviewed changes

Updates

24c017a

KFlash reviewed Jul 22, 2021

View reviewed changes

JLHwung approved these changes Jul 22, 2021

View reviewed changes

sosukesuzuki approved these changes Jul 26, 2021

View reviewed changes

nicolo-ribaudo merged commit 4a56387 into babel:main Jul 26, 2021

nicolo-ribaudo deleted the bug-11038 branch July 26, 2021 15:58

StorytellerCZ mentioned this pull request Jul 27, 2021

BABEL_PARSER_SYNTAX_ERROR error with 2.3.1 when using d3 meteor/meteor#11525

Closed

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 30, 2021

ts: Check if param is assignable when parsing arrow return type (babe…

0484086

…l#13581)

mbostock mentioned this pull request Aug 1, 2021

d3.cumsum fails to transpile with babel6 + preset-react d3/d3-array#147

Closed

sync-by-unito bot mentioned this pull request Aug 2, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.14 to 7.14.9 filecoin-project/slate#844

Closed

nicolo-ribaudo mentioned this pull request Aug 5, 2021

Fix array destructuring elision parsing in TS arrow functions #13641

Merged

This was referenced Aug 23, 2021

BUG: d3-array/dist/d3-array.js: Unexpected token (139:15) d3/d3-array#229

Closed

D3 latest version is using older version of d3-array d3/d3#3505

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 23, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ts] Check if param is assignable when parsing arrow return type #13581

[ts] Check if param is assignable when parsing arrow return type #13581

nicolo-ribaudo commented Jul 20, 2021 •

edited by gitpod-io bot

nicolo-ribaudo Jul 20, 2021

This comment was marked as resolved.

KFlash Jul 21, 2021

babel-bot commented Jul 20, 2021 •

edited

codesandbox-ci bot commented Jul 20, 2021 •

edited

nicolo-ribaudo commented Jul 21, 2021

JLHwung Jul 21, 2021

nicolo-ribaudo Jul 21, 2021

KFlash Jul 22, 2021

nicolo-ribaudo Jul 22, 2021

KFlash Jul 22, 2021

KFlash commented Jul 26, 2021

nicolo-ribaudo commented Jul 26, 2021

cederom commented Aug 23, 2021

JLHwung commented Aug 23, 2021

[ts] Check if param is assignable when parsing arrow return type #13581

[ts] Check if param is assignable when parsing arrow return type #13581

Conversation

nicolo-ribaudo commented Jul 20, 2021 • edited by gitpod-io bot

nicolo-ribaudo Jul 20, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

KFlash Jul 21, 2021

Choose a reason for hiding this comment

babel-bot commented Jul 20, 2021 • edited

codesandbox-ci bot commented Jul 20, 2021 • edited

nicolo-ribaudo commented Jul 21, 2021

JLHwung Jul 21, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Jul 21, 2021

Choose a reason for hiding this comment

KFlash Jul 22, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Jul 22, 2021

Choose a reason for hiding this comment

KFlash Jul 22, 2021

Choose a reason for hiding this comment

KFlash commented Jul 26, 2021

nicolo-ribaudo commented Jul 26, 2021

cederom commented Aug 23, 2021

JLHwung commented Aug 23, 2021

nicolo-ribaudo commented Jul 20, 2021 •

edited by gitpod-io bot

babel-bot commented Jul 20, 2021 •

edited

codesandbox-ci bot commented Jul 20, 2021 •

edited