fix(babel-parser): delete `static` property from class static block for TS by sosukesuzuki · Pull Request #13680 · babel/babel

sosukesuzuki · 2021-08-15T16:50:22Z

Q	A
Fixed Issues?	Fixes #13674
Patch: Bug Fix?	Y
Tests Added + Pass?	Yes
License	MIT

context: I noticed the bug when implementing to support static blocks for typescript-estree (ref :https://github.com/typescript-eslint/typescript-eslint/pull/3730/files#diff-f3405000b53c5abe00cbd3f4852b524d0e84478a76b8f8da33eecfe9acfc97f4R253-R256)

babel-bot · 2021-08-15T16:53:18Z

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

codesandbox-ci · 2021-08-15T16:56:31Z

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 828b241:

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

JLHwung · 2021-08-15T23:29:42Z

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

      const callParseClassMemberWithIsStatic = () => {
        const isStatic = !!member.static;
        if (isStatic && this.eat(tt.braceL)) {
+          delete member.static;


Deleting a property should be generally avoided (https://stackoverflow.com/a/44008788/1490357), especially when it is executed for normal execution path: static blocks. I think we can check if a tt.braceL is upcoming before member.static is assigned in tsParseModifiers.

It was difficult to avoid to attach static in tsParseModifiers without failing existing tests. So what do you think about to separate attaching modifier properties and parsing ( d5077ff )?

JLHwung · 2021-08-18T13:48:30Z

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

+        } else if (modifier !== "static") {
+          member[modifier] = true;
+        }
+      }


Object clone is also slow. Can we check tt.braceL when modifier === "static" in

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

Line 306 in fc66d4d

modified[modifier] = true;

?

For cases like static declare { }, it doesn't matter if static is set because static block can not have any modifiers.

We can check it in parseModifiers. But if we do that, we cannot know from the parseClassMember whether a member is static or not in

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

Lines 2397 to 2398 in fc66d4d

const isStatic = !!member.static;

if (isStatic && this.eat(tt.braceL)) {

It breaks some invalid test cases( static block has modifier ), but able to avoid deleting properties and copying objects by lookahead ( lookaheadCharCode ) them. What do you think? (f09c7f6, 57824b9)

In that case we can have parseModifiers stopped at static { so we can still recover from cases when modifiers are on static block.

See also https://github.com/microsoft/TypeScript/blob/7a19c22063660da0b2f03fca97178679814de1e7/src/compiler/parser.ts#L6916-L6919

Thanks for telling it me. I've fixed by referring to the TypeScript parser code ( d743bc8 )

JLHwung · 2021-08-20T15:45:53Z

...test/fixtures/typescript/static-blocks/invalid-static-block-with-modifier-static/output.json

  "start":0,"end":32,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
-  "errors": [
-    "SyntaxError: Duplicate modifier: 'static'. (2:9)"
-  ],


This is a regression, the input is

class Foo { static static {} }

@JLHwung Can you review again?

JLHwung · 2021-08-24T14:38:58Z

...st/fixtures/typescript/static-blocks/invalid-static-block-with-accessibility-public/input.ts

@@ -1,3 +1,3 @@
 class Foo {
-  static public {}


Q: Do we already have test cases covering static public?

Yes we do.

babel/packages/babel-parser/test/fixtures/typescript/class/modifiers-invalid-order/input.ts

Line 21 in 1d4bd31

static public m13() {}

That one is a static public class method. I mean static public {}, which should throw. Just wondering why the test case is changed, we can always add new test cases for public static {}.

Oops sorry, I misread the changed one🤦‍♂️

Thanks, I've added more tests (5ea75b9, 828b241). However, the syntax static followed by modifier followed by block({ }) is not recoverable. (TypeScript Compiler also doesn't seem to be able to throw a friendly error for such code (TypeScript Playground link))

Yeah we can always improve it later as long as we don't pass invalid input.

fedeci · 2021-08-24T15:45:25Z

...st/fixtures/typescript/static-blocks/invalid-static-block-with-accessibility-public/input.ts

@@ -1,3 +1,3 @@
 class Foo {
-  static public {}


Yes we do.

babel/packages/babel-parser/test/fixtures/typescript/class/modifiers-invalid-order/input.ts

Line 21 in 1d4bd31

static public m13() {}

sosukesuzuki added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: typescript labels Aug 15, 2021

fedeci approved these changes Aug 15, 2021

View reviewed changes

JLHwung reviewed Aug 15, 2021

View reviewed changes

JLHwung reviewed Aug 18, 2021

View reviewed changes

sosukesuzuki force-pushed the fix-13674 branch 2 times, most recently from 57824b9 to d743bc8 Compare August 18, 2021 22:42

JLHwung reviewed Aug 20, 2021

View reviewed changes

JLHwung reviewed Aug 24, 2021

View reviewed changes

JLHwung approved these changes Aug 24, 2021

View reviewed changes

fedeci approved these changes Aug 24, 2021

View reviewed changes

existentialism approved these changes Aug 24, 2021

View reviewed changes

sosukesuzuki added 6 commits August 25, 2021 15:02

Delete static modifier from static blocks node

a18637b

Update tests

cb5bff3

Address review

801aa8e

Address review

7fc070d

Add tests from review

5ea75b9

Add tests

828b241

sosukesuzuki force-pushed the fix-13674 branch from 94215e2 to 828b241 Compare August 25, 2021 06:16

JLHwung approved these changes Aug 25, 2021

View reviewed changes

JLHwung merged commit b141c85 into babel:main Aug 25, 2021

sosukesuzuki deleted the fix-13674 branch August 26, 2021 03:09

sosukesuzuki mentioned this pull request Sep 4, 2021

test(babel-parser): add tests for static blocks with line breaks #13734

Merged

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

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

	const isStatic = !!member.static;
	if (isStatic && this.eat(tt.braceL)) {

Uh oh!

Conversation

sosukesuzuki commented Aug 15, 2021 • edited by gitpod-io bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codesandbox-ci bot commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sosukesuzuki commented Aug 15, 2021 •

edited by gitpod-io bot

Loading

babel-bot commented Aug 15, 2021 •

edited

Loading

codesandbox-ci bot commented Aug 15, 2021 •

edited

Loading