refactor(semantic/cfg): alias petgraph's NodeIndex as BasicBlockId.

rzvxa · rzvxa · commit e0649aa885d8 · 2024-05-22T01:31:44.000+03:30
diff --git a/crates/oxc_linter/src/rules/eslint/getter_return.rs b/crates/oxc_linter/src/rules/eslint/getter_return.rs
@@ -182,7 +182,7 @@ impl GetterReturn {
 
         let output = neighbors_filtered_by_edge_weight(
             &cfg.graph,
-            node.cfg_ix(),
+            node.cfg_id(),
             &|edge| match edge {
                 EdgeType::Normal => None,
                 // We don't need to handle backedges because we would have already visited
@@ -229,7 +229,7 @@ impl GetterReturn {
                 }
 
                 // Scan through the values in this basic block.
-                for entry in cfg.basic_block_by_index(*basic_block_id) {
+                for entry in cfg.basic_block(*basic_block_id) {
                     match entry {
                         // If the element is an assignment.
                         //
diff --git a/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs b/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs
@@ -7,9 +7,7 @@ use oxc_ast::{
     AstKind,
 };
 use oxc_macros::declare_oxc_lint;
-use oxc_semantic::{
-    petgraph::stable_graph::NodeIndex, pg::neighbors_filtered_by_edge_weight, AstNodeId, EdgeType,
-};
+use oxc_semantic::{pg::neighbors_filtered_by_edge_weight, AstNodeId, BasicBlockId, EdgeType};
 use oxc_span::{GetSpan, Span};
 
 use crate::{context::LintContext, rule::Rule, AstNode};
@@ -59,8 +57,8 @@ impl Rule for NoThisBeforeSuper {
 
         // first pass -> find super calls and local violations
         let mut wanted_nodes = Vec::new();
-        let mut basic_blocks_with_super_called = HashSet::<NodeIndex>::new();
-        let mut basic_blocks_with_local_violations = HashMap::<NodeIndex, Vec<AstNodeId>>::new();
+        let mut basic_blocks_with_super_called = HashSet::<BasicBlockId>::new();
+        let mut basic_blocks_with_local_violations = HashMap::<BasicBlockId, Vec<AstNodeId>>::new();
         for node in semantic.nodes().iter() {
             match node.kind() {
                 AstKind::Function(_) | AstKind::ArrowFunctionExpression(_) => {
@@ -69,7 +67,7 @@ impl Rule for NoThisBeforeSuper {
                     }
                 }
                 AstKind::Super(_) => {
-                    let basic_block_id = node.cfg_ix();
+                    let basic_block_id = node.cfg_id();
                     if let Some(parent) = semantic.nodes().parent_node(node.id()) {
                         if let AstKind::CallExpression(_) = parent.kind() {
                             // Note: we don't need to worry about also having invalid
@@ -86,7 +84,7 @@ impl Rule for NoThisBeforeSuper {
                     }
                 }
                 AstKind::ThisExpression(_) => {
-                    let basic_block_id = node.cfg_ix();
+                    let basic_block_id = node.cfg_id();
                     if !basic_blocks_with_super_called.contains(&basic_block_id) {
                         basic_blocks_with_local_violations
                             .entry(basic_block_id)
@@ -103,7 +101,7 @@ impl Rule for NoThisBeforeSuper {
         for node in wanted_nodes {
             let output = neighbors_filtered_by_edge_weight(
                 &cfg.graph,
-                node.cfg_ix(),
+                node.cfg_id(),
                 &|edge| match edge {
                     EdgeType::Normal => None,
                     EdgeType::Backedge | EdgeType::NewFunction => {
diff --git a/crates/oxc_linter/src/rules/react/require_render_return.rs b/crates/oxc_linter/src/rules/react/require_render_return.rs
@@ -94,7 +94,7 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
     let cfg = ctx.semantic().cfg();
     let state = neighbors_filtered_by_edge_weight(
         &cfg.graph,
-        node.cfg_ix(),
+        node.cfg_id(),
         &|edge| match edge {
             // We only care about normal edges having a return statement.
             EdgeType::Normal => None,
@@ -109,7 +109,7 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
                 }
             }
 
-            for entry in cfg.basic_block_by_index(*basic_block_id) {
+            for entry in cfg.basic_block(*basic_block_id) {
                 if let BasicBlockElement::Assignment(to_reg, val) = entry {
                     if matches!(to_reg, Register::Return)
                         && matches!(val, AssignmentValue::NotImplicitUndefined)
diff --git a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs
@@ -5,9 +5,9 @@ use oxc_ast::{
 };
 use oxc_macros::declare_oxc_lint;
 use oxc_semantic::{
-    petgraph::{self, graph::NodeIndex},
+    petgraph::{self},
     pg::neighbors_filtered_by_edge_weight,
-    AstNodeId, AstNodes, BasicBlockElement, EdgeType, Register,
+    AstNodeId, AstNodes, BasicBlockElement, BasicBlockId, EdgeType, Register,
 };
 use oxc_span::{Atom, CompactStr};
 use oxc_syntax::operator::AssignmentOperator;
@@ -220,18 +220,18 @@ impl Rule for RulesOfHooks {
             return;
         }
 
-        let node_cfg_ix = node.cfg_ix();
-        let func_cfg_ix = parent_func.cfg_ix();
+        let node_cfg_id = node.cfg_id();
+        let func_cfg_id = parent_func.cfg_id();
 
         // there is no branch between us and our parent function
-        if node_cfg_ix == func_cfg_ix {
+        if node_cfg_id == func_cfg_id {
             return;
         }
 
         if !petgraph::algo::has_path_connecting(
             &semantic.cfg().graph,
-            func_cfg_ix,
-            node_cfg_ix,
+            func_cfg_id,
+            node_cfg_id,
             None,
         ) {
             // There should always be a control flow path between a parent and child node.
@@ -246,8 +246,8 @@ impl Rule for RulesOfHooks {
             return ctx.diagnostic(diagnostics::loop_hook(span, hook_name));
         }
 
-        if self.is_conditional(ctx, func_cfg_ix, node_cfg_ix)
-            || self.breaks_early(ctx, func_cfg_ix, node_cfg_ix)
+        if self.is_conditional(ctx, func_cfg_id, node_cfg_id)
+            || self.breaks_early(ctx, func_cfg_id, node_cfg_id)
         {
             #[allow(clippy::needless_return)]
             return ctx.diagnostic(diagnostics::conditional_hook(span, hook_name));
@@ -278,42 +278,42 @@ impl RulesOfHooks {
     fn is_conditional(
         &self,
         ctx: &LintContext,
-        func_cfg_ix: NodeIndex,
-        node_cfg_ix: NodeIndex,
+        func_cfg_id: BasicBlockId,
+        node_cfg_id: BasicBlockId,
     ) -> bool {
         let graph = &ctx.semantic().cfg().graph;
         // All nodes should be reachable from our hook, Otherwise we have a conditional/branching flow.
-        petgraph::algo::dijkstra(graph, func_cfg_ix, Some(node_cfg_ix), |e| match e.weight() {
+        petgraph::algo::dijkstra(graph, func_cfg_id, Some(node_cfg_id), |e| match e.weight() {
             EdgeType::NewFunction => 1,
             EdgeType::Backedge | EdgeType::Normal => 0,
         })
         .into_iter()
         .filter(|(_, val)| *val == 0)
-        .any(|(f, _)| !petgraph::algo::has_path_connecting(graph, f, node_cfg_ix, None))
+        .any(|(f, _)| !petgraph::algo::has_path_connecting(graph, f, node_cfg_id, None))
     }
 
     #[inline(always)]
     fn breaks_early(
         &self,
         ctx: &LintContext,
-        func_cfg_ix: NodeIndex,
-        node_cfg_ix: NodeIndex,
+        func_cfg_id: BasicBlockId,
+        node_cfg_id: BasicBlockId,
     ) -> bool {
         let cfg = ctx.semantic().cfg();
         neighbors_filtered_by_edge_weight(
             &cfg.graph,
-            func_cfg_ix,
+            func_cfg_id,
             &|e| match e {
                 EdgeType::Normal => None,
                 EdgeType::Backedge | EdgeType::NewFunction => Some(State::default()),
             },
-            &mut |ix: &NodeIndex, mut state: State| {
-                if node_cfg_ix == *ix {
+            &mut |id: &BasicBlockId, mut state: State| {
+                if node_cfg_id == *id {
                     return (state, false);
                 }
 
                 let (push, keep_walking) = cfg
-                    .basic_block_by_index(*ix)
+                    .basic_block(*id)
                     .iter()
                     .fold_while((false, true), |acc, it| match it {
                         BasicBlockElement::Break(_) => FoldWhile::Done((true, false)),
@@ -327,7 +327,7 @@ impl RulesOfHooks {
                     .into_inner();
 
                 if push {
-                    state.0.push(*ix);
+                    state.0.push(*id);
                 }
                 (state, keep_walking)
             },
@@ -340,7 +340,7 @@ impl RulesOfHooks {
 }
 
 #[derive(Debug, Default, Clone)]
-struct State(Vec<NodeIndex>);
+struct State(Vec<BasicBlockId>);
 
 fn parent_func<'a>(nodes: &'a AstNodes<'a>, node: &AstNode) -> Option<&'a AstNode<'a>> {
     nodes.ancestors(node.id()).map(|id| nodes.get_node(id)).find(|it| it.kind().is_function_like())
@@ -380,10 +380,10 @@ fn is_somewhere_inside_component_or_hook(nodes: &AstNodes, node_id: AstNodeId) -
                 },
             )
         })
-        .any(|(ix, id)| {
-            id.is_some_and(|name| {
+        .any(|(id, ident)| {
+            ident.is_some_and(|name| {
                 is_react_component_or_hook_name(name.as_str())
-                    || is_memo_or_forward_ref_callback(nodes, ix)
+                    || is_memo_or_forward_ref_callback(nodes, id)
             })
         })
 }
diff --git a/crates/oxc_semantic/examples/cfg.rs b/crates/oxc_semantic/examples/cfg.rs
@@ -54,7 +54,7 @@ fn main() -> std::io::Result<()> {
 
     let mut ast_nodes_by_block = HashMap::<_, Vec<_>>::new();
     for node in semantic.semantic.nodes().iter() {
-        let block = node.cfg_ix();
+        let block = node.cfg_id();
         let block_ix = semantic.semantic.cfg().graph.node_weight(block).unwrap();
         ast_nodes_by_block.entry(*block_ix).or_default().push(node);
     }
diff --git a/crates/oxc_semantic/src/control_flow/builder.rs b/crates/oxc_semantic/src/control_flow/builder.rs
@@ -1,12 +1,14 @@
 use super::{
-    AssignmentValue, AstNodeId, BasicBlockElement, CompactStr, ControlFlowGraph, EdgeType, Graph,
-    NodeIndex, PreservedExpressionState, PreservedStatementState, Register,
+    AssignmentValue, AstNodeId, BasicBlockElement, BasicBlockId, CompactStr, ControlFlowGraph,
+    EdgeType, Graph, PreservedExpressionState, PreservedStatementState, Register,
     StatementControlFlowType,
 };
 
 #[derive(Debug, Default)]
 pub struct ControlFlowGraphBuilder {
+    pub graph: Graph<usize, EdgeType>,
     pub basic_blocks: Vec<Vec<BasicBlockElement>>,
+    pub current_node_ix: BasicBlockId,
     // note: this should only land in the big box for all things that take arguments
     // ie: callexpression, arrayexpression, etc
     // todo: add assert that it is used every time?
@@ -22,16 +24,14 @@ pub struct ControlFlowGraphBuilder {
     // (start, tail, last_register_used)
     pub saved_stores: Vec<(Vec<BasicBlockElement>, Option<Register>)>,
     pub saved_store: Option<usize>,
-    pub graph: Graph<usize, EdgeType>,
-    pub current_node_ix: NodeIndex,
-    pub basic_blocks_with_breaks: Vec<Vec<NodeIndex>>,
-    pub basic_blocks_with_continues: Vec<Vec<NodeIndex>>,
+    pub basic_blocks_with_breaks: Vec<Vec<BasicBlockId>>,
+    pub basic_blocks_with_continues: Vec<Vec<BasicBlockId>>,
     // node indexes of the basic blocks of switch case conditions
-    pub switch_case_conditions: Vec<Vec<NodeIndex>>,
+    pub switch_case_conditions: Vec<Vec<BasicBlockId>>,
     pub next_label: Option<CompactStr>,
     pub label_to_ast_node_ix: Vec<(CompactStr, AstNodeId)>,
     pub ast_node_to_break_continue: Vec<(AstNodeId, usize, Option<usize>)>,
-    pub after_throw_block: Option<NodeIndex>,
+    pub after_throw_block: Option<BasicBlockId>,
 }
 
 impl ControlFlowGraphBuilder {
@@ -51,7 +51,7 @@ impl ControlFlowGraphBuilder {
     }
 
     #[must_use]
-    pub fn new_basic_block_for_function(&mut self) -> NodeIndex {
+    pub fn new_basic_block_for_function(&mut self) -> BasicBlockId {
         self.basic_blocks.push(Vec::new());
         let basic_block_id = self.basic_blocks.len() - 1;
         let graph_index = self.graph.add_node(basic_block_id);
@@ -66,7 +66,7 @@ impl ControlFlowGraphBuilder {
     }
 
     #[must_use]
-    pub fn new_basic_block(&mut self) -> NodeIndex {
+    pub fn new_basic_block(&mut self) -> BasicBlockId {
         self.basic_blocks.push(Vec::new());
         let graph_index = self.graph.add_node(self.basic_blocks.len() - 1);
         self.current_node_ix = graph_index;
@@ -79,7 +79,7 @@ impl ControlFlowGraphBuilder {
         graph_index
     }
 
-    pub fn add_edge(&mut self, a: NodeIndex, b: NodeIndex, weight: EdgeType) {
+    pub fn add_edge(&mut self, a: BasicBlockId, b: BasicBlockId, weight: EdgeType) {
         self.graph.add_edge(a, b, weight);
     }
 
@@ -202,8 +202,8 @@ impl ControlFlowGraphBuilder {
         &mut self,
         preserved_state: &PreservedStatementState,
         id: AstNodeId,
-        break_jump_position: NodeIndex<u32>,
-        continue_jump_position: Option<NodeIndex<u32>>,
+        break_jump_position: BasicBlockId,
+        continue_jump_position: Option<BasicBlockId>,
     ) {
         let basic_blocks_with_breaks = self
             .basic_blocks_with_breaks
diff --git a/crates/oxc_semantic/src/control_flow/mod.rs b/crates/oxc_semantic/src/control_flow/mod.rs
@@ -10,6 +10,8 @@ use crate::AstNodeId;
 
 pub use builder::ControlFlowGraphBuilder;
 
+pub type BasicBlockId = NodeIndex;
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum Register {
     Index(u32),
@@ -128,17 +130,15 @@ pub struct ControlFlowGraph {
 
 impl ControlFlowGraph {
     /// # Panics
-    pub fn basic_block_by_index(&self, index: NodeIndex) -> &Vec<BasicBlockElement> {
-        let idx =
-            *self.graph.node_weight(index).expect("expected a valid node index in self.graph");
-        self.basic_blocks.get(idx).expect("expected a valid node index in self.basic_blocks")
+    pub fn basic_block(&self, id: BasicBlockId) -> &Vec<BasicBlockElement> {
+        let ix = *self.graph.node_weight(id).expect("expected a valid node id in self.graph");
+        self.basic_blocks.get(ix).expect("expected a valid node id in self.basic_blocks")
     }
 
     /// # Panics
-    pub fn basic_block_by_index_mut(&mut self, index: NodeIndex) -> &mut Vec<BasicBlockElement> {
-        let idx =
-            *self.graph.node_weight(index).expect("expected a valid node index in self.graph");
-        self.basic_blocks.get_mut(idx).expect("expected a valid node index in self.basic_blocks")
+    pub fn basic_block_mut(&mut self, id: BasicBlockId) -> &mut Vec<BasicBlockElement> {
+        let ix = *self.graph.node_weight(id).expect("expected a valid node id in self.graph");
+        self.basic_blocks.get_mut(ix).expect("expected a valid node id in self.basic_blocks")
     }
 }
 
diff --git a/crates/oxc_semantic/src/lib.rs b/crates/oxc_semantic/src/lib.rs
@@ -31,10 +31,10 @@ use rustc_hash::FxHashSet;
 
 pub use crate::{
     control_flow::{
-        print_basic_block, AssignmentValue, BasicBlockElement, BinaryAssignmentValue, BinaryOp,
-        CallType, CalleeWithArgumentsAssignmentValue, CollectionAssignmentValue, ControlFlowGraph,
-        EdgeType, ObjectPropertyAccessAssignmentValue, Register, UnaryExpressioneAssignmentValue,
-        UpdateAssignmentValue,
+        print_basic_block, AssignmentValue, BasicBlockElement, BasicBlockId, BinaryAssignmentValue,
+        BinaryOp, CallType, CalleeWithArgumentsAssignmentValue, CollectionAssignmentValue,
+        ControlFlowGraph, EdgeType, ObjectPropertyAccessAssignmentValue, Register,
+        UnaryExpressioneAssignmentValue, UpdateAssignmentValue,
     },
     node::{AstNode, AstNodeId, AstNodes},
     reference::{Reference, ReferenceFlag, ReferenceId},
diff --git a/crates/oxc_semantic/src/node.rs b/crates/oxc_semantic/src/node.rs
@@ -1,9 +1,7 @@
-use petgraph::stable_graph::NodeIndex;
-
 use oxc_ast::AstKind;
 use oxc_index::IndexVec;
 
-use crate::scope::ScopeId;
+use crate::{control_flow::BasicBlockId, scope::ScopeId};
 
 pub use oxc_syntax::node::{AstNodeId, NodeFlags};
 
@@ -17,23 +15,28 @@ pub struct AstNode<'a> {
     /// Associated Scope (initialized by binding)
     scope_id: ScopeId,
 
-    /// Associated NodeIndex in CFG (initialized by control_flow)
-    cfg_ix: NodeIndex,
+    /// Associated `BasicBlockId` in CFG (initialized by control_flow)
+    cfg_id: BasicBlockId,
 
     flags: NodeFlags,
 }
 
 impl<'a> AstNode<'a> {
-    pub fn new(kind: AstKind<'a>, scope_id: ScopeId, cfg_ix: NodeIndex, flags: NodeFlags) -> Self {
-        Self { id: AstNodeId::new(0), kind, cfg_ix, scope_id, flags }
+    pub fn new(
+        kind: AstKind<'a>,
+        scope_id: ScopeId,
+        cfg_id: BasicBlockId,
+        flags: NodeFlags,
+    ) -> Self {
+        Self { id: AstNodeId::new(0), kind, cfg_id, scope_id, flags }
     }
 
     pub fn id(&self) -> AstNodeId {
         self.id
     }
 
-    pub fn cfg_ix(&self) -> NodeIndex {
-        self.cfg_ix
+    pub fn cfg_id(&self) -> BasicBlockId {
+        self.cfg_id
     }
 
     pub fn kind(&self) -> AstKind<'a> {
diff --git a/crates/oxc_semantic/src/pg.rs b/crates/oxc_semantic/src/pg.rs

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ fn main() -> std::io::Result<()> {`
`54`	`54`
`55`	`55`	`let mut ast_nodes_by_block = HashMap::<_, Vec<_>>::new();`
`56`	`56`	`for node in semantic.semantic.nodes().iter() {`
`57`		`- let block = node.cfg_ix();`
	`57`	`+ let block = node.cfg_id();`
`58`	`58`	`let block_ix = semantic.semantic.cfg().graph.node_weight(block).unwrap();`
`59`	`59`	`ast_nodes_by_block.entry(*block_ix).or_default().push(node);`
`60`	`60`	`}`