Skip to content
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(babel-parser): delete static property from class static block for TS #13680

Merged
merged 6 commits into from Aug 25, 2021

Conversation

@sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Aug 15, 2021

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
Copy link
Collaborator

@babel-bot babel-bot commented Aug 15, 2021

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

@codesandbox
Copy link

@codesandbox codesandbox bot commented Aug 15, 2021

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

fedeci
fedeci approved these changes Aug 15, 2021
@@ -2396,6 +2396,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
const callParseClassMemberWithIsStatic = () => {
const isStatic = !!member.static;
if (isStatic && this.eat(tt.braceL)) {
delete member.static;
Copy link
Contributor

@JLHwung JLHwung Aug 15, 2021

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.

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 16, 2021

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 )?

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

@JLHwung JLHwung Aug 18, 2021

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

?

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

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 18, 2021

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

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

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 18, 2021

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)

Copy link
Contributor

@JLHwung JLHwung Aug 18, 2021

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

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 18, 2021

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

@sosukesuzuki sosukesuzuki force-pushed the fix-13674 branch 2 times, most recently from 57824b9 to d743bc8 Aug 18, 2021
@@ -1,9 +1,6 @@
{
"type": "File",
"start":0,"end":32,"loc":{"start":{"line":1,"column":0},"end":{"line":3,"column":1}},
"errors": [
"SyntaxError: Duplicate modifier: 'static'. (2:9)"
],
Copy link
Contributor

@JLHwung JLHwung Aug 20, 2021

This is a regression, the input is

class Foo {
  static static {}
}

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 24, 2021

@JLHwung Can you review again?

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

@JLHwung JLHwung Aug 24, 2021

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

Copy link
Member

@fedeci fedeci Aug 24, 2021

Copy link
Contributor

@JLHwung JLHwung Aug 24, 2021

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 {}.

Copy link
Member

@fedeci fedeci Aug 24, 2021

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

Copy link
Member Author

@sosukesuzuki sosukesuzuki Aug 25, 2021

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))

Copy link
Contributor

@JLHwung JLHwung Aug 25, 2021

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

fedeci
fedeci approved these changes Aug 24, 2021
@@ -1,3 +1,3 @@
class Foo {
static public {}
Copy link
Member

@fedeci fedeci Aug 24, 2021

@JLHwung JLHwung merged commit b141c85 into babel:main Aug 25, 2021
26 of 28 checks passed
@sosukesuzuki sosukesuzuki deleted the fix-13674 branch Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants