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

Hides petgraph's general `NodeIndex` type behind `BasicBlockId`.
This commit is contained in:
rzvxa 2024-05-22 03:09:38 +00:00
parent 40ab95b055
commit 78e6326e48
10 changed files with 77 additions and 74 deletions

View file

@ -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.
//

View file

@ -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 => {

View file

@ -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)

View file

@ -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)
})
})
}

View file

@ -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);
}

View file

@ -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

View file

@ -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")
}
}

View file

@ -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},

View file

@ -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> {

View file

@ -1,15 +1,17 @@
use petgraph::{stable_graph::NodeIndex, visit::EdgeRef, Direction, Graph};
use petgraph::{visit::EdgeRef, Direction, Graph};
use crate::BasicBlockId;
/// # Panics
pub fn neighbors_filtered_by_edge_weight<State: Default + Clone, NodeWeight, EdgeWeight, F, G>(
graph: &Graph<NodeWeight, EdgeWeight>,
node: NodeIndex,
node: BasicBlockId,
edge_filter: &F,
visitor: &mut G,
) -> Vec<State>
where
F: Fn(&EdgeWeight) -> Option<State>,
G: FnMut(&NodeIndex, State) -> (State, bool),
G: FnMut(&BasicBlockId, State) -> (State, bool),
{
let mut q = vec![];
let mut final_states = vec![];