From 3db9e949c94879b366310ddda5752c9d82ffcd49 Mon Sep 17 00:00:00 2001 From: Boshen Date: Fri, 23 Aug 2024 23:10:24 +0800 Subject: [PATCH] refactor(semantic): rewrite handling of label statement errors This reverts the previous changes to handling labels so that all tests can pass. This passes all false postivies found in `monitor-oxc` (node_modules/flow-parser/flow_parser.js) --- crates/oxc_semantic/src/builder.rs | 26 +-- crates/oxc_semantic/src/checker/javascript.rs | 128 +++++++------- crates/oxc_semantic/src/checker/mod.rs | 3 +- crates/oxc_semantic/src/label.rs | 161 +++--------------- crates/oxc_semantic/src/lib.rs | 5 +- tasks/coverage/parser_test262.snap | 10 +- tasks/coverage/parser_typescript.snap | 11 +- 7 files changed, 109 insertions(+), 235 deletions(-) diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index 4977834c96c425..5c68400be0e198 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -24,7 +24,7 @@ use crate::{ counter::Counter, diagnostics::redeclaration, jsdoc::JSDocBuilder, - label::LabelBuilder, + label::UnusedLabels, module_record::ModuleRecordBuilder, node::{AstNodeId, AstNodes, NodeFlags}, reference::{Reference, ReferenceFlags, ReferenceId}, @@ -94,8 +94,7 @@ pub struct SemanticBuilder<'a> { pub(crate) module_record: Arc, - pub(crate) label_builder: LabelBuilder<'a>, - + unused_labels: UnusedLabels<'a>, build_jsdoc: bool, jsdoc: JSDocBuilder<'a>, @@ -141,7 +140,7 @@ impl<'a> SemanticBuilder<'a> { symbols: SymbolTable::default(), unresolved_references: UnresolvedReferencesStack::new(), module_record: Arc::new(ModuleRecord::default()), - label_builder: LabelBuilder::default(), + unused_labels: UnusedLabels::default(), build_jsdoc: false, jsdoc: JSDocBuilder::new(source_text, trivias), check_syntax_error: false, @@ -271,7 +270,7 @@ impl<'a> SemanticBuilder<'a> { classes: self.class_table_builder.build(), module_record: Arc::clone(&self.module_record), jsdoc, - unused_labels: self.label_builder.unused_node_ids, + unused_labels: self.unused_labels.labels, cfg: self.cfg.map(ControlFlowGraphBuilder::build), }; SemanticBuilderReturn { semantic, errors: self.errors.into_inner() } @@ -1755,13 +1754,11 @@ impl<'a> SemanticBuilder<'a> { decl.bind(self); self.make_all_namespaces_valuelike(); } - AstKind::StaticBlock(_) => self.label_builder.enter_function_or_static_block(), AstKind::Function(func) => { self.function_stack.push(self.current_node_id); if func.is_declaration() { func.bind(self); } - self.label_builder.enter_function_or_static_block(); self.make_all_namespaces_valuelike(); } AstKind::ArrowFunctionExpression(_) => { @@ -1894,12 +1891,12 @@ impl<'a> SemanticBuilder<'a> { self.current_reference_flags |= ReferenceFlags::Write; } AstKind::LabeledStatement(stmt) => { - self.label_builder.enter(stmt, self.current_node_id); + self.unused_labels.add(stmt.label.name.as_str()); } AstKind::ContinueStatement(ContinueStatement { label, .. }) | AstKind::BreakStatement(BreakStatement { label, .. }) => { if let Some(label) = &label { - self.label_builder.mark_as_used(label); + self.unused_labels.reference(&label.name); } } AstKind::YieldExpression(_) => { @@ -1927,15 +1924,7 @@ impl<'a> SemanticBuilder<'a> { self.current_reference_flags = ReferenceFlags::empty(); } } - AstKind::LabeledStatement(_) => self.label_builder.leave(), - AstKind::StaticBlock(_) => { - self.label_builder.leave_function_or_static_block(); - } - AstKind::Function(_) => { - self.label_builder.leave_function_or_static_block(); - self.function_stack.pop(); - } - AstKind::ArrowFunctionExpression(_) => { + AstKind::Function(_) | AstKind::ArrowFunctionExpression(_) => { self.function_stack.pop(); } AstKind::FormalParameters(parameters) => { @@ -1975,6 +1964,7 @@ impl<'a> SemanticBuilder<'a> { self.current_reference_flags = ReferenceFlags::empty(); } AstKind::AssignmentTarget(_) => self.current_reference_flags -= ReferenceFlags::Write, + AstKind::LabeledStatement(_) => self.unused_labels.mark_unused(self.current_node_id), _ => {} } } diff --git a/crates/oxc_semantic/src/checker/javascript.rs b/crates/oxc_semantic/src/checker/javascript.rs index 7df5ff04a2fdab..2995d36a57b407 100644 --- a/crates/oxc_semantic/src/checker/javascript.rs +++ b/crates/oxc_semantic/src/checker/javascript.rs @@ -554,31 +554,6 @@ fn invalid_label_non_iteration(x0: &str, span1: Span, span2: Span) -> OxcDiagnos ]) } -fn check_label(label: &LabelIdentifier, ctx: &SemanticBuilder, is_continue: bool) { - if ctx.label_builder.is_inside_labeled_statement() { - for labeled in ctx.label_builder.get_accessible_labels() { - if label.name == labeled.name { - if is_continue - && matches!(ctx.nodes.kind(labeled.id), AstKind::LabeledStatement(stmt) if { - let mut body = &stmt.body; - while let Statement::LabeledStatement(stmt) = body { - body = &stmt.body; - } - !body.is_iteration_statement() - }) - { - ctx.error(invalid_label_non_iteration("continue", labeled.span, label.span)); - } - return; - } - } - if ctx.label_builder.is_inside_function_or_static_block() { - return ctx.error(invalid_label_jump_target(label.span)); - } - } - ctx.error(invalid_label_target(label.span)); -} - fn invalid_break(span0: Span) -> OxcDiagnostic { OxcDiagnostic::error("Illegal break statement") .with_help("A `break` statement can only be used within an enclosing iteration or switch statement.") @@ -590,15 +565,29 @@ pub fn check_break_statement<'a>( node: &AstNode<'a>, ctx: &SemanticBuilder<'a>, ) { - if let Some(label) = &stmt.label { - return check_label(label, ctx, false); - } - // It is a Syntax Error if this BreakStatement is not nested, directly or indirectly (but not crossing function or static initialization block boundaries), within an IterationStatement or a SwitchStatement. for node_id in ctx.nodes.ancestors(node.id()).skip(1) { match ctx.nodes.kind(node_id) { - AstKind::Program(_) | AstKind::Function(_) | AstKind::StaticBlock(_) => { - ctx.error(invalid_break(stmt.span)); + AstKind::Program(_) => { + return stmt.label.as_ref().map_or_else( + || ctx.error(invalid_break(stmt.span)), + |label| ctx.error(invalid_label_target(label.span)), + ); + } + AstKind::Function(_) | AstKind::StaticBlock(_) => { + return stmt.label.as_ref().map_or_else( + || ctx.error(invalid_break(stmt.span)), + |label| ctx.error(invalid_label_jump_target(label.span)), + ); + } + AstKind::LabeledStatement(labeled_statement) => { + if stmt + .label + .as_ref() + .is_some_and(|label| label.name == labeled_statement.label.name) + { + break; + } } kind if (kind.is_iteration_statement() || matches!(kind, AstKind::SwitchStatement(_))) @@ -622,16 +611,42 @@ pub fn check_continue_statement<'a>( node: &AstNode<'a>, ctx: &SemanticBuilder<'a>, ) { - if let Some(label) = &stmt.label { - return check_label(label, ctx, true); - } - // It is a Syntax Error if this ContinueStatement is not nested, directly or indirectly (but not crossing function or static initialization block boundaries), within an IterationStatement. for node_id in ctx.nodes.ancestors(node.id()).skip(1) { match ctx.nodes.kind(node_id) { - AstKind::Program(_) | AstKind::Function(_) | AstKind::StaticBlock(_) => { - ctx.error(invalid_continue(stmt.span)); + AstKind::Program(_) => { + return stmt.label.as_ref().map_or_else( + || ctx.error(invalid_continue(stmt.span)), + |label| ctx.error(invalid_label_target(label.span)), + ); + } + AstKind::Function(_) | AstKind::StaticBlock(_) => { + return stmt.label.as_ref().map_or_else( + || ctx.error(invalid_continue(stmt.span)), + |label| ctx.error(invalid_label_jump_target(label.span)), + ); } + AstKind::LabeledStatement(labeled_statement) => match &stmt.label { + Some(label) if label.name == labeled_statement.label.name => { + if matches!( + labeled_statement.body, + Statement::LabeledStatement(_) + | Statement::DoWhileStatement(_) + | Statement::WhileStatement(_) + | Statement::ForStatement(_) + | Statement::ForInStatement(_) + | Statement::ForOfStatement(_) + ) { + break; + } + return ctx.error(invalid_label_non_iteration( + "continue", + labeled_statement.label.span, + label.span, + )); + } + _ => {} + }, kind if kind.is_iteration_statement() && stmt.label.is_none() => break, _ => {} } @@ -645,29 +660,26 @@ fn label_redeclaration(x0: &str, span1: Span, span2: Span) -> OxcDiagnostic { ]) } -#[allow(clippy::option_if_let_else)] -pub fn check_labeled_statement(ctx: &SemanticBuilder) { - ctx.label_builder.labels.iter().for_each(|labels| { - let mut defined = FxHashMap::default(); - //only need to care about the monotone increasing depth of the array - let mut increase_depth = vec![]; - for labeled in labels { - increase_depth.push(labeled); - // have to traverse because HashMap can only delete one by one - while increase_depth.len() > 2 - && increase_depth.iter().rev().nth(1).unwrap().depth - >= increase_depth.iter().next_back().unwrap().depth - { - defined.remove(increase_depth[increase_depth.len() - 2].name); - increase_depth.remove(increase_depth.len() - 2); - } - if let Some(span) = defined.get(labeled.name) { - ctx.error(label_redeclaration(labeled.name, *span, labeled.span)); - } else { - defined.insert(labeled.name, labeled.span); +pub fn check_labeled_statement<'a>( + stmt: &LabeledStatement, + node: &AstNode<'a>, + ctx: &SemanticBuilder<'a>, +) { + for node_id in ctx.nodes.ancestors(node.id()).skip(1) { + match ctx.nodes.kind(node_id) { + // label cannot cross boundary on function or static block + AstKind::Function(_) | AstKind::StaticBlock(_) | AstKind::Program(_) => break, + // check label name redeclaration + AstKind::LabeledStatement(label_stmt) if stmt.label.name == label_stmt.label.name => { + return ctx.error(label_redeclaration( + stmt.label.name.as_str(), + label_stmt.label.span, + stmt.label.span, + )); } + _ => {} } - }); + } } fn multiple_declaration_in_for_loop_head(x0: &str, span1: Span) -> OxcDiagnostic { diff --git a/crates/oxc_semantic/src/checker/mod.rs b/crates/oxc_semantic/src/checker/mod.rs index 9b8120b1919e02..faecc7b4e61bf8 100644 --- a/crates/oxc_semantic/src/checker/mod.rs +++ b/crates/oxc_semantic/src/checker/mod.rs @@ -16,7 +16,7 @@ pub fn check<'a>(node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) { match kind { AstKind::Program(_) => { - js::check_labeled_statement(ctx); + // js::check_labeled_statement(ctx); js::check_duplicate_class_elements(ctx); } AstKind::BindingIdentifier(ident) => { @@ -47,6 +47,7 @@ pub fn check<'a>(node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) { AstKind::BreakStatement(stmt) => js::check_break_statement(stmt, node, ctx), AstKind::ContinueStatement(stmt) => js::check_continue_statement(stmt, node, ctx), AstKind::LabeledStatement(stmt) => { + js::check_labeled_statement(stmt, node, ctx); js::check_function_declaration(&stmt.body, true, ctx); } AstKind::ForInStatement(stmt) => { diff --git a/crates/oxc_semantic/src/label.rs b/crates/oxc_semantic/src/label.rs index 822a2b6a80b9d4..fe346c94a26515 100644 --- a/crates/oxc_semantic/src/label.rs +++ b/crates/oxc_semantic/src/label.rs @@ -1,156 +1,37 @@ -use oxc_ast::ast::LabeledStatement; -use oxc_span::Span; -use rustc_hash::FxHashSet; - use crate::AstNodeId; #[derive(Debug)] -pub struct Label<'a> { - pub id: AstNodeId, +pub struct LabeledScope<'a> { pub name: &'a str, - pub span: Span, - used: bool, - /// depth is the number of nested labeled statements - pub depth: usize, - /// is accessible means that the label is accessible from the current position - is_accessible: bool, - /// is_inside_function_or_static_block means that the label is inside a function or static block - is_inside_function_or_static_block: bool, -} - -impl<'a> Label<'a> { - pub fn new( - id: AstNodeId, - name: &'a str, - span: Span, - depth: usize, - is_inside_function_or_static_block: bool, - ) -> Self { - Self { - id, - name, - span, - depth, - is_inside_function_or_static_block, - used: false, - is_accessible: true, - } - } + pub used: bool, + pub parent: usize, } -#[derive(Default)] -pub struct LabelBuilder<'a> { - pub labels: Vec>>, - depth: usize, - pub unused_node_ids: FxHashSet, +#[derive(Debug, Default)] +pub struct UnusedLabels<'a> { + pub scopes: Vec>, + pub curr_scope: usize, + pub labels: Vec, } -impl<'a> LabelBuilder<'a> { - pub fn enter(&mut self, stmt: &'a LabeledStatement<'a>, current_node_id: AstNodeId) { - let is_empty = self.labels.last().map_or(false, Vec::is_empty); - - if !self.is_inside_labeled_statement() { - self.labels.push(vec![]); - } - - self.depth += 1; - - self.labels.last_mut().unwrap_or_else(|| unreachable!()).push(Label::new( - current_node_id, - stmt.label.name.as_str(), - stmt.label.span, - self.depth, - is_empty, - )); - } - - pub fn leave(&mut self) { - let depth = self.depth; - - // Mark labels at the current depth as inaccessible - // ```ts - // label: {} // leave here, mark label as inaccessible - // break label // So we cannot find label here - // ``` - for label in self.get_accessible_labels_mut() { - if depth == label.depth { - label.is_accessible = false; - } - } - - // If depth is 0, move last labels to the front of `labels` and set `depth` to the length of the last labels. - // We need to do this because we're currently inside a function or static block - while self.depth == 0 { - if let Some(last_labels) = self.labels.pop() { - if !last_labels.is_empty() { - self.labels.insert(0, last_labels); - } - } - self.depth = self.labels.last().unwrap().len(); - } - - self.depth -= 1; - - // insert unused labels into `unused_node_ids` - if self.depth == 0 { - if let Some(labels) = self.labels.last() { - for label in labels { - if !label.used { - self.unused_node_ids.insert(label.id); - } - } - } - } - } - - pub fn enter_function_or_static_block(&mut self) { - if self.is_inside_labeled_statement() { - self.depth = 0; - self.labels.push(vec![]); - } +impl<'a> UnusedLabels<'a> { + pub fn add(&mut self, name: &'a str) { + self.scopes.push(LabeledScope { name, used: false, parent: self.curr_scope }); + self.curr_scope = self.scopes.len() - 1; } - pub fn leave_function_or_static_block(&mut self) { - if self.is_inside_labeled_statement() { - let labels = self.labels.pop().unwrap_or_else(|| unreachable!()); - if !labels.is_empty() { - self.labels.insert(0, labels); - } - self.depth = self.labels.last().map_or(0, Vec::len); + pub fn reference(&mut self, name: &'a str) { + let scope = self.scopes.iter_mut().rev().find(|x| x.name == name); + if let Some(scope) = scope { + scope.used = true; } } - pub fn is_inside_labeled_statement(&self) -> bool { - self.depth != 0 || self.labels.last().is_some_and(Vec::is_empty) - } - - pub fn is_inside_function_or_static_block(&self) -> bool { - self.labels - .last() - .is_some_and(|labels| labels.is_empty() || labels[0].is_inside_function_or_static_block) - } - - pub fn get_accessible_labels(&self) -> impl DoubleEndedIterator> { - return self.labels.last().unwrap().iter().filter(|label| label.is_accessible).rev(); - } - - pub fn get_accessible_labels_mut(&mut self) -> impl DoubleEndedIterator> { - return self - .labels - .last_mut() - .unwrap() - .iter_mut() - .filter(|label| label.is_accessible) - .rev(); - } - - pub fn mark_as_used(&mut self, label: &oxc_ast::ast::LabelIdentifier) { - if self.is_inside_labeled_statement() { - let label = self.get_accessible_labels_mut().find(|x| x.name == label.name); - - if let Some(label) = label { - label.used = true; - } + pub fn mark_unused(&mut self, current_node_id: AstNodeId) { + let scope = &self.scopes[self.curr_scope]; + if !scope.used { + self.labels.push(current_node_id); } + self.curr_scope = scope.parent; } } diff --git a/crates/oxc_semantic/src/lib.rs b/crates/oxc_semantic/src/lib.rs index 35a59e43fa09d9..4f66b5ccbb399d 100644 --- a/crates/oxc_semantic/src/lib.rs +++ b/crates/oxc_semantic/src/lib.rs @@ -36,7 +36,6 @@ pub use oxc_syntax::{ scope::{ScopeFlags, ScopeId}, symbol::{SymbolFlags, SymbolId}, }; -use rustc_hash::FxHashSet; pub use crate::{ reference::{Reference, ReferenceFlags, ReferenceId}, @@ -83,7 +82,7 @@ pub struct Semantic<'a> { /// Parsed JSDoc comments. jsdoc: JSDocFinder<'a>, - unused_labels: FxHashSet, + unused_labels: Vec, /// Control flow graph. Only present if [`Semantic`] is built with cfg /// creation enabled using [`SemanticBuilder::with_cfg`]. @@ -150,7 +149,7 @@ impl<'a> Semantic<'a> { &self.symbols } - pub fn unused_labels(&self) -> &FxHashSet { + pub fn unused_labels(&self) -> &Vec { &self.unused_labels } diff --git a/tasks/coverage/parser_test262.snap b/tasks/coverage/parser_test262.snap index 27d28db939ed2d..cd6440b6ada888 100644 --- a/tasks/coverage/parser_test262.snap +++ b/tasks/coverage/parser_test262.snap @@ -23861,7 +23861,7 @@ Negative Passed: 4220/4220 (100.00%) 24 │ var y=2; ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[test262/test/language/statements/break/S12.8_A5_T1.js:23:15] 22 │ return; 23 │ break LABEL_ANOTHER_LOOP; @@ -23869,7 +23869,7 @@ Negative Passed: 4220/4220 (100.00%) 24 │ LABEL_IN_2 : y++; ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[test262/test/language/statements/break/S12.8_A5_T2.js:25:15] 24 │ return; 25 │ break IN_DO_FUNC; @@ -23877,7 +23877,7 @@ Negative Passed: 4220/4220 (100.00%) 26 │ LABEL_IN_2 : y++; ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[test262/test/language/statements/break/S12.8_A5_T3.js:25:15] 24 │ return; 25 │ break LABEL_IN; @@ -30352,7 +30352,7 @@ Negative Passed: 4220/4220 (100.00%) 21 │ } ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[test262/test/language/statements/class/static-init-invalid-undefined-break-target.js:22:13] 21 │ x: while (false) { 22 │ break y; @@ -30360,7 +30360,7 @@ Negative Passed: 4220/4220 (100.00%) 23 │ } ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[test262/test/language/statements/class/static-init-invalid-undefined-continue-target.js:22:16] 21 │ x: while (false) { 22 │ continue y; diff --git a/tasks/coverage/parser_typescript.snap b/tasks/coverage/parser_typescript.snap index e98818396594db..e5635bb34e2b09 100644 --- a/tasks/coverage/parser_typescript.snap +++ b/tasks/coverage/parser_typescript.snap @@ -8118,15 +8118,6 @@ Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/salsa/typeFro ╰──── help: A `continue` statement can only be used within an enclosing `for`, `while` or `do while` - × Illegal continue statement: no surrounding iteration statement - ╭─[typescript/tests/cases/compiler/invalidContinueInDownlevelAsync.ts:3:9] - 2 │ if (true) { - 3 │ continue; - · ───────── - 4 │ } - ╰──── - help: A `continue` statement can only be used within an enclosing `for`, `while` or `do while` - × Expected `;` but found `[` ╭─[typescript/tests/cases/compiler/invalidLetInForOfAndForIn_ES5.ts:5:13] 4 │ var let = 10; @@ -21438,7 +21429,7 @@ Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/salsa/typeFro 28 │ return toFixed() ╰──── - × Use of undefined label + × Jump target cannot cross function boundary. ╭─[typescript/tests/cases/conformance/salsa/plainJSBinderErrors.ts:34:19] 33 │ label: var x = 1 34 │ break label