Skip to content
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

refactor(semantic): make control flow generation optional. #3737

Merged
merged 1 commit into from
Jun 18, 2024
Merged

refactor(semantic): make control flow generation optional. #3737

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: 4 additions & 0 deletions crates/oxc_cfg/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ pub struct ControlFlowGraph {
}

impl ControlFlowGraph {
pub fn graph(&self) -> &Graph<usize, EdgeType> {
&self.graph
}

/// # Panics
pub fn basic_block(&self, id: BasicBlockId) -> &BasicBlock {
let ix = *self.graph.node_weight(id).expect("expected a valid node id in self.graph");
Expand Down
21 changes: 14 additions & 7 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 @@ -8,7 +8,7 @@ use oxc_ast::{
use oxc_diagnostics::OxcDiagnostic;

use oxc_cfg::{
graph::visit::neighbors_filtered_by_edge_weight, EdgeType, InstructionKind,
graph::visit::neighbors_filtered_by_edge_weight, ControlFlowGraph, EdgeType, InstructionKind,
ReturnInstructionKind,
};
use oxc_macros::declare_oxc_lint;
Expand Down Expand Up @@ -55,16 +55,19 @@ declare_oxc_lint!(

impl Rule for GetterReturn {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be let cfg = ctx.semantic().cfg().unwrap()? i.e. Panic if linter has been passed a Semantic instance which doesn't have CFG.

It's not ideal to have runtime panics, but on the other hand, if there's some mistake and linter gets passed a Semantic with no CFG, then these lint rules will just silently fail to run, and user will be told "Lint successful! No problem with your code." regardless of whether that's true or not.

This is why my preference was for a Semantic<CfgType> generic type - then this would be enforced at compile time.

I'm not sure. There's arguments on both sides. Maybe a compromise would be:

debug_assert!(ctx.semantic().cfg().is_some());
let Some(cfg) = ctx.semantic().cfg() else { return };

I think this at least communicates to someone reading the code that cfg must be Some(_), and that it's not actually optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentional, I was thinking that we can make the linter to omit CFG generation and disable the rules depending on it. Or the other way around if we have no rule dependent on CFG enabled we can disable the CFG generation altogether.
But we can go either way since it has no difference at the moment. I had no objection against unwrap/panics here.

Copy link
Contributor

@overlookmotel overlookmotel Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have no rule dependent on CFG enabled we can disable the CFG generation altogether.

That's a nice optimization. I think that way around is the better option, in which case it could be unwrap here.

I have a broader feeling that we need a convention for communicating in code when (1) something is actually optional vs (2) there's an unstated invariant that means this branch must always go one way. When delving into parts of Oxc codebase which I'm new to, this ambiguity has left me scratching my head quite a few times, wondering "in what circumstances can this happen?", and it turns out the answer is "it can't".

unwrap/unreachable! is one way to communicate that, but it has the downside of bloating the ASM with panic code for the unreachable branch. So I don't have a good answer to this.

Anyway, it's a broader problem, so I think we can just leave it as is in this PR, since I was too late with my review.


// https://eslint.org/docs/latest/rules/getter-return#handled_by_typescript
if ctx.source_type().is_typescript() {
return;
}
match node.kind() {
AstKind::Function(func) if !func.is_typescript_syntax() => {
self.run_diagnostic(node, ctx, func.span);
self.run_diagnostic(node, ctx, cfg, func.span);
}
AstKind::ArrowFunctionExpression(expr) => {
self.run_diagnostic(node, ctx, expr.span);
self.run_diagnostic(node, ctx, cfg, expr.span);
}
_ => {}
}
Expand Down Expand Up @@ -178,15 +181,19 @@ impl GetterReturn {
false
}

fn run_diagnostic<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>, span: Span) {
fn run_diagnostic<'a>(
&self,
node: &AstNode<'a>,
ctx: &LintContext<'a>,
cfg: &ControlFlowGraph,
span: Span,
) {
if !Self::is_wanted_node(node, ctx) {
return;
}

let cfg = ctx.semantic().cfg();

let output = neighbors_filtered_by_edge_weight(
&cfg.graph,
cfg.graph(),
node.cfg_id(),
&|edge| match edge {
EdgeType::Jump | EdgeType::Normal => None,
Expand Down
14 changes: 8 additions & 6 deletions crates/oxc_linter/src/rules/eslint/no_fallthrough.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use oxc_cfg::{
visit::{neighbors_filtered_by_edge_weight, EdgeRef},
Direction,
},
BasicBlockId, EdgeType, ErrorEdgeKind, InstructionKind,
BasicBlockId, ControlFlowGraph, EdgeType, ErrorEdgeKind, InstructionKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
Expand Down Expand Up @@ -89,13 +89,15 @@ impl Rule for NoFallthrough {
}

fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };

let AstKind::SwitchStatement(switch) = node.kind() else { return };

let switch_id = node.cfg_id();
let cfg = ctx.semantic().cfg();
let graph = &cfg.graph;
let graph = cfg.graph();

let (cfg_ids, tests, default, exit) = get_switch_semantic_cases(ctx, node, switch);
let (cfg_ids, tests, default, exit) = get_switch_semantic_cases(ctx, cfg, node, switch);

let Some(default_or_exit) = default.or(exit) else {
// TODO: our `get_switch_semantic_cases` can't evaluate cfg_ids for switch statements
Expand Down Expand Up @@ -260,6 +262,7 @@ impl NoFallthrough {
// Issue: <https://github.com/oxc-project/oxc/issues/3662>
fn get_switch_semantic_cases(
ctx: &LintContext,
cfg: &ControlFlowGraph,
node: &AstNode,
switch: &SwitchStatement,
) -> (
Expand All @@ -268,8 +271,7 @@ fn get_switch_semantic_cases(
/* default */ Option<BasicBlockId>,
/* exit */ Option<BasicBlockId>,
) {
let cfg = &ctx.semantic().cfg();
let graph = &cfg.graph;
let graph = cfg.graph();
let has_default = switch.cases.iter().any(SwitchCase::is_default_case);
let (tests, exit) = graph
.edges_directed(node.cfg_id(), Direction::Outgoing)
Expand Down
3 changes: 2 additions & 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 @@ -58,7 +58,8 @@ enum DefinitelyCallsThisBeforeSuper {
impl Rule for NoThisBeforeSuper {
fn run_once(&self, ctx: &LintContext) {
let semantic = ctx.semantic();
let cfg = semantic.cfg();
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };

// first pass -> find super calls and local violations
let mut wanted_nodes = Vec::new();
Expand Down
6 changes: 4 additions & 2 deletions crates/oxc_linter/src/rules/eslint/no_unreachable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ declare_oxc_lint!(

impl Rule for NoUnreachable {
fn run_once(&self, ctx: &LintContext) {
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };

let nodes = ctx.nodes();
let Some(root) = nodes.root_node() else { return };
let cfg = ctx.semantic().cfg();
let graph = &cfg.graph;
let graph = cfg.graph();

// A pre-allocated vector containing the reachability status of all the basic blocks.
// We initialize this vector with all nodes set to `unreachable` since if we don't visit a
Expand Down
14 changes: 8 additions & 6 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 @@ -2,8 +2,8 @@ use oxc_ast::{ast::Expression, AstKind};
use oxc_diagnostics::OxcDiagnostic;

use oxc_cfg::{
graph::visit::neighbors_filtered_by_edge_weight, EdgeType, Instruction, InstructionKind,
ReturnInstructionKind,
graph::visit::neighbors_filtered_by_edge_weight, ControlFlowGraph, EdgeType, Instruction,
InstructionKind, ReturnInstructionKind,
};
use oxc_macros::declare_oxc_lint;
use oxc_span::{GetSpan, Span};
Expand Down Expand Up @@ -51,6 +51,9 @@ declare_oxc_lint!(

impl Rule for RequireRenderReturn {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };

if !matches!(node.kind(), AstKind::ArrowFunctionExpression(_) | AstKind::Function(_)) {
return;
}
Expand All @@ -64,7 +67,7 @@ impl Rule for RequireRenderReturn {
return;
}

if !contains_return_statement(node, ctx) {
if !contains_return_statement(node, cfg) {
match parent.kind() {
AstKind::MethodDefinition(method) => {
ctx.diagnostic(require_render_return_diagnostic(method.key.span()));
Expand All @@ -91,10 +94,9 @@ enum FoundReturn {
const KEEP_WALKING_ON_THIS_PATH: bool = true;
const STOP_WALKING_ON_THIS_PATH: bool = false;

fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool {
let cfg = ctx.semantic().cfg();
fn contains_return_statement(node: &AstNode, cfg: &ControlFlowGraph) -> bool {
let state = neighbors_filtered_by_edge_weight(
&cfg.graph,
cfg.graph(),
node.cfg_id(),
&|edge| match edge {
// We only care about normal edges having a return statement.
Expand Down
16 changes: 9 additions & 7 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 @@ -4,7 +4,7 @@ use oxc_ast::{
};
use oxc_cfg::{
graph::{algo, visit::Control},
EdgeType, ErrorEdgeKind, InstructionKind,
ControlFlowGraph, EdgeType, ErrorEdgeKind, InstructionKind,
};
use oxc_macros::declare_oxc_lint;
use oxc_semantic::{AstNodeId, AstNodes};
Expand Down Expand Up @@ -107,6 +107,9 @@ declare_oxc_lint!(

impl Rule for RulesOfHooks {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
// control flow dependant
let Some(cfg) = ctx.semantic().cfg() else { return };

let AstKind::CallExpression(call) = node.kind() else { return };

if !is_react_hook(&call.callee) {
Expand Down Expand Up @@ -227,7 +230,7 @@ impl Rule for RulesOfHooks {
return;
}

if !ctx.semantic().cfg().is_reachable(func_cfg_id, node_cfg_id) {
if !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 All @@ -236,26 +239,25 @@ impl Rule for RulesOfHooks {
}

// Is this node cyclic?
if semantic.cfg().is_cyclic(node_cfg_id) {
if cfg.is_cyclic(node_cfg_id) {
return ctx.diagnostic(diagnostics::loop_hook(span, hook_name));
}

if has_conditional_path_accept_throw(ctx, parent_func, node) {
if has_conditional_path_accept_throw(cfg, parent_func, node) {
#[allow(clippy::needless_return)]
return ctx.diagnostic(diagnostics::conditional_hook(span, hook_name));
}
}
}

fn has_conditional_path_accept_throw(
ctx: &LintContext<'_>,
cfg: &ControlFlowGraph,
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;
let graph = cfg.graph();
if graph
.edges(to_graph_id)
.any(|it| matches!(it.weight(), EdgeType::Error(ErrorEdgeKind::Explicit)))
Expand Down
20 changes: 11 additions & 9 deletions crates/oxc_semantic/examples/cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ fn main() -> std::io::Result<()> {
let semantic = SemanticBuilder::new(&source_text, source_type)
.with_check_syntax_error(true)
.with_trivias(ret.trivias)
.with_cfg(true)
.build(program);

if !semantic.errors.is_empty() {
Expand All @@ -59,16 +60,19 @@ fn main() -> std::io::Result<()> {
return Ok(());
}

let cfg = semantic
.semantic
.cfg()
.expect("we set semantic to build the control flow (`with_cfg`) for us so it should always be `Some`");

let mut ast_nodes_by_block = HashMap::<_, Vec<_>>::new();
for node in semantic.semantic.nodes().iter() {
let block = node.cfg_id();
let block_ix = semantic.semantic.cfg().graph.node_weight(block).unwrap();
let block_ix = cfg.graph.node_weight(block).unwrap();
ast_nodes_by_block.entry(*block_ix).or_default().push(node);
}

let basic_blocks_printed = semantic
.semantic
.cfg()
let basic_blocks_printed = cfg
.basic_blocks
.iter()
.map(DisplayDot::display_dot)
Expand All @@ -93,13 +97,13 @@ fn main() -> std::io::Result<()> {
let cfg_dot_diagram = format!(
"{:?}",
Dot::with_attr_getters(
&semantic.semantic.cfg().graph,
cfg.graph(),
&[Config::EdgeNoLabel, Config::NodeNoLabel],
&|_graph, edge| {
let weight = edge.weight();
let label = format!("label = \"{weight:?}\"");
if matches!(weight, EdgeType::Unreachable)
|| semantic.semantic.cfg().basic_block(edge.source()).unreachable
|| cfg.basic_block(edge.source()).unreachable
{
format!("{label}, style = \"dotted\" ")
} else {
Expand All @@ -124,9 +128,7 @@ fn main() -> std::io::Result<()> {
node.1,
nodes,
node.1,
semantic.semantic.cfg().basic_blocks[*node.1]
.debug_dot(semantic.semantic.nodes().into())
.trim()
cfg.basic_blocks[*node.1].debug_dot(semantic.semantic.nodes().into()).trim()
)
}
)
Expand Down
Loading
Loading