Skip to content

refactor(semantic/cfg): cleanup control flow and it's builder. #3650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rzvxa wants to merge 1 commit into from
Closed

refactor(semantic/cfg): cleanup control flow and it's builder. #3650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/oxc_linter/src/rules/react/rules_of_hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ impl Rule for RulesOfHooks {
return;
}

if !ctx.semantic().cfg().is_reachabale(func_cfg_id, node_cfg_id) {
if !ctx.semantic().cfg().is_reachable(func_cfg_id, node_cfg_id) {
// There should always be a control flow path between a parent and child node.
// If there is none it means we always do an early exit before reaching our hook call.
// In some cases it might mean that we are operating on an invalid `cfg` but in either
Expand Down Expand Up @@ -302,7 +302,7 @@ fn has_conditional_path_accept_throw(
.into_iter()
.filter(|(_, val)| *val == 0)
.any(|(f, _)| {
!cfg.is_reachabale_filtered(f, to_graph_id, |it| {
!cfg.is_reachable_filtered(f, to_graph_id, |it| {
if cfg
.basic_block(it)
.instructions()
Expand Down
36 changes: 2 additions & 34 deletions crates/oxc_semantic/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
class::ClassTableBuilder,
control_flow::{
ControlFlowGraphBuilder, CtxCursor, CtxFlags, EdgeType, ErrorEdgeKind,
IterationInstructionKind, Register, ReturnInstructionKind,
IterationInstructionKind, ReturnInstructionKind,
},
diagnostics::redeclaration,
jsdoc::JSDocBuilder,
Expand Down Expand Up @@ -541,13 +541,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
fn visit_debugger_statement(&mut self, stmt: &DebuggerStatement) {
let kind = AstKind::DebuggerStatement(self.alloc(stmt));
self.enter_node(kind);

/* cfg */
// just take the next_label since it should be taken by the next
// statement regardless of whether the statement can use it or not
self.cfg.next_label.take();
/* cfg */

self.leave_node(kind);
}

Expand Down Expand Up @@ -1006,10 +999,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {

self.visit_label_identifier(&stmt.label);

/* cfg */
self.cfg.next_label = Some(stmt.label.name.to_compact_str());
/* cfg */

self.visit_statement(&stmt.body);

/* cfg */
Expand Down Expand Up @@ -1373,8 +1362,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
func.scope_id.set(Some(self.current_scope_id));

/* cfg */
let preserved = self.cfg.preserve_expression_state();

let before_function_graph_ix = self.cfg.current_node_ix;
let error_harness = self.cfg.attach_error_harness(ErrorEdgeKind::Implicit);
let function_graph_ix = self.cfg.new_basic_block_function();
Expand All @@ -1398,7 +1385,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
}

/* cfg */
self.cfg.restore_expression_state(preserved);
self.cfg.ctx(None).resolve_expect(CtxFlags::FUNCTION);
self.cfg.release_error_harness(error_harness);
let after_function_graph_ix = self.cfg.new_basic_block_normal();
Expand Down Expand Up @@ -1435,11 +1421,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {

self.enter_node(kind);

/* cfg */
let preserved = self.cfg.preserve_expression_state();
self.cfg.store_final_assignments_into_this_array.push(vec![]);
/* cfg */

if let Some(id) = &class.id {
self.visit_binding_identifier(id);
}
Expand All @@ -1455,14 +1436,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
}
self.visit_class_body(&class.body);

/* cfg */
let _elements = self.cfg.store_final_assignments_into_this_array.pop().expect(
"expected there to be atleast one vec in the store_final_assignments_into_this_arrays",
);
self.cfg.restore_expression_state(preserved);
self.cfg.spread_indices.push(vec![]);
/* cfg */

self.leave_node(kind);
if is_class_expr {
self.leave_scope();
Expand All @@ -1485,7 +1458,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
expr.scope_id.set(Some(self.current_scope_id));

/* cfg */
let preserved = self.cfg.preserve_expression_state();
let current_node_ix = self.cfg.current_node_ix;
let error_harness = self.cfg.attach_error_harness(ErrorEdgeKind::Implicit);
let function_graph_ix = self.cfg.new_basic_block_function();
Expand All @@ -1500,15 +1472,11 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {

/* cfg */
self.cfg.add_edge(current_node_ix, function_graph_ix, EdgeType::NewFunction);
if expr.expression {
self.cfg.store_assignments_into_this_array.push(vec![]);
self.cfg.use_this_register = Some(Register::Return);
}
/* cfg */

self.visit_function_body(&expr.body);

/* cfg */
self.cfg.restore_expression_state(preserved);
self.cfg.ctx(None).resolve_expect(CtxFlags::FUNCTION);
self.cfg.release_error_harness(error_harness);
self.cfg.current_node_ix = current_node_ix;
Expand Down
47 changes: 2 additions & 45 deletions crates/oxc_semantic/src/control_flow/builder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ pub use context::{CtxCursor, CtxFlags};
use petgraph::Direction;

use super::{
AstNodeId, BasicBlock, BasicBlockId, CompactStr, ControlFlowGraph, EdgeType, ErrorEdgeKind,
Graph, Instruction, InstructionKind, IterationInstructionKind, LabeledInstruction,
PreservedExpressionState, Register,
AstNodeId, BasicBlock, BasicBlockId, ControlFlowGraph, EdgeType, ErrorEdgeKind, Graph,
Instruction, InstructionKind, IterationInstructionKind, LabeledInstruction,
};

#[derive(Debug, Default)]
Expand All @@ -25,27 +24,6 @@ pub struct ControlFlowGraphBuilder<'a> {
error_path: Vec<ErrorHarness>,
/// Stack of finalizers, the top most element is always the appropriate one for current node.
finalizers: Vec<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?
pub use_this_register: Option<Register>,
pub next_free_register: u32,
pub store_assignments_into_this_array: Vec<Vec<Register>>,
pub store_final_assignments_into_this_array: Vec<Vec<Register>>,
// indexes of spreads in the store_assignments_into_this_array
pub spread_indices: Vec<Vec<usize>>,
// computed member expressions are only executed when we reach
// that part of the chain, so we keep this vec to patch them in later
pub should_save_stores_for_patching: bool,
pub saved_store: Option<usize>,
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<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<BasicBlockId>,
}

impl<'a> ControlFlowGraphBuilder<'a> {
Expand Down Expand Up @@ -243,27 +221,6 @@ impl<'a> ControlFlowGraphBuilder<'a> {
self.basic_block_mut(block).instructions.push(Instruction { kind, node_id });
}

#[must_use]
pub fn preserve_expression_state(&mut self) -> PreservedExpressionState {
let use_this_register = self.use_this_register.take();
let mut store_final_assignments_into_this_array = vec![];
std::mem::swap(
&mut store_final_assignments_into_this_array,
&mut self.store_final_assignments_into_this_array,
);

// DO NOT preserve: saved_stores, should_save_stores_for_patching
// should_save_stores_for_patching must always be active to catch
// all stores, preserving will mess it up.
PreservedExpressionState { use_this_register, store_final_assignments_into_this_array }
}

pub fn restore_expression_state(&mut self, mut preserved_state: PreservedExpressionState) {
self.use_this_register = preserved_state.use_this_register.take();
self.store_final_assignments_into_this_array =
preserved_state.store_final_assignments_into_this_array;
}

pub fn enter_statement(&mut self, stmt: AstNodeId) {
self.push_statement(stmt);
}
Expand Down
23 changes: 3 additions & 20 deletions crates/oxc_semantic/src/control_flow/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,11 @@ impl ControlFlowGraph {
self.basic_blocks.get_mut(ix).expect("expected a valid node id in self.basic_blocks")
}

pub fn is_reachabale(&self, from: BasicBlockId, to: BasicBlockId) -> bool {
self.is_reachabale_filtered(from, to, |_| Control::Continue)
pub fn is_reachable(&self, from: BasicBlockId, to: BasicBlockId) -> bool {
self.is_reachable_filtered(from, to, |_| Control::Continue)
}

pub fn is_reachabale_filtered<F: Fn(BasicBlockId) -> Control<bool>>(
pub fn is_reachable_filtered<F: Fn(BasicBlockId) -> Control<bool>>(
&self,
from: BasicBlockId,
to: BasicBlockId,
Expand Down Expand Up @@ -352,21 +352,4 @@ impl ControlFlowGraph {
})
.is_err()
}

pub fn has_conditional_path(&self, from: BasicBlockId, to: BasicBlockId) -> bool {
let graph = &self.graph;
// All nodes should be able to reach the `to` node, Otherwise we have a conditional/branching flow.
petgraph::algo::dijkstra(graph, from, Some(to), |e| match e.weight() {
EdgeType::NewFunction | EdgeType::Error(_) | EdgeType::Finalize | EdgeType::Join => 1,
EdgeType::Jump | EdgeType::Unreachable | EdgeType::Backedge | EdgeType::Normal => 0,
})
.into_iter()
.filter(|(_, val)| *val == 0)
.any(|(f, _)| !self.is_reachabale(f, to))
}
}

pub struct PreservedExpressionState {
pub use_this_register: Option<Register>,
pub store_final_assignments_into_this_array: Vec<Vec<Register>>,
}