refactor(parser): remove refNeedsArrowPos by tony-go · Pull Request #13419 · babel/babel

tony-go · 2021-06-03T16:33:58Z

Q
Fixed Issues?	⛔
Patch: Bug Fix?	⛔
Major: Breaking Change?	⛔
Minor: New Feature?	⛔
Tests Added + Pass?	👍
Documentation PR Link	⛔
Any Dependency Changes?	⛔
License	MIT

Hey @JLHwung 👋

Following your comment on my previous PR, I open this draft PR. Let me know If I'm wrong or If I misunderstand something.

🚀

babel-bot · 2021-06-03T16:36:40Z

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

nicolo-ribaudo · 2021-06-03T17:04:51Z

The CI failure seems to be unrelated.

tony-go · 2021-06-03T17:17:57Z

The CI failure seems to be unrelated.

I received the same message for my previous PR.

JLHwung · 2021-06-03T18:03:21Z

I am wondering if we could remove all the refNeedsArrowPos usage, according to

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

Lines 346 to 348 in a0369fd

    
           // FIXME: Disabling this for now since can't seem to get it to play nicely 
        
           // eslint-disable-next-line no-unused-vars 
        
           refNeedsArrowPos?: ?Pos,

it is not engaging with the parser.

The refNeedsArrowPos is changed only in Flow / TypeScript plugin:

babel/packages/babel-parser/src/plugins/flow/index.js

Line 1926 in a0369fd

refNeedsArrowPos.start = result.error.pos || this.state.start;

babel/packages/babel-parser/src/plugins/typescript/index.js

Line 2474 in a0369fd

refNeedsArrowPos.start = result.error.pos || this.state.start;

We could try removing it and see if it breaks any test.

tony-go · 2021-06-04T07:20:19Z

We could try removing it and see if it breaks any test.

Hey 👋 @JLHwung ^^ It breaks a lot of tests (20 at least) and as you predicted it's on flow an typescript tests.

For example in this code:

babel/packages/babel-parser/src/plugins/flow/index.js

Line 1926 in a0369fd

refNeedsArrowPos.start = result.error.pos || this.state.start;

Maybe could we pass a boolean instead of refNeedsArrowPos as it's only use to match a branch and .start is reset with result.error.pos || this.state.start.

WDYT ?

tony-go · 2021-06-07T09:34:44Z

Ping @JLHwung ^^

JLHwung · 2021-06-08T12:34:13Z

@tony-go refNeedsArrowPos.start is a number, Babel parser throws when it is not zero. So I guess boolean does not work here.

It seems to me refNeedsArrowPos is set in flow/typescript plugin only. Maybe we can move

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

Line 1537 in 56db172

if (refNeedsArrowPos.start) this.unexpected(refNeedsArrowPos.start);

to these plugins so when parsing plain JavaScript we don't care refNeedsArrowPos.

tony-go · 2021-06-09T10:23:26Z

Hi 👋 @JLHwung

Regarding the issue #13419 I pushed a test branch (which contains the move of if (refNeedsArrowPos.start) this.unexpected(refNeedsArrowPos.start); in the flow/ts files.)

Where I’m trying to figured out why test fails :/

Even with the callstack debugger I didn’t understand why en expression like const f = (x?) => {} throw there. (modifié)

tony-go · 2021-06-11T06:32:36Z

Update: I continue to dig with the help of @JLHwung ^^

We merge few things but it still remains two tests which fails. I'm actively working on it ^^

Sorry for the wait.

codesandbox-ci · 2021-06-11T14:43:33Z

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 6d9321e:

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

tony-go · 2021-06-11T15:37:11Z

Let me fix linter :/

JLHwung · 2021-06-11T15:53:34Z

-        this.parseExprListItem(
-          false,
-          refExpressionErrors,
-          { start: 0 },


Q: refNeedsArrowPos is reset here. Do we need to reset refExpressionErrors.optionalParameters to -1? Maybe worth a test covering this.

nicolo-ribaudo

I like how there are more deleted lines than added lines 😄

tony-go · 2021-06-11T20:20:09Z

Did all corrections.

Just left this [comment](#13419 (comment):

Q: refNeedsArrowPos is reset here. Do we need to reset refExpressionErrors.optionalParameters to -1? Maybe worth a test covering this.

As I did found the test relevant test case for now :/

JLHwung

The PR generally looks good to me! Left some nit comments on extra checking.

JLHwung

Thanks!

tony-go · 2021-06-18T20:46:05Z

Thanks!

Thanks you for you time. You (and @nicolo-ribaudo) provide me an excellent OSS experience ^^

nicolo-ribaudo

I'm surprised that we need these changed in code that looks unrelated 🤔

nicolo-ribaudo

🎉

fix(parser:expression): remove refNeedsArrowPos in parseExprListItem

9bb8274

nicolo-ribaudo added the pkg: parser label Jun 3, 2021

tony-go mentioned this pull request Jun 11, 2021

[Bug]: Omitted spread properties are incorrectly hoisted when using computed properties with template strings #13430

Closed

1 task

feat: use refExpressionErrors to catch optional parameters

6b29e13

tony-go added 2 commits June 11, 2021 16:52

feat: delete useless refNeedArrowPos

b443d89

fix: forgotten deletion

3fa08d2

JLHwung reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

JLHwung reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

JLHwung reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/plugins/flow/index.js Outdated

JLHwung reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/util.js

JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jun 11, 2021

JLHwung changed the title ~~fix(parser:expression): remove refNeedsArrowPos in parseExprListItem~~ refactor(parser): remove refNeedsArrowPos Jun 11, 2021

tony-go added 3 commits June 11, 2021 18:04

fix: lint and factorize

2ad5050

fix: delete useless comment

1950d7d

fix: review corrections

0a35f8a

nicolo-ribaudo reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

Comment thread packages/babel-parser/src/parser/expression.js Outdated

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

JLHwung reviewed Jun 11, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/util.js

fix: review corrections pt2

cbbc10a

tony-go requested review from JLHwung and nicolo-ribaudo June 11, 2021 20:20

JLHwung approved these changes Jun 14, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

Comment thread packages/babel-parser/src/plugins/flow/index.js Outdated

Comment thread packages/babel-parser/src/plugins/flow/index.js Outdated

Comment thread packages/babel-parser/src/parser/expression.js Outdated

fix: review correction pt2

9666004

tony-go requested a review from JLHwung June 16, 2021 13:03

nicolo-ribaudo reviewed Jun 16, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

fix: nit correction

da283bf

tony-go requested a review from nicolo-ribaudo June 18, 2021 10:19

tony-go mentioned this pull request Jun 18, 2021

[Bug]: async(x?) should throw #13446

Closed

1 task

JLHwung reviewed Jun 18, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/util.js

JLHwung reviewed Jun 18, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

JLHwung reviewed Jun 18, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/expression.js Outdated

fix: update types

63be4e1

tony-go requested a review from JLHwung June 18, 2021 14:19

JLHwung approved these changes Jun 18, 2021

View reviewed changes

nicolo-ribaudo reviewed Jun 18, 2021

View reviewed changes

Comment thread packages/babel-parser/src/parser/util.js Outdated

Update packages/babel-parser/src/parser/util.js

fe20d8c

nicolo-ribaudo reviewed Jun 18, 2021

View reviewed changes

Comment thread packages/babel-parser/src/plugins/flow/index.js Outdated

Comment thread packages/babel-parser/src/plugins/typescript/index.js Outdated

Comment thread packages/babel-parser/src/plugins/typescript/index.js Outdated

fix: add invariant comment in flow and ts file

6d9321e

nicolo-ribaudo approved these changes Jun 19, 2021

View reviewed changes

nicolo-ribaudo merged commit 101249f into babel:main Jun 19, 2021

SudharakaP mentioned this pull request Jun 21, 2021

Propose RFC for Merging PRs opencollective/opencollective#4367

Merged

sync-by-unito Bot mentioned this pull request Jun 22, 2021

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

Closed

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

github-actions Bot locked as resolved and limited conversation to collaborators Sep 19, 2021

Uh oh!

Conversation

tony-go commented Jun 3, 2021 • edited by nicolo-ribaudo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Jun 3, 2021

Uh oh!

tony-go commented Jun 3, 2021

Uh oh!

JLHwung commented Jun 3, 2021

Uh oh!

tony-go commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony-go commented Jun 7, 2021

Uh oh!

JLHwung commented Jun 8, 2021

Uh oh!

tony-go commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony-go commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codesandbox-ci Bot commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony-go commented Jun 11, 2021

Uh oh!

Uh oh!

JLHwung Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tony-go commented Jun 11, 2021

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

tony-go commented Jun 18, 2021

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tony-go commented Jun 3, 2021 •

edited by nicolo-ribaudo

Loading

babel-bot commented Jun 3, 2021 •

edited

Loading

tony-go commented Jun 4, 2021 •

edited

Loading

tony-go commented Jun 9, 2021 •

edited

Loading

tony-go commented Jun 11, 2021 •

edited

Loading

codesandbox-ci Bot commented Jun 11, 2021 •

edited

Loading