Skip to content

improvement(semantic/cfg): rework basic blocks. #3381

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

Merged
graphite-app merged 1 commit into from
Jun 6, 2024
Merged

improvement(semantic/cfg): rework basic blocks. #3381

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
57 changes: 19 additions & 38 deletions crates/oxc_linter/src/rules/eslint/getter_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use oxc_diagnostics::OxcDiagnostic;

use oxc_macros::declare_oxc_lint;
use oxc_semantic::{
pg::neighbors_filtered_by_edge_weight, AssignmentValue, BasicBlockElement, EdgeType, Register,
pg::neighbors_filtered_by_edge_weight, EdgeType, InstructionKind, ReturnInstructionKind,
};
use oxc_span::Span;

Expand Down Expand Up @@ -191,6 +191,8 @@ impl GetterReturn {
// We don't need to visit NewFunction edges because it's not going to be evaluated
// immediately, and we are only doing a pass over things that will be immediately evaluated
| EdgeType::NewFunction
// Unreachable nodes aren't reachable so we don't follow them.
| EdgeType::Unreachable
// By returning Some(X),
// we signal that we don't walk to this path any farther.
//
Expand Down Expand Up @@ -229,49 +231,28 @@ impl GetterReturn {
}

// Scan through the values in this basic block.
for entry in cfg.basic_block(*basic_block_id) {
match entry {
// If the element is an assignment.
//
// Everything you can write in javascript that would have
// the function continue are expressed as assignments in the cfg.
BasicBlockElement::Assignment(to_reg, val) => {
// If the assignment is to the return register.
//
// The return register is a special register that return statements
// assign the returned value to.
if matches!(to_reg, Register::Return) {
for entry in cfg.basic_block(*basic_block_id).instructions() {
match entry.kind {
// If the element is a return.
// `allow_implicit` allows returning without a value to not
// fail the rule. We check for this by checking if the value
// being returned in the cfg this is expressed as
// `AssignmentValue::ImplicitUndefined`.
//
// There is an assumption being made here that returning an
// `undefined` will put the `undefined` directly into the
// return and will not put the `undefined` into an immediate
// register and return the register. However, the tests for
// this rule enforce that this invariant is not broken.
if !self.allow_implicit
&& matches!(val, AssignmentValue::ImplicitUndefined)
{
// Return false as the second argument to signify we should
// not continue walking this branch, as we know a return
// is the end of this path.
return (DefinitelyReturnsOrThrowsOrUnreachable::No, false);
}
// fail the rule.
// Return false as the second argument to signify we should
// not continue walking this branch, as we know a return
// is the end of this path.
InstructionKind::Return(ReturnInstructionKind::ImplicitUndefined) if !self.allow_implicit => {
return (DefinitelyReturnsOrThrowsOrUnreachable::No, false);
}
// Otherwise, we definitely returned since we assigned
// to the return register.
//
// Return false as the second argument to signify we should
// not continue walking this branch, as we know a return
// is the end of this path.
return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false);
}
}
| InstructionKind::Return(_)
// Throws are classified as returning.
//
// todo: test with catching...
BasicBlockElement::Throw(_) |
| InstructionKind::Throw
// Although the unreachable code is not returned, it will never be executed.
// There is no point in checking it for return.
//
Expand All @@ -283,12 +264,12 @@ impl GetterReturn {
// return -1;
// ```
// Make return useless.
BasicBlockElement::Unreachable => {

return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false);
| InstructionKind::Unreachable =>{
return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false);
}
// Ignore irrelevant elements.
BasicBlockElement::Break(_) => {}
| InstructionKind::Break(_)
| InstructionKind::Statement => {}
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/eslint/no_this_before_super.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Rule for NoThisBeforeSuper {
node.cfg_id(),
&|edge| match edge {
EdgeType::Normal => None,
EdgeType::Backedge | EdgeType::NewFunction => {
EdgeType::Unreachable | EdgeType::Backedge | EdgeType::NewFunction => {
Some(DefinitelyCallsThisBeforeSuper::No)
}
},
Expand Down
25 changes: 13 additions & 12 deletions crates/oxc_linter/src/rules/react/require_render_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use oxc_diagnostics::OxcDiagnostic;

use oxc_macros::declare_oxc_lint;
use oxc_semantic::{
pg::neighbors_filtered_by_edge_weight, AssignmentValue, BasicBlockElement, EdgeType, Register,
pg::neighbors_filtered_by_edge_weight, EdgeType, Instruction, InstructionKind,
ReturnInstructionKind,
};
use oxc_span::{GetSpan, Span};

Expand Down Expand Up @@ -99,7 +100,9 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
// We only care about normal edges having a return statement.
EdgeType::Normal => None,
// For these two type, we flag it as not found.
EdgeType::NewFunction | EdgeType::Backedge => Some(FoundReturn::No),
EdgeType::Unreachable | 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.
Expand All @@ -109,19 +112,17 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
}
}

for entry in cfg.basic_block(*basic_block_id) {
match entry {
BasicBlockElement::Assignment(to_reg, val) => {
if matches!(to_reg, Register::Return)
&& matches!(val, AssignmentValue::NotImplicitUndefined)
{
return (FoundReturn::Yes, STOP_WALKING_ON_THIS_PATH);
}
for Instruction { kind, .. } in cfg.basic_block(*basic_block_id).instructions() {
match kind {
InstructionKind::Return(ReturnInstructionKind::NotImplicitUndefined) => {
return (FoundReturn::Yes, STOP_WALKING_ON_THIS_PATH);
}
BasicBlockElement::Unreachable | BasicBlockElement::Throw(_) => {
InstructionKind::Unreachable | InstructionKind::Throw => {
return (FoundReturn::No, STOP_WALKING_ON_THIS_PATH);
}
BasicBlockElement::Break(_) => {}
InstructionKind::Return(ReturnInstructionKind::ImplicitUndefined)
| InstructionKind::Break(_)
| InstructionKind::Statement => {}
}
}

Expand Down
28 changes: 15 additions & 13 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 @@ -7,7 +7,7 @@ use oxc_macros::declare_oxc_lint;
use oxc_semantic::{
petgraph::{self},
pg::neighbors_filtered_by_edge_weight,
AstNodeId, AstNodes, BasicBlockElement, BasicBlockId, EdgeType, Register,
AstNodeId, AstNodes, BasicBlockId, EdgeType, Instruction, InstructionKind,
};
use oxc_span::{Atom, CompactStr};
use oxc_syntax::operator::AssignmentOperator;
Expand Down Expand Up @@ -281,15 +281,16 @@ impl RulesOfHooks {
func_cfg_id: BasicBlockId,
node_cfg_id: BasicBlockId,
) -> bool {
let graph = &ctx.semantic().cfg().graph;
let cfg = ctx.semantic().cfg();
let graph = &cfg.graph;
// All nodes should be reachable from our hook, Otherwise we have a conditional/branching flow.
petgraph::algo::dijkstra(graph, func_cfg_id, Some(node_cfg_id), |e| match e.weight() {
EdgeType::NewFunction => 1,
EdgeType::Backedge | EdgeType::Normal => 0,
EdgeType::Unreachable | EdgeType::Backedge | EdgeType::Normal => 0,
})
.into_iter()
.filter(|(_, val)| *val == 0)
.any(|(f, _)| !petgraph::algo::has_path_connecting(graph, f, node_cfg_id, None))
.any(|(f, _)| !cfg.is_reachabale(f, node_cfg_id))
}

#[inline(always)]
Expand All @@ -305,7 +306,9 @@ impl RulesOfHooks {
func_cfg_id,
&|e| match e {
EdgeType::Normal => None,
EdgeType::Backedge | EdgeType::NewFunction => Some(State::default()),
EdgeType::Unreachable | EdgeType::Backedge | EdgeType::NewFunction => {
Some(State::default())
}
},
&mut |id: &BasicBlockId, mut state: State| {
if node_cfg_id == *id {
Expand All @@ -314,15 +317,14 @@ impl RulesOfHooks {

let (push, keep_walking) = cfg
.basic_block(*id)
.instructions
.iter()
.fold_while((false, true), |acc, it| match it {
BasicBlockElement::Break(_) => FoldWhile::Done((true, false)),
BasicBlockElement::Unreachable
| BasicBlockElement::Throw(_)
| BasicBlockElement::Assignment(Register::Return, _) => {
FoldWhile::Continue((acc.0, false))
}
BasicBlockElement::Assignment(_, _) => FoldWhile::Continue(acc),
.fold_while((false, true), |acc, Instruction { kind, .. }| match kind {
InstructionKind::Break(_) => FoldWhile::Done((true, false)),
InstructionKind::Unreachable
| InstructionKind::Throw
| InstructionKind::Return(_) => FoldWhile::Continue((acc.0, false)),
InstructionKind::Statement => FoldWhile::Continue(acc),
})
.into_inner();

Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ phf = { workspace = true, features = ["macros"] }
rustc-hash = { workspace = true }
serde = { workspace = true, features = ["derive"], optional = true }
petgraph = { workspace = true }
itertools = { workspace = true }

tsify = { workspace = true, optional = true }
wasm-bindgen = { workspace = true, optional = true }

[dev-dependencies]
oxc_parser = { workspace = true }

itertools = { workspace = true }
indexmap = { workspace = true }
insta = { workspace = true, features = ["glob"] }
phf = { workspace = true, features = ["macros"] }
Expand Down
46 changes: 31 additions & 15 deletions crates/oxc_semantic/examples/cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{collections::HashMap, env, path::Path, sync::Arc};
use itertools::Itertools;
use oxc_allocator::Allocator;
use oxc_parser::Parser;
use oxc_semantic::{print_basic_block, SemanticBuilder};
use oxc_semantic::{DebugDot, DisplayDot, EdgeType, SemanticBuilder};
use oxc_span::SourceType;
use petgraph::dot::{Config, Dot};

Expand Down Expand Up @@ -64,7 +64,7 @@ fn main() -> std::io::Result<()> {
.cfg()
.basic_blocks
.iter()
.map(print_basic_block)
.map(DisplayDot::display_dot)
.enumerate()
.map(|(i, it)| {
format!(
Expand All @@ -88,21 +88,37 @@ fn main() -> std::io::Result<()> {
Dot::with_attr_getters(
&semantic.semantic.cfg().graph,
&[Config::EdgeNoLabel, Config::NodeNoLabel],
&|_graph, edge| format!("label = {:?}", edge.weight()),
&|_graph, node| format!(
"xlabel = {:?}, label = {:?}",
&|_graph, edge| {
let weight = edge.weight();
let label = format!("label = {weight:?}");
if matches!(weight, EdgeType::Unreachable) {
format!("{label}, style = \"dotted\"")
} else {
label
}
},
&|_graph, node| {
let nodes = ast_nodes_by_block.get(node.1).map_or("None".to_string(), |nodes| {
let nodes: Vec<_> =
nodes.iter().map(|node| format!("{}", node.kind().debug_name())).collect();
if nodes.len() > 1 {
format!(
"{}\\l",
nodes.into_iter().map(|it| format!("\\l {it}")).join("")
)
} else {
nodes.into_iter().join("")
}
});
format!(
"bb{} [{}]",
"xlabel = \"nodes [{}]\\l\", label = \"bb{}\n{}\"",
nodes,
node.1,
print_basic_block(&semantic.semantic.cfg().basic_blocks[*node.1],).trim()
),
ast_nodes_by_block
.get(node.1)
.map(|nodes| {
nodes.iter().map(|node| format!("{}", node.kind().debug_name())).join("\n")
})
.unwrap_or_default()
)
semantic.semantic.cfg().basic_blocks[*node.1]
.debug_dot(semantic.semantic.nodes().into())
.trim()
)
}
)
);
std::fs::write(dot_file_path, cfg_dot_diagram)?;
Expand Down
Loading