From 73ccf8a4da0420f64de2741552652ffe4b3253e2 Mon Sep 17 00:00:00 2001 From: Tzvi Melamed Date: Thu, 1 Feb 2024 07:46:38 -0500 Subject: [PATCH] fix(oxc_semantic): proper traversal of try statements (#2250) Closes #2227 --- .../src/snapshots/empty_brace_spaces.snap | 36 +++ crates/oxc_linter/src/snapshots/no_empty.snap | 32 ++ .../src/snapshots/no_return_await.snap | 9 + .../src/snapshots/no_unsafe_finally.snap | 130 ++++++++ crates/oxc_semantic/src/builder.rs | 282 ++++++++++-------- crates/oxc_semantic/src/pg.rs | 66 ---- .../tests/cfg_fixtures/do_while_break.js | 15 +- .../cfg__cfg_files@do_while_break.js-2.snap | 50 ++-- .../cfg__cfg_files@do_while_break.js.snap | 18 +- ...g__cfg_files@function_in_finally.js-2.snap | 21 +- ...cfg__cfg_files@function_in_finally.js.snap | 8 +- ...g__cfg_files@labeled_block_break.js-2.snap | 12 +- ...fg__cfg_files@labelled_try_break.js-2.snap | 46 ++- .../cfg__cfg_files@labelled_try_break.js.snap | 20 +- tasks/coverage/parser_test262.snap | 32 ++ 15 files changed, 472 insertions(+), 305 deletions(-) diff --git a/crates/oxc_linter/src/snapshots/empty_brace_spaces.snap b/crates/oxc_linter/src/snapshots/empty_brace_spaces.snap index be3dce8ed..c68cf3fba 100644 --- a/crates/oxc_linter/src/snapshots/empty_brace_spaces.snap +++ b/crates/oxc_linter/src/snapshots/empty_brace_spaces.snap @@ -94,6 +94,13 @@ expression: empty_brace_spaces ╰──── help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed + ╭─[empty_brace_spaces.tsx:1:29] + 1 │ try {} catch(foo){} finally { } + · ─── + ╰──── + help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed ╭─[empty_brace_spaces.tsx:1:4] 1 │ do { } while (foo) @@ -262,6 +269,13 @@ expression: empty_brace_spaces ╰──── help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed + ╭─[empty_brace_spaces.tsx:1:29] + 1 │ try {} catch(foo){} finally { } + · ───── + ╰──── + help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed ╭─[empty_brace_spaces.tsx:1:4] 1 │ do { } while (foo) @@ -437,6 +451,13 @@ expression: empty_brace_spaces ╰──── help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed + ╭─[empty_brace_spaces.tsx:1:29] + 1 │ try {} catch(foo){} finally { } + · ────────── + ╰──── + help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed ╭─[empty_brace_spaces.tsx:1:4] 1 │ do { } while (foo) @@ -618,6 +639,14 @@ expression: empty_brace_spaces ╰──── help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed + ╭─[empty_brace_spaces.tsx:1:29] + 1 │ ╭─▶ try {} catch(foo){} finally { + 2 │ │ + 3 │ ╰─▶ } + ╰──── + help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed ╭─[empty_brace_spaces.tsx:1:4] 1 │ ╭─▶ do { @@ -797,6 +826,13 @@ expression: empty_brace_spaces ╰──── help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed + ╭─[empty_brace_spaces.tsx:1:29] + 1 │ ╭─▶ try {} catch(foo){} finally { + 2 │ ╰─▶ } + ╰──── + help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code. + ⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed ╭─[empty_brace_spaces.tsx:1:4] 1 │ ╭─▶ do { diff --git a/crates/oxc_linter/src/snapshots/no_empty.snap b/crates/oxc_linter/src/snapshots/no_empty.snap index 69c3b66c3..bf82411c9 100644 --- a/crates/oxc_linter/src/snapshots/no_empty.snap +++ b/crates/oxc_linter/src/snapshots/no_empty.snap @@ -19,6 +19,14 @@ expression: no_empty ╰──── help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements + ╭─[no_empty.tsx:1:45] + 1 │ try { foo() } catch (ex) {throw ex} finally {} + · ─┬ + · ╰── Empty block statement + ╰──── + help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements ╭─[no_empty.tsx:1:26] 1 │ try { foo() } catch (ex) {} @@ -83,6 +91,14 @@ expression: no_empty ╰──── help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements + ╭─[no_empty.tsx:1:38] + 1 │ try { foo(); } catch (ex) {} finally {} + · ─┬ + · ╰── Empty block statement + ╰──── + help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements ╭─[no_empty.tsx:1:5] 1 │ try {} catch (ex) {} finally {} @@ -99,6 +115,14 @@ expression: no_empty ╰──── help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements + ╭─[no_empty.tsx:1:30] + 1 │ try {} catch (ex) {} finally {} + · ─┬ + · ╰── Empty block statement + ╰──── + help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements ╭─[no_empty.tsx:1:27] 1 │ try { foo(); } catch (ex) {} finally {} @@ -115,3 +139,11 @@ expression: no_empty ╰──── help: Add comment inside empty block statement + ⚠ eslint(no-empty): Disallow empty block statements + ╭─[no_empty.tsx:1:38] + 1 │ try { foo(); } catch (ex) {} finally {} + · ─┬ + · ╰── Empty block statement + ╰──── + help: Add comment inside empty block statement + diff --git a/crates/oxc_linter/src/snapshots/no_return_await.snap b/crates/oxc_linter/src/snapshots/no_return_await.snap index 7836fcbf9..b39d3204f 100644 --- a/crates/oxc_linter/src/snapshots/no_return_await.snap +++ b/crates/oxc_linter/src/snapshots/no_return_await.snap @@ -230,6 +230,15 @@ expression: no_return_await ╰──── help: Remove redundant `await`. + ⚠ eslint(no-return-await): Redundant use of `await` on a return value. + ╭─[no_return_await.tsx:4:8] + 3 │ finally { + 4 │ return await bar(); + · ───── + 5 │ } + ╰──── + help: Remove redundant `await`. + ⚠ eslint(no-return-await): Redundant use of `await` on a return value. ╭─[no_return_await.tsx:5:8] 4 │ catch (e) { diff --git a/crates/oxc_linter/src/snapshots/no_unsafe_finally.snap b/crates/oxc_linter/src/snapshots/no_unsafe_finally.snap index 5bc214786..48b542440 100644 --- a/crates/oxc_linter/src/snapshots/no_unsafe_finally.snap +++ b/crates/oxc_linter/src/snapshots/no_unsafe_finally.snap @@ -12,6 +12,29 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:7:2] + 6 │ } finally { + 7 │ return 3; + · ───────── + 8 │ } + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:86] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { if(true) { return 3 } else { return 2 } } } + · ──────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:104] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { if(true) { return 3 } else { return 2 } } } + · ──────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:86] 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { if(true) { return 3 } else { return 2 } } } @@ -33,6 +56,13 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:75] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { return 3 } } + · ──────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:75] 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { return function(x) { return y } } } @@ -40,6 +70,20 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:75] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { return function(x) { return y } } } + · ─────────────────────────────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:75] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { return { x: function(c) { return c } } } } + · ────────────────────────────────────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:75] 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { return { x: function(c) { return c } } } } @@ -54,6 +98,34 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:75] + 1 │ var foo = function() { try { return 1 } catch(err) { return 2 } finally { throw new Error() } } + · ───────────────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:74] + 1 │ var foo = function() { try { foo(); } finally { try { bar(); } finally { return; } } }; + · ─────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:74] + 1 │ var foo = function() { try { foo(); } finally { try { bar(); } finally { return; } } }; + · ─────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:74] + 1 │ var foo = function() { try { foo(); } finally { try { bar(); } finally { return; } } }; + · ─────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:74] 1 │ var foo = function() { try { foo(); } finally { try { bar(); } finally { return; } } }; @@ -68,6 +140,22 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:59] + 1 │ var foo = function() { label: try { return 0; } finally { break label; } return 1; } + · ──────────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:7:2] + 6 │ } finally { + 7 │ break a; + · ──────── + 8 │ } + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:7:2] 6 │ } finally { @@ -84,6 +172,13 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:54] + 1 │ var foo = function() { while (true) try {} finally { break; } } + · ────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:54] 1 │ var foo = function() { while (true) try {} finally { continue; } } @@ -91,6 +186,20 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:54] + 1 │ var foo = function() { while (true) try {} finally { continue; } } + · ───────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:68] + 1 │ var foo = function() { switch (true) { case true: try {} finally { break; } } } + · ────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:68] 1 │ var foo = function() { switch (true) { case true: try {} finally { break; } } } @@ -105,6 +214,20 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:84] + 1 │ var foo = function() { a: while (true) try {} finally { switch (true) { case true: break a; } } } + · ──────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:84] + 1 │ var foo = function() { a: while (true) try {} finally { switch (true) { case true: continue; } } } + · ───────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block ╭─[no_unsafe_finally.tsx:1:84] 1 │ var foo = function() { a: while (true) try {} finally { switch (true) { case true: continue; } } } @@ -119,3 +242,10 @@ expression: no_unsafe_finally ╰──── help: Control flow inside try or catch blocks will be overwritten by this statement + ⚠ eslint(no-unsafe-finally): Unsafe finally block + ╭─[no_unsafe_finally.tsx:1:98] + 1 │ var foo = function() { a: switch (true) { case true: try {} finally { switch (true) { case true: break a; } } } } + · ──────── + ╰──── + help: Control flow inside try or catch blocks will be overwritten by this statement + diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index bb218423a..b8fcc8149 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -7,11 +7,7 @@ use itertools::Itertools; use oxc_ast::{ast::*, AstKind, Trivias, TriviasMap, Visit}; use oxc_diagnostics::Error; use oxc_span::{Atom, SourceType, Span}; -use oxc_syntax::{ - module_record::ModuleRecord, - node::{AstNodeId, NodeFlags}, - operator::AssignmentOperator, -}; +use oxc_syntax::{module_record::ModuleRecord, operator::AssignmentOperator}; use rustc_hash::FxHashMap; use crate::{ @@ -25,8 +21,7 @@ use crate::{ jsdoc::JSDocBuilder, label::LabelBuilder, module_record::ModuleRecordBuilder, - node::{AstNode, AstNodes}, - pg::replicate_tree_to_leaves, + node::{AstNode, AstNodeId, AstNodes, NodeFlags}, reference::{Reference, ReferenceFlag, ReferenceId}, scope::{ScopeFlags, ScopeId, ScopeTree}, symbol::{SymbolFlags, SymbolId, SymbolTable}, @@ -1158,167 +1153,194 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> { let kind = AstKind::TryStatement(self.alloc(stmt)); self.enter_node(kind); + // There are 3 possible kinds of Try Statements (See + // ): + // 1. try-catch + // 2. try-finally + // 3. try-catch-finally + // + // We will consider each kind of try statement separately. + // + // For a try-catch, there are only 2 ways to reach + // the outgoing node (after the entire statement): + // + // 1. after the try block completing successfully + // 2. after the catch block completing successfully, + // in which case some statement in the try block + // must have thrown. + // + // For a try-finally, there is only 1 way to reach + // the outgoing node, whereby: + // - the try block completed successfully, and + // - the finally block completed successfully + // + // But the finally block can also be reached when the try + // fails. We thus need to fork the control flow graph into + // 2 different finally statements: + // 1. one where the try block completes successfully, (finally_succ) + // 2. one where some statement in the try block throws (finally_err) + // Only the end of the try block will have an incoming edge to the + // finally_succ, and only finally_succ will have an outgoing node to + // the next statement. + // + // For a try-catch-finally, we have seemlingly more cases: + // 1. after the try block completing successfully + // 2. after the catch block completing successfully + // 3. after the try block if the catch block throws + // Despite having 3 distings scenarios, we can simplify the control flow + // graph by still only using a finally_succ and a finally_err node. + // The key is that the outgoing edge going past the entire + // try-catch-finally statement is guaranteed that all code paths have + // either completed the try block or the catch block in full. + + // Implementation notes: + // We will use the following terminology: + // + // the "parent after_throw block" is the block that would be the target + // of a throw if there were no try-catch-finally. + // + // Within the try block, a throw will not go to the parent after_throw + // block. Instead, it will go to the catch block in a try-catch or to + // the finally_err block in a try-catch-finally. + // + // In a catch block, a throw will go to the finally_err block in a + // try-catch-finally, or to the parent after_throw block in a basic + // try-catch. + // + // In a finally block, a throw will always go to the parent after_throw + // block, both for finally_succ and finally_err. + /* cfg */ let statement_state = self .cfg .before_statement(self.current_node_id, StatementControlFlowType::DoesNotUseContinue); - let before_try_stmt_graph_ix = self.cfg.current_node_ix; - let catch_block_graph_ix = self.cfg.new_basic_block(); - // every statement created with this active adds an edge from that node to this - // catch block node + // TODO: support unwinding finally/catch blocks that aren't in this function + // even if something throws. + let parent_after_throw_block_ix = self.cfg.after_throw_block; + + let try_stmt_pre_start_ix = self.cfg.current_node_ix; + + let try_stmt_start_ix = self.cfg.new_basic_block(); + self.cfg.add_edge(try_stmt_pre_start_ix, try_stmt_start_ix, EdgeType::Normal); + let try_after_throw_block_ix = self.cfg.new_basic_block(); + + self.cfg.current_node_ix = try_stmt_start_ix; + + // every statement created with this active adds an edge from that node to this node // // NOTE: we oversimplify here, realistically even in between basic blocks we // do throwsy things which could cause problems, but for the most part simply // pointing the end of every basic block to the catch block is enough - self.cfg.after_throw_block = Some(catch_block_graph_ix); - let start_of_try_block_graph_ix = self.cfg.new_basic_block(); + self.cfg.after_throw_block = Some(try_after_throw_block_ix); + // The one case that needs to be handled specially is if the first statement in the + // try block throws. In that case, it is not sufficient to rely on an edge after that + // statement, because the catch will run before that edge is taken. + self.cfg.add_edge(try_stmt_pre_start_ix, try_after_throw_block_ix, EdgeType::Normal); /* cfg */ self.visit_block_statement(&stmt.block); /* cfg */ - self.cfg.after_throw_block = None; // only active during try block + let end_of_try_block_ix = self.cfg.current_node_ix; + self.cfg.add_edge(end_of_try_block_ix, try_after_throw_block_ix, EdgeType::Normal); + self.cfg.after_throw_block = parent_after_throw_block_ix; - let end_of_try_block_graph_ix = self.cfg.current_node_ix; + let start_of_finally_err_block_ix = if stmt.finalizer.is_some() { + if stmt.handler.is_some() { + // try-catch-finally + Some(self.cfg.new_basic_block()) + } else { + // try-finally + Some(try_after_throw_block_ix) + } + } else { + // try-catch + None + }; /* cfg */ - let optional_catch_block_graph_ix = if let Some(handler) = &stmt.handler { + let catch_block_end_ix = if let Some(handler) = &stmt.handler { /* cfg */ - self.cfg.current_node_ix = catch_block_graph_ix; + let catch_after_throw_block_ix = if stmt.finalizer.is_some() { + start_of_finally_err_block_ix + } else { + parent_after_throw_block_ix + }; + self.cfg.after_throw_block = catch_after_throw_block_ix; + + let catch_block_start_ix = try_after_throw_block_ix; + self.cfg.current_node_ix = catch_block_start_ix; + + if let Some(catch_after_throw_block_ix) = catch_after_throw_block_ix { + self.cfg.add_edge( + catch_block_start_ix, + catch_after_throw_block_ix, + EdgeType::Normal, + ); + } /* cfg */ self.visit_catch_clause(handler); - Some((catch_block_graph_ix, self.cfg.current_node_ix)) + /* cfg */ + Some(self.cfg.current_node_ix) + /* cfg */ } else { - /* cfg */ - let preserved_node_ix = self.cfg.current_node_ix; - // each statement that connects the would be catch block should just point - // to an unreachable() as if any statement throws this function would end early - self.cfg.current_node_ix = catch_block_graph_ix; - self.cfg.put_unreachable(); - self.cfg.current_node_ix = preserved_node_ix; - /* cfg */ - None }; - let finally = if let Some(finalizer) = &stmt.finalizer { + + // Restore the after_throw_block + self.cfg.after_throw_block = parent_after_throw_block_ix; + + if let Some(finalizer) = &stmt.finalizer { /* cfg */ - let start_of_finally_block_graph_ix = self.cfg.new_basic_block(); + let finally_err_block_start_ix = + start_of_finally_err_block_ix.expect("this try statement has a finally_err block"); + + self.cfg.current_node_ix = finally_err_block_start_ix; /* cfg */ self.visit_finally_clause(finalizer); /* cfg */ - Some((start_of_finally_block_graph_ix, self.cfg.current_node_ix)) + // put an unreachable after the finally_err block + self.cfg.put_unreachable(); + + let finally_succ_block_start_ix = self.cfg.new_basic_block(); + + // The end_of_try_block has an outgoing edge to finally_succ also + // for when the try block completes successfully. + self.cfg.add_edge(end_of_try_block_ix, finally_succ_block_start_ix, EdgeType::Normal); + + // The end_of_catch_block has an outgoing edge to finally_succ for + // when the catch block in a try-catch-finally completes successfully. + if let Some(end_of_catch_block_ix) = catch_block_end_ix { + // try-catch-finally + self.cfg.add_edge( + end_of_catch_block_ix, + finally_succ_block_start_ix, + EdgeType::Normal, + ); + } /* cfg */ - } else { - None - }; + + self.visit_finally_clause(finalizer); + } /* cfg */ - let after_try_stmt_graph_ix = self.cfg.new_basic_block(); - - self.cfg.add_edge(before_try_stmt_graph_ix, start_of_try_block_graph_ix, EdgeType::Normal); - match (finally, optional_catch_block_graph_ix) { - ( - Some((nothrow_finally_start, nothrow_finally_end)), - Some((catch_start, catch_end)), - ) => { - // - // | - // (1)/ \(2) - // | - // | | - // - // | - // - // | - // - // | - // todo: should continue to upper function's catch - let duplicated = replicate_tree_to_leaves(nothrow_finally_start, &mut self.cfg); - let throw_finally_start = duplicated[¬hrow_finally_start]; - - // 1 - self.cfg.add_edge(end_of_try_block_graph_ix, catch_start, EdgeType::Normal); - self.cfg.add_edge(catch_end, throw_finally_start, EdgeType::Normal); - // 2 - self.cfg.add_edge( - end_of_try_block_graph_ix, - nothrow_finally_start, - EdgeType::Normal, - ); - self.cfg.add_edge(nothrow_finally_end, after_try_stmt_graph_ix, EdgeType::Normal); - - // throw can happen before a single statement finishes in try block - essentially skipping try block - self.cfg.add_edge(before_try_stmt_graph_ix, catch_start, EdgeType::Normal); - } - (Some((nothrow_finally_start, nothrow_finally_end)), None) => { - // - // | - // (1)/ \(2) - // | - // | | - // - // | - // - // | - // - // | - // todo: should continue to upper function's catch - - let duplicated = replicate_tree_to_leaves(nothrow_finally_start, &mut self.cfg); - let throw_finally_start = duplicated[¬hrow_finally_start]; - // 1 - self.cfg.add_edge( - end_of_try_block_graph_ix, - nothrow_finally_start, - EdgeType::Normal, - ); - self.cfg.add_edge(nothrow_finally_end, after_try_stmt_graph_ix, EdgeType::Normal); - - // 2 - - // even though we don't explicitly have a catch block, we still make a catch - // block in the cfg which immediately points to the throw catch block. - self.cfg.add_edge( - end_of_try_block_graph_ix, - catch_block_graph_ix, - EdgeType::Normal, - ); - self.cfg.add_edge(catch_block_graph_ix, throw_finally_start, EdgeType::Normal); - // throw can happen before a single statement finishes in try block - essentially skipping try block - self.cfg.add_edge(before_try_stmt_graph_ix, throw_finally_start, EdgeType::Normal); - } - (None, Some((catch_start, catch_end))) => { - self.cfg.add_edge(end_of_try_block_graph_ix, catch_start, EdgeType::Normal); - self.cfg.add_edge(catch_end, after_try_stmt_graph_ix, EdgeType::Normal); - // if nothing throws - self.cfg.add_edge( - end_of_try_block_graph_ix, - after_try_stmt_graph_ix, - EdgeType::Normal, - ); - // throw can happen before a single statement finishes in try block - essentially skipping try block - self.cfg.add_edge(before_try_stmt_graph_ix, catch_start, EdgeType::Normal); - } - (None, None) => { - // todo: support unwinding finally/catch blocks that aren't in this function - // even if something throws - self.cfg.add_edge( - end_of_try_block_graph_ix, - after_try_stmt_graph_ix, - EdgeType::Normal, - ); - } - } + let try_statement_block_end_ix = self.cfg.current_node_ix; + let after_try_statement_block_ix = self.cfg.new_basic_block(); + self.cfg.add_edge( + try_statement_block_end_ix, + after_try_statement_block_ix, + EdgeType::Normal, + ); self.cfg.after_statement( &statement_state, self.current_node_id, - after_try_stmt_graph_ix, + self.cfg.current_node_ix, None, ); /* cfg */ diff --git a/crates/oxc_semantic/src/pg.rs b/crates/oxc_semantic/src/pg.rs index 1fb9e3b3a..e8aa74c38 100644 --- a/crates/oxc_semantic/src/pg.rs +++ b/crates/oxc_semantic/src/pg.rs @@ -1,9 +1,5 @@ -use std::collections::{HashMap, HashSet}; - use petgraph::{stable_graph::NodeIndex, visit::EdgeRef, Direction, Graph}; -use crate::control_flow::ControlFlowGraph; - /// # Panics pub fn neighbors_filtered_by_edge_weight< State: Default + Copy + Clone, @@ -58,65 +54,3 @@ where final_states } - -/// # Panics -pub fn replicate_tree_to_leaves( - start_at: NodeIndex, - cfg: &mut ControlFlowGraph, -) -> HashMap { - fn duplicate_graph_node(basic_block_id: NodeIndex, cfg: &mut ControlFlowGraph) -> NodeIndex { - let new_basic_block_graph_ix = cfg.new_basic_block(); - - // todo: what's a better way to do this? - let items = cfg.basic_block_by_index(basic_block_id).clone(); - for item in items { - cfg.current_basic_block().push(item); - } - - // todo: should we add the functions copied here to the function_to_node_ix? - - new_basic_block_graph_ix - } - - let preserved_graph_ix = cfg.current_node_ix; - - let mut old_to_new: HashMap = HashMap::default(); - let mut q = vec![]; - - let new_graph_start_ix = duplicate_graph_node(start_at, cfg); - old_to_new.insert(start_at, new_graph_start_ix); - q.push((new_graph_start_ix, start_at)); - let mut edges_finished_already = HashSet::new(); - - while let Some(node) = q.pop() { - // todo: are there any incoming edges we won't cover? - let edges = cfg - .graph - .edges_directed(node.1, Direction::Outgoing) - .map(|x| (x.target(), x.weight().to_owned(), x.id())) - .collect::>(); - - for (to, weight, id) in edges { - if edges_finished_already.contains(&id) { - continue; - } - - edges_finished_already.insert(id); - - let new_graph_ix = if old_to_new.contains_key(&to) { - old_to_new[&to] - } else { - let new_graph_ix = duplicate_graph_node(to, cfg); - old_to_new.insert(node.1, new_graph_start_ix); - new_graph_ix - }; - - cfg.add_edge(node.0, new_graph_ix, weight); - - q.push((new_graph_ix, to)); - } - } - - cfg.current_node_ix = preserved_graph_ix; - old_to_new -} diff --git a/crates/oxc_semantic/tests/cfg_fixtures/do_while_break.js b/crates/oxc_semantic/tests/cfg_fixtures/do_while_break.js index e55ca1e20..78a1d033e 100644 --- a/crates/oxc_semantic/tests/cfg_fixtures/do_while_break.js +++ b/crates/oxc_semantic/tests/cfg_fixtures/do_while_break.js @@ -1,9 +1,8 @@ var foo = function () { - try { - - } finally { - do { - break; - } while (true) - } -} + try { + } finally { + do { + break; + } while (true); + } +}; diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js-2.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js-2.snap index 5f56c3b5e..735052fb7 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js-2.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js-2.snap @@ -8,41 +8,37 @@ digraph { 1 [ label = ""] 2 [ label = ""] 3 [ label = ""] - 4 [ label = "Unreachable()"] - 5 [ label = ""] + 4 [ label = ""] + 5 [ label = "Unreachable()"] 6 [ label = ""] - 7 [ label = "Unreachable()"] - 8 [ label = ""] + 7 [ label = ""] + 8 [ label = "Unreachable()"] 9 [ label = ""] 10 [ label = ""] - 11 [ label = ""] + 11 [ label = "Unreachable()"] 12 [ label = ""] 13 [ label = ""] 14 [ label = ""] - 15 [ label = "Unreachable()"] - 16 [ label = "Unreachable()"] - 17 [ label = ""] + 15 [ label = ""] 0 -> 1 [ ] - 3 -> 2 [ ] - 2 -> 4 [ ] - 6 -> 7 [ ] - 6 -> 7 [ ] - 5 -> 8 [ ] - 6 -> 8 [ ] - 8 -> 9 [ ] - 8 -> 6 [ ] + 1 -> 2 [ ] 1 -> 3 [ ] - 11 -> 12 [ ] + 2 -> 3 [ ] + 4 -> 5 [ ] + 4 -> 5 [ ] + 3 -> 6 [ ] + 4 -> 6 [ ] + 6 -> 7 [ ] + 6 -> 4 [ ] + 7 -> 8 [ ] + 2 -> 9 [ ] + 10 -> 11 [ ] + 10 -> 11 [ ] + 9 -> 12 [ ] + 10 -> 12 [ ] 12 -> 13 [ ] - 12 -> 14 [ ] - 13 -> 11 [ ] - 13 -> 15 [ ] - 13 -> 16 [ ] - 3 -> 5 [ ] - 9 -> 10 [ ] - 3 -> 2 [ ] - 2 -> 11 [ ] - 1 -> 11 [ ] - 0 -> 17 [ ] + 12 -> 10 [ ] + 13 -> 14 [ ] + 0 -> 15 [ ] } diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js.snap index 0863defc2..d35d3d8e3 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@do_while_break.js.snap @@ -20,11 +20,11 @@ bb3: { } bb4: { - Unreachable() + } bb5: { - + Unreachable() } bb6: { @@ -32,11 +32,11 @@ bb6: { } bb7: { - Unreachable() + } bb8: { - + Unreachable() } bb9: { @@ -48,7 +48,7 @@ bb10: { } bb11: { - + Unreachable() } bb12: { @@ -64,13 +64,5 @@ bb14: { } bb15: { - Unreachable() -} - -bb16: { - Unreachable() -} - -bb17: { } diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js-2.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js-2.snap index 474dcbace..0da69e934 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js-2.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js-2.snap @@ -7,25 +7,22 @@ digraph { 0 [ label = ""] 1 [ label = ""] 2 [ label = ""] - 3 [ label = "Unreachable()"] + 3 [ label = ""] 4 [ label = ""] - 5 [ label = ""] + 5 [ label = "Unreachable()"] 6 [ label = ""] 7 [ label = ""] 8 [ label = ""] 9 [ label = ""] - 10 [ label = ""] - 2 -> 1 [ ] - 1 -> 3 [ ] - 4 -> 5 [ ] - 4 -> 6 [ ] + 0 -> 1 [ ] 0 -> 2 [ ] - 8 -> 9 [ ] - 8 -> 10 [ ] + 1 -> 2 [ ] + 2 -> 3 [ ] 2 -> 4 [ ] + 4 -> 5 [ ] + 1 -> 6 [ ] 6 -> 7 [ ] - 2 -> 1 [ ] - 1 -> 8 [ ] - 0 -> 8 [ ] + 6 -> 8 [ ] + 8 -> 9 [ ] } diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js.snap index fc79886ee..2f5feb49b 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@function_in_finally.js.snap @@ -16,7 +16,7 @@ bb2: { } bb3: { - Unreachable() + } bb4: { @@ -24,7 +24,7 @@ bb4: { } bb5: { - + Unreachable() } bb6: { @@ -42,7 +42,3 @@ bb8: { bb9: { } - -bb10: { - -} diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labeled_block_break.js-2.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labeled_block_break.js-2.snap index 674a154a9..0bef73f3d 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labeled_block_break.js-2.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labeled_block_break.js-2.snap @@ -11,16 +11,14 @@ digraph { 4 [ label = "Unreachable()"] 5 [ label = ""] 6 [ label = ""] - 2 -> 1 [ ] + 0 -> 1 [ ] + 0 -> 2 [ ] + 1 -> 2 [ ] 3 -> 4 [ ] 4 -> 5 [ ] - 1 -> 3 [ ] - 1 -> 5 [ ] + 2 -> 3 [ ] + 2 -> 5 [ ] 3 -> 5 [ ] - 0 -> 2 [ ] - 2 -> 1 [ ] 5 -> 6 [ ] - 2 -> 6 [ ] - 0 -> 1 [ ] } diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js-2.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js-2.snap index 21e5c3e07..362cd9238 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js-2.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js-2.snap @@ -6,35 +6,33 @@ input_file: crates/oxc_semantic/tests/cfg_fixtures/labelled_try_break.js digraph { 0 [ label = ""] 1 [ label = ""] - 2 [ label = ""] - 3 [ label = "$return = "] + 2 [ label = "$return = "] + 3 [ label = ""] 4 [ label = ""] 5 [ label = "Unreachable()"] 6 [ label = "Unreachable()"] - 7 [ label = ""] - 8 [ label = "Unreachable()"] - 9 [ label = "$return = "] - 10 [ label = ""] - 11 [ label = "Unreachable()"] - 12 [ label = ""] - 13 [ label = "Unreachable()"] - 14 [ label = ""] + 7 [ label = "Unreachable()"] + 8 [ label = ""] + 9 [ label = "Unreachable()"] + 10 [ label = "$return = "] + 11 [ label = ""] + 12 [ label = "Unreachable()"] + 13 [ label = ""] 0 -> 1 [ ] - 3 -> 2 [ ] - 4 -> 2 [ ] - 5 -> 2 [ ] - 4 -> 5 [ ] - 2 -> 6 [ ] - 7 -> 8 [ ] + 1 -> 2 [ ] 1 -> 3 [ ] - 10 -> 11 [ ] - 5 -> 7 [ ] + 4 -> 3 [ ] + 5 -> 3 [ ] + 4 -> 5 [ ] + 5 -> 3 [ ] + 3 -> 6 [ ] + 6 -> 7 [ ] + 5 -> 8 [ ] 8 -> 9 [ ] - 5 -> 2 [ ] - 2 -> 10 [ ] - 1 -> 10 [ ] - 7 -> 9 [ ] - 12 -> 13 [ ] - 0 -> 14 [ ] + 9 -> 10 [ ] + 3 -> 10 [ ] + 8 -> 10 [ ] + 11 -> 12 [ ] + 0 -> 13 [ ] } diff --git a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js.snap b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js.snap index 3b345cb5d..ee495a6d0 100644 --- a/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js.snap +++ b/crates/oxc_semantic/tests/snapshots/cfg__cfg_files@labelled_try_break.js.snap @@ -12,11 +12,11 @@ bb1: { } bb2: { - + $return = } bb3: { - $return = + } bb4: { @@ -32,33 +32,29 @@ bb6: { } bb7: { - + Unreachable() } bb8: { - Unreachable() + } bb9: { - $return = + Unreachable() } bb10: { - + $return = } bb11: { - Unreachable() + } bb12: { - -} - -bb13: { Unreachable() } -bb14: { +bb13: { } diff --git a/tasks/coverage/parser_test262.snap b/tasks/coverage/parser_test262.snap index df877bd7f..fc91ecbc1 100644 --- a/tasks/coverage/parser_test262.snap +++ b/tasks/coverage/parser_test262.snap @@ -20094,6 +20094,14 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" × 'export statement' declaration can only be used at the top level of a module ╭─[language/module-code/parse-err-decl-pos-export-try-catch-finally.js:14:1] 14 │ try { } catch (err) { } finally { + 15 │ export default null; + · ────── + 16 │ } + ╰──── + + × 'export statement' declaration can only be used at the top level of a module + ╭─[language/module-code/parse-err-decl-pos-export-try-catch-finally.js:14:1] + 14 │ try { } catch (err) { } finally { 15 │ export default null; · ────── 16 │ } @@ -20110,6 +20118,14 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" × 'export statement' declaration can only be used at the top level of a module ╭─[language/module-code/parse-err-decl-pos-export-try-finally.js:14:1] 14 │ try { } finally { + 15 │ export default null; + · ────── + 16 │ } + ╰──── + + × 'export statement' declaration can only be used at the top level of a module + ╭─[language/module-code/parse-err-decl-pos-export-try-finally.js:14:1] + 14 │ try { } finally { 15 │ export default null; · ────── 16 │ } @@ -20399,6 +20415,14 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" × 'import statement' declaration can only be used at the top level of a module ╭─[language/module-code/parse-err-decl-pos-import-try-catch-finally.js:14:1] 14 │ try { } catch (err) { } finally { + 15 │ import v from './decl-pos-import-try-catch-finally.js'; + · ────── + 16 │ } + ╰──── + + × 'import statement' declaration can only be used at the top level of a module + ╭─[language/module-code/parse-err-decl-pos-import-try-catch-finally.js:14:1] + 14 │ try { } catch (err) { } finally { 15 │ import v from './decl-pos-import-try-catch-finally.js'; · ────── 16 │ } @@ -20415,6 +20439,14 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" × 'import statement' declaration can only be used at the top level of a module ╭─[language/module-code/parse-err-decl-pos-import-try-finally.js:14:1] 14 │ try { } finally { + 15 │ import v from './decl-pos-import-try-finally.js'; + · ────── + 16 │ } + ╰──── + + × 'import statement' declaration can only be used at the top level of a module + ╭─[language/module-code/parse-err-decl-pos-import-try-finally.js:14:1] + 14 │ try { } finally { 15 │ import v from './decl-pos-import-try-finally.js'; · ────── 16 │ }