From 7f1adddaf0eaee872fb818151ae534ea5fda94e9 Mon Sep 17 00:00:00 2001 From: Dunqing <29533304+Dunqing@users.noreply.github.com> Date: Thu, 11 Jul 2024 08:45:29 +0000 Subject: [PATCH] refactor(semantic): correct scope in CatchClause (#4192) close: #4186 CatchClause has two scopes. The first one is `CatchClause`, which will add a `CatchParameter` to it. The second one is `Block`, which will add binding that declares in the current block scope. The spec has a syntax error about `CatchParameter` - It is a Syntax Error if any element of the BoundNames of CatchParameter also occurs in the LexicallyDeclaredNames of Block. --- crates/oxc_ast/src/ast/js.rs | 2 +- crates/oxc_ast/src/generated/visit.rs | 9 ++----- crates/oxc_ast/src/generated/visit_mut.rs | 9 ++----- .../oxc_linter/src/rules/eslint/no_empty.rs | 16 +++++------- .../src/rules/unicorn/empty_brace_spaces.rs | 7 ----- crates/oxc_semantic/src/builder.rs | 26 +++++++++---------- .../snapshots/labeled_block_break.snap | 3 ++- tasks/coverage/parser_typescript.snap | 15 ++++++++++- 8 files changed, 40 insertions(+), 47 deletions(-) diff --git a/crates/oxc_ast/src/ast/js.rs b/crates/oxc_ast/src/ast/js.rs index 0a802c1f7..68ace391b 100644 --- a/crates/oxc_ast/src/ast/js.rs +++ b/crates/oxc_ast/src/ast/js.rs @@ -1300,7 +1300,7 @@ pub struct TryStatement<'a> { } #[visited_node] -#[scope(if(self.param.is_some()))] +#[scope] #[derive(Debug)] #[cfg_attr(feature = "serialize", derive(Serialize, Tsify))] #[cfg_attr(feature = "serialize", serde(tag = "type"))] diff --git a/crates/oxc_ast/src/generated/visit.rs b/crates/oxc_ast/src/generated/visit.rs index 6a633fd00..fa2697957 100644 --- a/crates/oxc_ast/src/generated/visit.rs +++ b/crates/oxc_ast/src/generated/visit.rs @@ -3690,10 +3690,7 @@ pub mod walk { #[inline] pub fn walk_catch_clause<'a, V: Visit<'a>>(visitor: &mut V, it: &CatchClause<'a>) { - let scope_events_cond = it.param.is_some(); - if scope_events_cond { - visitor.enter_scope(ScopeFlags::empty(), &it.scope_id); - } + visitor.enter_scope(ScopeFlags::empty(), &it.scope_id); let kind = AstKind::CatchClause(visitor.alloc(it)); visitor.enter_node(kind); if let Some(param) = &it.param { @@ -3701,9 +3698,7 @@ pub mod walk { } visitor.visit_block_statement(&it.body); visitor.leave_node(kind); - if scope_events_cond { - visitor.leave_scope(); - } + visitor.leave_scope(); } #[inline] diff --git a/crates/oxc_ast/src/generated/visit_mut.rs b/crates/oxc_ast/src/generated/visit_mut.rs index 2103a64a5..6c89eb082 100644 --- a/crates/oxc_ast/src/generated/visit_mut.rs +++ b/crates/oxc_ast/src/generated/visit_mut.rs @@ -3895,10 +3895,7 @@ pub mod walk_mut { #[inline] pub fn walk_catch_clause<'a, V: VisitMut<'a>>(visitor: &mut V, it: &mut CatchClause<'a>) { - let scope_events_cond = it.param.is_some(); - if scope_events_cond { - visitor.enter_scope(ScopeFlags::empty(), &it.scope_id); - } + visitor.enter_scope(ScopeFlags::empty(), &it.scope_id); let kind = AstType::CatchClause; visitor.enter_node(kind); if let Some(param) = &mut it.param { @@ -3906,9 +3903,7 @@ pub mod walk_mut { } visitor.visit_block_statement(&mut it.body); visitor.leave_node(kind); - if scope_events_cond { - visitor.leave_scope(); - } + visitor.leave_scope(); } #[inline] diff --git a/crates/oxc_linter/src/rules/eslint/no_empty.rs b/crates/oxc_linter/src/rules/eslint/no_empty.rs index 74c961351..472942365 100644 --- a/crates/oxc_linter/src/rules/eslint/no_empty.rs +++ b/crates/oxc_linter/src/rules/eslint/no_empty.rs @@ -48,21 +48,17 @@ impl Rule for NoEmpty { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { match node.kind() { AstKind::BlockStatement(block) if block.body.is_empty() => { + if self.allow_empty_catch + && matches!(ctx.nodes().parent_kind(node.id()), Some(AstKind::CatchClause(_))) + { + return; + } + if ctx.semantic().trivias().has_comments_between(block.span) { return; } ctx.diagnostic(no_empty_diagnostic("block", block.span)); } - // The visitor does not visit the `BlockStatement` inside the `CatchClause`. - // See `Visit::visit_catch_clause`. - AstKind::CatchClause(catch_clause) - if !self.allow_empty_catch && catch_clause.body.body.is_empty() => - { - if ctx.semantic().trivias().has_comments_between(catch_clause.body.span) { - return; - } - ctx.diagnostic(no_empty_diagnostic("block", catch_clause.body.span)); - } // The visitor does not visit the `BlockStatement` inside the `FinallyClause`. // See `Visit::visit_finally_clause`. AstKind::FinallyClause(finally_clause) if finally_clause.body.is_empty() => { diff --git a/crates/oxc_linter/src/rules/unicorn/empty_brace_spaces.rs b/crates/oxc_linter/src/rules/unicorn/empty_brace_spaces.rs index 7cd316ac1..56d7dec19 100644 --- a/crates/oxc_linter/src/rules/unicorn/empty_brace_spaces.rs +++ b/crates/oxc_linter/src/rules/unicorn/empty_brace_spaces.rs @@ -62,13 +62,6 @@ impl Rule for EmptyBraceSpaces { AstKind::BlockStatement(block_stmt) => { remove_empty_braces_spaces(ctx, block_stmt.body.is_empty(), block_stmt.span); } - AstKind::CatchClause(catch_clause) => { - remove_empty_braces_spaces( - ctx, - catch_clause.body.body.is_empty(), - catch_clause.body.span, - ); - } AstKind::FinallyClause(finally_clause) => { remove_empty_braces_spaces( ctx, diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index 710ec475a..0302ebb22 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -1348,19 +1348,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> { self.leave_node(kind); } - fn visit_catch_clause(&mut self, clause: &CatchClause<'a>) { - let kind = AstKind::CatchClause(self.alloc(clause)); - self.enter_scope(ScopeFlags::empty(), &clause.scope_id); - clause.scope_id.set(Some(self.current_scope_id)); - self.enter_node(kind); - if let Some(param) = &clause.param { - self.visit_catch_parameter(param); - } - self.visit_statements(&clause.body.body); - self.leave_node(kind); - self.leave_scope(); - } - fn visit_finally_clause(&mut self, clause: &BlockStatement<'a>) { let kind = AstKind::FinallyClause(self.alloc(clause)); self.enter_scope(ScopeFlags::empty(), &clause.scope_id); @@ -1826,6 +1813,19 @@ impl<'a> SemanticBuilder<'a> { AstKind::YieldExpression(_) => { self.set_function_node_flag(NodeFlags::HasYield); } + AstKind::BlockStatement(_) => { + if matches!( + self.nodes.parent_kind(self.current_node_id), + Some(AstKind::CatchClause(_)) + ) { + // Clone the `CatchClause` bindings and add them to the current scope. + // to make it easier to check redeclare errors. + if let Some(parent_scope_id) = self.scope.get_parent_id(self.current_scope_id) { + let bindings = self.scope.get_bindings(parent_scope_id).clone(); + self.scope.get_bindings_mut(self.current_scope_id).extend(bindings); + } + } + } _ => {} } } diff --git a/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap b/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap index 1cae2d252..1253f3eaf 100644 --- a/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap +++ b/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap @@ -23,6 +23,7 @@ bb4: { statement statement statement + statement } bb5: { @@ -55,7 +56,7 @@ digraph { 1 [ label = "TryStatement" ] 2 [ label = "" ] 3 [ label = "BlockStatement" ] - 4 [ label = "LabeledStatement\nBlockStatement\nIfStatement" ] + 4 [ label = "BlockStatement\nLabeledStatement\nBlockStatement\nIfStatement" ] 5 [ label = "Condition(IdentifierReference(condition))" ] 6 [ label = "BlockStatement\nbreak