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

Make all scopes unconditional #5008

Closed
overlookmotel opened this issue Aug 20, 2024 · 10 comments · Fixed by #5114
Closed

Make all scopes unconditional #5008

overlookmotel opened this issue Aug 20, 2024 · 10 comments · Fixed by #5114
Assignees
Labels
A-semantic Area - Semantic C-enhancement Category - New feature or request E-Help Wanted Experience level - For the experienced collaborators

Comments

@overlookmotel
Copy link
Contributor

Almost all nodes which can hold a scope always hold a scope. But a small handful only create a scope if required:

  • ForStatement
  • ForInStatement
  • ForOfStatement
  • CatchClause

I propose that we make all scopes unconditional i.e. all these nodes always have a scope.

My rationale is:

  • Simplicity - especially in transformer where scope-related logic is already complicated.
  • In most JS code, these nodes will create a scope anyway (for (let item of arr) is more common than for (item of arr), and catch (err) { is much more common than catch {).
  • Resolving references against a scope which has no bindings is very cheap.
  • These are not the most common constructs (vs if or function), so even if it wasn't so cheap, a few extra unnecessary scopes would not hurt perf much anyway.

If I remember right, Dunqing was in favour of this a while back, but I resisted. At that point I didn't realise how few nodes had conditional scopes, so I thought the perf upside of eliding them was much bigger than it is.

@overlookmotel overlookmotel added the A-semantic Area - Semantic label Aug 20, 2024
@Dunqing
Copy link
Member

Dunqing commented Aug 22, 2024

  • Simplicity - especially in transformer where scope-related logic is already complicated.

Yes, I agreed

@Dunqing Dunqing added C-enhancement Category - New feature or request E-Help Wanted Experience level - For the experienced collaborators labels Aug 22, 2024
@Dunqing
Copy link
Member

Dunqing commented Aug 22, 2024

We need to remove these conditional in SemanticBuilder

let is_lexical_declaration =
stmt.init.as_ref().is_some_and(ForStatementInit::is_lexical_declaration);
if is_lexical_declaration {
self.enter_scope(ScopeFlags::empty(), &stmt.scope_id);
}

@overlookmotel
Copy link
Contributor Author

Glad you agree!

We would also need to remove the #[scope(if(...))] attrs from AST (and remove support for that attr from ast_tools and from Traverse codegen).

@overlookmotel
Copy link
Contributor Author

@Boshen Would you be happy with this change?

@Dunqing
Copy link
Member

Dunqing commented Aug 22, 2024

@Boshen Would you be happy with this change?

I bet @Boshen totally agree this, he likes the standard way to handle scopes 😀

@Boshen
Copy link
Member

Boshen commented Aug 22, 2024

:shipit:

@overlookmotel
Copy link
Contributor Author

:shipit:

What does a mouse dressed as a detective mean??!

@Boshen
Copy link
Member

Boshen commented Aug 23, 2024

:shipit:

What does a mouse dressed as a detective mean??!

:shipit: https://www.quora.com/On-GitHub-what-is-the-significance-of-the-Ship-It-squirrel

Sorry I'm old.

@overlookmotel
Copy link
Contributor Author

Not as old as me! But thanks for explaining.

Boshen pushed a commit that referenced this issue Aug 23, 2024
Part of #5008. Make scope for `CatchClause` unconditional. i.e. always create a scope, even if there is no catch parameter.
Dunqing pushed a commit that referenced this issue Aug 23, 2024
Part of #5008. Make scopes for `ForStatement`, `ForInStatement` and `ForOfStatement` unconditional. i.e. always create a scope, even if there is no lexical binding (e.g. `for (i of a) {}`).
Dunqing pushed a commit that referenced this issue Aug 23, 2024
Closes #5008.

There are no longer any nodes with conditional scopes. Remove support for `#[scope(if(...))]` attr from `oxc_traverse` codegen - it's no longer needed.
@overlookmotel
Copy link
Contributor Author

Implemented in #5114 and preceding PRs.

Dunqing pushed a commit that referenced this issue Aug 23, 2024
Part of #5008. Make scopes for `ForStatement`, `ForInStatement` and `ForOfStatement` unconditional. i.e. always create a scope, even if there is no lexical binding (e.g. `for (i of a) {}`).
Dunqing pushed a commit that referenced this issue Aug 23, 2024
Closes #5008.

There are no longer any nodes with conditional scopes. Remove support for `#[scope(if(...))]` attr from `oxc_traverse` codegen - it's no longer needed.
overlookmotel pushed a commit that referenced this issue Aug 23, 2024
…in the correct scope (#5066)

Blocked by #5008

In the SemanticBuilder, we insert all CatchClause parameters in the BlockStatement scope, a child-scope of CatchClause

https://github.com/oxc-project/oxc/blob/858f510d59cb9ea29cca36c31fdf037b0d627f2c/crates/oxc_semantic/src/builder.rs#L709-L718

So we should do the same thing in this plugin, but because CatchClause has no parameters, SemanticBuilder doesn't create scope for `CatchClause`. This cause we cannot find the `BlockStatement` scope_id.

This PR has changed to the correct logic. Just to wait #5008 solved
Boshen pushed a commit that referenced this issue Aug 27, 2024
#5252)

All scopes are now unconditional (#5008). Update transform checker to reflect this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic C-enhancement Category - New feature or request E-Help Wanted Experience level - For the experienced collaborators
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants