Skip to content

Commit

Permalink
refactor(semantic): rewrite handling of label statement errors
Browse files Browse the repository at this point in the history 
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)
  • Loading branch information
@Boshen
Boshen committed Aug 23, 2024
1 parent 7cc2bbd commit 3db9e94
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 235 deletions.
26 changes: 8 additions & 18 deletions crates/oxc_semantic/src/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -94,8 +94,7 @@ pub struct SemanticBuilder<'a> {

pub(crate) module_record: Arc<ModuleRecord>,

pub(crate) label_builder: LabelBuilder<'a>,

unused_labels: UnusedLabels<'a>,
build_jsdoc: bool,
jsdoc: JSDocBuilder<'a>,

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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() }
Expand Down Expand Up @@ -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(_) => {
Expand Down Expand Up @@ -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(_) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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),
_ => {}
}
}
Expand Down
128 changes: 70 additions & 58 deletions crates/oxc_semantic/src/checker/javascript.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand All @@ -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(_)))
Expand All @@ -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,
_ => {}
}
Expand All @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion crates/oxc_semantic/src/checker/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down
Loading

0 comments on commit 3db9e94

Please sign in to comment.