improvement(semantic/cfg): rework error path. (#3519)

This PR aims to provide a more accurate error/finalization flow, I've nuked the old error path approach and rewrote it with more versatility in mind.

We used to visit the finalizer block twice and create 2 sets of AstNodes/Basic Blocks for them, This was there to differentiate between the error path finalizer and success path one. There is no longer a need for having 2 separate sets of nodes to do this differentiation.

As for the error path now we have 2 kinds of them, Everything is attached to an error block - even if it is not in a try-catch statement - this results in a lot of extra edges to keep track of these "Implicit" error blocks but I believe in future it can help us to track cross block error paths, For example, we can dynamically attach and cache the implicit error block of a function to its call site error path (either implicit or explicit).

crates/oxc_ast/src/ast_kind.rs

@@ -588,7 +588,7 @@ impl<'a> AstKind<'a> {
             Self::PrivateIdentifier(x) => format!("PrivateIdentifier({})", x.name).into(),
 
             Self::NumericLiteral(n) => format!("NumericLiteral({})", n.value).into(),
-            Self::StringLiteral(s) => format!("NumericLiteral({})", s.value).into(),
+            Self::StringLiteral(s) => format!("StringLiteral({})", s.value).into(),
             Self::BooleanLiteral(b) => format!("BooleanLiteral({})", b.value).into(),
             Self::NullLiteral(_) => "NullLiteral".into(),
             Self::BigintLiteral(b) => format!("BigintLiteral({})", b.raw).into(),

crates/oxc_linter/src/rules/eslint/getter_return.rs

@@ -193,6 +193,11 @@ impl GetterReturn {
                 | EdgeType::NewFunction
                 // Unreachable nodes aren't reachable so we don't follow them.
                 | EdgeType::Unreachable
+                // TODO: For now we ignore the error path to simplify this rule, We can also
+                // analyze the error path as a nice to have addition.
+                | EdgeType::Error(_)
+                | EdgeType::Finalize
+                | EdgeType::Join
                 // By returning Some(X),
                 // we signal that we don't walk to this path any farther.
                 //

crates/oxc_linter/src/rules/eslint/no_this_before_super.rs

@@ -7,7 +7,10 @@ use oxc_ast::{
     AstKind,
 };
 use oxc_macros::declare_oxc_lint;
-use oxc_semantic::{pg::neighbors_filtered_by_edge_weight, AstNodeId, BasicBlockId, EdgeType};
+use oxc_semantic::{
+    petgraph::visit::EdgeRef, pg::neighbors_filtered_by_edge_weight, AstNodeId, BasicBlockId,
+    ControlFlowGraph, EdgeType,
+};
 use oxc_span::{GetSpan, Span};
 
 use crate::{context::LintContext, rule::Rule, AstNode};
@@ -48,6 +51,7 @@ enum DefinitelyCallsThisBeforeSuper {
     #[default]
     No,
     Yes,
+    Maybe(BasicBlockId),
 }
 
 impl Rule for NoThisBeforeSuper {
@@ -99,35 +103,20 @@ impl Rule for NoThisBeforeSuper {
         // second pass, walk cfg for wanted nodes and propagate
         // cross-block super calls:
         for node in wanted_nodes {
-            let output = neighbors_filtered_by_edge_weight(
-                &cfg.graph,
+            let output = Self::analyze(
+                cfg,
                 node.cfg_id(),
-                &|edge| match edge {
-                    EdgeType::Jump | EdgeType::Normal => None,
-                    EdgeType::Unreachable | EdgeType::Backedge | EdgeType::NewFunction => {
-                        Some(DefinitelyCallsThisBeforeSuper::No)
-                    }
-                },
-                &mut |basic_block_id, _| {
-                    let super_called = basic_blocks_with_super_called.contains(basic_block_id);
-                    if basic_blocks_with_local_violations.contains_key(basic_block_id) {
-                        // super was not called before this in the current code path:
-                        return (DefinitelyCallsThisBeforeSuper::Yes, false);
-                    }
-
-                    if super_called {
-                        (DefinitelyCallsThisBeforeSuper::No, false)
-                    } else {
-                        (DefinitelyCallsThisBeforeSuper::No, true)
-                    }
-                },
+                &basic_blocks_with_super_called,
+                &basic_blocks_with_local_violations,
+                false,
             );
 
-            // Deciding whether we definitely call this before super in all
-            // codepaths is as simple as seeing if any individual codepath
-            // definitely calls this before super.
-            let violation_in_any_codepath =
-                output.into_iter().any(|y| matches!(y, DefinitelyCallsThisBeforeSuper::Yes));
+            let violation_in_any_codepath = Self::check_for_violation(
+                cfg,
+                output,
+                &basic_blocks_with_super_called,
+                &basic_blocks_with_local_violations,
+            );
 
             // If not, flag it as a diagnostic.
             if violation_in_any_codepath {
@@ -163,6 +152,108 @@ impl NoThisBeforeSuper {
 
         false
     }
+
+    fn analyze(
+        cfg: &ControlFlowGraph,
+        id: BasicBlockId,
+        basic_blocks_with_super_called: &HashSet<oxc_semantic::petgraph::prelude::NodeIndex>,
+        basic_blocks_with_local_violations: &HashMap<
+            oxc_semantic::petgraph::prelude::NodeIndex,
+            Vec<AstNodeId>,
+        >,
+        follow_join: bool,
+    ) -> Vec<DefinitelyCallsThisBeforeSuper> {
+        neighbors_filtered_by_edge_weight(
+            &cfg.graph,
+            id,
+            &|edge| match edge {
+                EdgeType::Jump | EdgeType::Normal => None,
+                EdgeType::Join if follow_join => None,
+                EdgeType::Unreachable
+                | EdgeType::Join
+                | EdgeType::Error(_)
+                | EdgeType::Finalize
+                | EdgeType::Backedge
+                | EdgeType::NewFunction => Some(DefinitelyCallsThisBeforeSuper::No),
+            },
+            &mut |basic_block_id, _| {
+                let super_called = basic_blocks_with_super_called.contains(basic_block_id);
+                if basic_blocks_with_local_violations.contains_key(basic_block_id) {
+                    // super was not called before this in the current code path:
+                    return (DefinitelyCallsThisBeforeSuper::Yes, false);
+                }
+
+                if super_called {
+                    // If super is called but we are in a try-catch(-finally) block mark it as a
+                    // maybe, since we might throw on super call and still call this in
+                    // `catch`/`finally` block(s).
+                    if cfg.graph.edges(*basic_block_id).any(|it| {
+                        matches!(
+                            it.weight(),
+                            EdgeType::Error(oxc_semantic::ErrorEdgeKind::Explicit)
+                                | EdgeType::Finalize
+                        )
+                    }) {
+                        (DefinitelyCallsThisBeforeSuper::Maybe(*basic_block_id), false)
+                    // Otherwise we know for sure that super is called in this branch before
+                    // reaching a this expression.
+                    } else {
+                        (DefinitelyCallsThisBeforeSuper::No, false)
+                    }
+                // If we haven't visited a super call and we have a non-error/finalize path
+                // forward, continue visiting this branch.
+                } else if cfg
+                    .graph
+                    .edges(*basic_block_id)
+                    .any(|it| !matches!(it.weight(), EdgeType::Error(_) | EdgeType::Finalize))
+                {
+                    (DefinitelyCallsThisBeforeSuper::No, true)
+                // Otherwise we mark it as a `Maybe` so we can analyze error/finalize paths separately.
+                } else {
+                    (DefinitelyCallsThisBeforeSuper::Maybe(*basic_block_id), false)
+                }
+            },
+        )
+    }
+
+    fn check_for_violation(
+        cfg: &ControlFlowGraph,
+        output: Vec<DefinitelyCallsThisBeforeSuper>,
+        basic_blocks_with_super_called: &HashSet<oxc_semantic::petgraph::prelude::NodeIndex>,
+        basic_blocks_with_local_violations: &HashMap<
+            oxc_semantic::petgraph::prelude::NodeIndex,
+            Vec<AstNodeId>,
+        >,
+    ) -> bool {
+        // Deciding whether we definitely call this before super in all
+        // codepaths is as simple as seeing if any individual codepath
+        // definitely calls this before super.
+        output.into_iter().any(|y| match y {
+            DefinitelyCallsThisBeforeSuper::Yes => true,
+            DefinitelyCallsThisBeforeSuper::No => false,
+            DefinitelyCallsThisBeforeSuper::Maybe(id) => cfg.graph.edges(id).any(|edge| {
+                let weight = edge.weight();
+                let is_explicit_error =
+                    matches!(weight, EdgeType::Error(oxc_semantic::ErrorEdgeKind::Explicit));
+                if is_explicit_error || matches!(weight, EdgeType::Finalize) {
+                    Self::check_for_violation(
+                        cfg,
+                        Self::analyze(
+                            cfg,
+                            edge.target(),
+                            basic_blocks_with_super_called,
+                            basic_blocks_with_local_violations,
+                            is_explicit_error,
+                        ),
+                        basic_blocks_with_super_called,
+                        basic_blocks_with_local_violations,
+                    )
+                } else {
+                    false
+                }
+            }),
+        })
+    }
 }
 
 #[test]

crates/oxc_linter/src/rules/react/require_render_return.rs

@@ -100,9 +100,12 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
             // We only care about normal edges having a return statement.
             EdgeType::Jump | EdgeType::Normal => None,
             // For these two type, we flag it as not found.
-            EdgeType::Unreachable | EdgeType::NewFunction | EdgeType::Backedge => {
-                Some(FoundReturn::No)
-            }
+            EdgeType::Unreachable
+            | EdgeType::Error(_)
+            | EdgeType::Finalize
+            | EdgeType::Join
+            | EdgeType::NewFunction
+            | EdgeType::Backedge => Some(FoundReturn::No),
         },
         &mut |basic_block_id, _state_going_into_this_rule| {
             // If its an arrow function with an expression, marked as founded and stop walking.

crates/oxc_linter/src/rules/react/rules_of_hooks.rs

@@ -4,7 +4,7 @@ use oxc_ast::{
 };
 use oxc_macros::declare_oxc_lint;
 use oxc_semantic::{
-    algo, petgraph::visit::Control, AstNodeId, AstNodes, BasicBlockId, EdgeType, InstructionKind,
+    algo, petgraph::visit::Control, AstNodeId, AstNodes, EdgeType, ErrorEdgeKind, InstructionKind,
 };
 use oxc_span::{Atom, CompactStr};
 use oxc_syntax::operator::AssignmentOperator;
@@ -238,7 +238,7 @@ impl Rule for RulesOfHooks {
             return ctx.diagnostic(diagnostics::loop_hook(span, hook_name));
         }
 
-        if has_conditional_path_accept_throw(ctx, func_cfg_id, node_cfg_id) {
+        if has_conditional_path_accept_throw(ctx, parent_func, node) {
             #[allow(clippy::needless_return)]
             return ctx.diagnostic(diagnostics::conditional_hook(span, hook_name));
         }
@@ -247,20 +247,62 @@ impl Rule for RulesOfHooks {
 
 fn has_conditional_path_accept_throw(
     ctx: &LintContext<'_>,
-    from: BasicBlockId,
-    to: BasicBlockId,
+    from: &AstNode<'_>,
+    to: &AstNode<'_>,
 ) -> bool {
+    let from_graph_id = from.cfg_id();
+    let to_graph_id = to.cfg_id();
     let cfg = ctx.semantic().cfg();
     let graph = &cfg.graph;
+    if graph
+        .edges(to_graph_id)
+        .any(|it| matches!(it.weight(), EdgeType::Error(ErrorEdgeKind::Explicit)))
+    {
+        // TODO: We are simplifying here, There is a real need for a trait like `MayThrow` that
+        // would provide a method `may_throw`, since not everything may throw and break the control flow.
+        return true;
+        // let paths = algo::all_simple_paths::<Vec<_>, _>(graph, from_graph_id, to_graph_id, 0, None);
+        // if paths
+        //     .flatten()
+        //     .flat_map(|id| cfg.basic_block(id).instructions())
+        //     .filter_map(|it| match it {
+        //         Instruction { kind: InstructionKind::Statement, node_id: Some(node_id) } => {
+        //             let r = Some(nodes.get_node(*node_id));
+        //             dbg!(&r);
+        //             r
+        //         }
+        //         _ => None,
+        //     })
+        //     .filter(|it| it.id() != to.id())
+        //     .any(|it| {
+        //         // TODO: it.may_throw()
+        //         matches!(
+        //             it.kind(),
+        //             AstKind::ExpressionStatement(ExpressionStatement {
+        //                 expression: Expression::CallExpression(_),
+        //                 ..
+        //             })
+        //         )
+        //     })
+        // {
+        //     // return true;
+        // }
+    }
     // All nodes should be able to reach the hook node, Otherwise we have a conditional/branching flow.
-    algo::dijkstra(graph, from, Some(to), |e| match e.weight() {
-        EdgeType::NewFunction => 1,
-        EdgeType::Jump | EdgeType::Unreachable | EdgeType::Backedge | EdgeType::Normal => 0,
+    algo::dijkstra(graph, from_graph_id, Some(to_graph_id), |e| match e.weight() {
+        EdgeType::NewFunction | EdgeType::Error(ErrorEdgeKind::Implicit) => 1,
+        EdgeType::Error(ErrorEdgeKind::Explicit)
+        | EdgeType::Join
+        | EdgeType::Finalize
+        | EdgeType::Jump
+        | EdgeType::Unreachable
+        | EdgeType::Backedge
+        | EdgeType::Normal => 0,
     })
     .into_iter()
     .filter(|(_, val)| *val == 0)
     .any(|(f, _)| {
-        !cfg.is_reachabale_filtered(f, to, |it| {
+        !cfg.is_reachabale_filtered(f, to_graph_id, |it| {
             if cfg
                 .basic_block(it)
                 .instructions()

crates/oxc_linter/src/snapshots/empty_brace_spaces.snap

@@ -93,13 +93,6 @@ 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)
@@ -268,13 +261,6 @@ 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)
@@ -450,13 +436,6 @@ 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)
@@ -638,14 +617,6 @@ 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 {
@@ -825,13 +796,6 @@ 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 {