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

improvement(semantic/cfg): rework error path. #3519

Merged
merged 1 commit into from
Jun 13, 2024
Merged

improvement(semantic/cfg): rework error path. #3519

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
2 changes: 1 addition & 1 deletion crates/oxc_ast/src/ast_kind.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ impl<'a> AstKind<'a> {
Self::PrivateIdentifier(x) => format!("PrivateIdentifier({})", x.name).into(),

Self::NumericLiteral(n) => format!("NumericLiteral({})", n.value).into(),
Self::StringLiteral(s) => format!("NumericLiteral({})", s.value).into(),
Self::StringLiteral(s) => format!("StringLiteral({})", s.value).into(),
Self::BooleanLiteral(b) => format!("BooleanLiteral({})", b.value).into(),
Self::NullLiteral(_) => "NullLiteral".into(),
Self::BigintLiteral(b) => format!("BigintLiteral({})", b.raw).into(),
Expand Down
5 changes: 5 additions & 0 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 @@ -193,6 +193,11 @@ impl GetterReturn {
| EdgeType::NewFunction
// Unreachable nodes aren't reachable so we don't follow them.
| EdgeType::Unreachable
// TODO: For now we ignore the error path to simplify this rule, We can also
// analyze the error path as a nice to have addition.
| EdgeType::Error(_)
| EdgeType::Finalize
| EdgeType::Join
// By returning Some(X),
// we signal that we don't walk to this path any farther.
//
Expand Down
145 changes: 118 additions & 27 deletions 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 @@ -7,7 +7,10 @@ use oxc_ast::{
AstKind,
};
use oxc_macros::declare_oxc_lint;
use oxc_semantic::{pg::neighbors_filtered_by_edge_weight, AstNodeId, BasicBlockId, EdgeType};
use oxc_semantic::{
petgraph::visit::EdgeRef, pg::neighbors_filtered_by_edge_weight, AstNodeId, BasicBlockId,
ControlFlowGraph, EdgeType,
};
use oxc_span::{GetSpan, Span};

use crate::{context::LintContext, rule::Rule, AstNode};
Expand Down Expand Up @@ -48,6 +51,7 @@ enum DefinitelyCallsThisBeforeSuper {
#[default]
No,
Yes,
Maybe(BasicBlockId),
}

impl Rule for NoThisBeforeSuper {
Expand Down Expand Up @@ -99,35 +103,20 @@ impl Rule for NoThisBeforeSuper {
// second pass, walk cfg for wanted nodes and propagate
// cross-block super calls:
for node in wanted_nodes {
let output = neighbors_filtered_by_edge_weight(
&cfg.graph,
let output = Self::analyze(
cfg,
node.cfg_id(),
&|edge| match edge {
EdgeType::Jump | EdgeType::Normal => None,
EdgeType::Unreachable | EdgeType::Backedge | EdgeType::NewFunction => {
Some(DefinitelyCallsThisBeforeSuper::No)
}
},
&mut |basic_block_id, _| {
let super_called = basic_blocks_with_super_called.contains(basic_block_id);
if basic_blocks_with_local_violations.contains_key(basic_block_id) {
// super was not called before this in the current code path:
return (DefinitelyCallsThisBeforeSuper::Yes, false);
}

if super_called {
(DefinitelyCallsThisBeforeSuper::No, false)
} else {
(DefinitelyCallsThisBeforeSuper::No, true)
}
},
&basic_blocks_with_super_called,
&basic_blocks_with_local_violations,
false,
);

// Deciding whether we definitely call this before super in all
// codepaths is as simple as seeing if any individual codepath
// definitely calls this before super.
let violation_in_any_codepath =
output.into_iter().any(|y| matches!(y, DefinitelyCallsThisBeforeSuper::Yes));
let violation_in_any_codepath = Self::check_for_violation(
cfg,
output,
&basic_blocks_with_super_called,
&basic_blocks_with_local_violations,
);

// If not, flag it as a diagnostic.
if violation_in_any_codepath {
Expand Down Expand Up @@ -163,6 +152,108 @@ impl NoThisBeforeSuper {

false
}

fn analyze(
cfg: &ControlFlowGraph,
id: BasicBlockId,
basic_blocks_with_super_called: &HashSet<oxc_semantic::petgraph::prelude::NodeIndex>,
basic_blocks_with_local_violations: &HashMap<
oxc_semantic::petgraph::prelude::NodeIndex,
Vec<AstNodeId>,
>,
follow_join: bool,
) -> Vec<DefinitelyCallsThisBeforeSuper> {
neighbors_filtered_by_edge_weight(
&cfg.graph,
id,
&|edge| match edge {
EdgeType::Jump | EdgeType::Normal => None,
EdgeType::Join if follow_join => None,
EdgeType::Unreachable
| EdgeType::Join
| EdgeType::Error(_)
| EdgeType::Finalize
| EdgeType::Backedge
| EdgeType::NewFunction => Some(DefinitelyCallsThisBeforeSuper::No),
},
&mut |basic_block_id, _| {
let super_called = basic_blocks_with_super_called.contains(basic_block_id);
if basic_blocks_with_local_violations.contains_key(basic_block_id) {
// super was not called before this in the current code path:
return (DefinitelyCallsThisBeforeSuper::Yes, false);
}

if super_called {
// If super is called but we are in a try-catch(-finally) block mark it as a
// maybe, since we might throw on super call and still call this in
// `catch`/`finally` block(s).
if cfg.graph.edges(*basic_block_id).any(|it| {
matches!(
it.weight(),
EdgeType::Error(oxc_semantic::ErrorEdgeKind::Explicit)
| EdgeType::Finalize
)
}) {
(DefinitelyCallsThisBeforeSuper::Maybe(*basic_block_id), false)
// Otherwise we know for sure that super is called in this branch before
// reaching a this expression.
} else {
(DefinitelyCallsThisBeforeSuper::No, false)
}
// If we haven't visited a super call and we have a non-error/finalize path
// forward, continue visiting this branch.
} else if cfg
.graph
.edges(*basic_block_id)
.any(|it| !matches!(it.weight(), EdgeType::Error(_) | EdgeType::Finalize))
{
(DefinitelyCallsThisBeforeSuper::No, true)
// Otherwise we mark it as a `Maybe` so we can analyze error/finalize paths separately.
} else {
(DefinitelyCallsThisBeforeSuper::Maybe(*basic_block_id), false)
}
},
)
}

fn check_for_violation(
cfg: &ControlFlowGraph,
output: Vec<DefinitelyCallsThisBeforeSuper>,
basic_blocks_with_super_called: &HashSet<oxc_semantic::petgraph::prelude::NodeIndex>,
basic_blocks_with_local_violations: &HashMap<
oxc_semantic::petgraph::prelude::NodeIndex,
Vec<AstNodeId>,
>,
) -> bool {
// Deciding whether we definitely call this before super in all
// codepaths is as simple as seeing if any individual codepath
// definitely calls this before super.
output.into_iter().any(|y| match y {
DefinitelyCallsThisBeforeSuper::Yes => true,
DefinitelyCallsThisBeforeSuper::No => false,
DefinitelyCallsThisBeforeSuper::Maybe(id) => cfg.graph.edges(id).any(|edge| {
let weight = edge.weight();
let is_explicit_error =
matches!(weight, EdgeType::Error(oxc_semantic::ErrorEdgeKind::Explicit));
if is_explicit_error || matches!(weight, EdgeType::Finalize) {
Self::check_for_violation(
cfg,
Self::analyze(
cfg,
edge.target(),
basic_blocks_with_super_called,
basic_blocks_with_local_violations,
is_explicit_error,
),
basic_blocks_with_super_called,
basic_blocks_with_local_violations,
)
} else {
false
}
}),
})
}
}

#[test]
Expand Down
9 changes: 6 additions & 3 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 @@ -100,9 +100,12 @@ fn contains_return_statement<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> b
// We only care about normal edges having a return statement.
EdgeType::Jump | EdgeType::Normal => None,
// For these two type, we flag it as not found.
EdgeType::Unreachable | EdgeType::NewFunction | EdgeType::Backedge => {
Some(FoundReturn::No)
}
EdgeType::Unreachable
| EdgeType::Error(_)
| EdgeType::Finalize
| EdgeType::Join
| 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 Down
58 changes: 50 additions & 8 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_macros::declare_oxc_lint;
use oxc_semantic::{
algo, petgraph::visit::Control, AstNodeId, AstNodes, BasicBlockId, EdgeType, InstructionKind,
algo, petgraph::visit::Control, AstNodeId, AstNodes, EdgeType, ErrorEdgeKind, InstructionKind,
};
use oxc_span::{Atom, CompactStr};
use oxc_syntax::operator::AssignmentOperator;
Expand Down Expand Up @@ -238,7 +238,7 @@ impl Rule for RulesOfHooks {
return ctx.diagnostic(diagnostics::loop_hook(span, hook_name));
}

if has_conditional_path_accept_throw(ctx, func_cfg_id, node_cfg_id) {
if has_conditional_path_accept_throw(ctx, parent_func, node) {
#[allow(clippy::needless_return)]
return ctx.diagnostic(diagnostics::conditional_hook(span, hook_name));
}
Expand All @@ -247,20 +247,62 @@ impl Rule for RulesOfHooks {

fn has_conditional_path_accept_throw(
ctx: &LintContext<'_>,
from: BasicBlockId,
to: BasicBlockId,
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;
if graph
.edges(to_graph_id)
.any(|it| matches!(it.weight(), EdgeType::Error(ErrorEdgeKind::Explicit)))
{
// TODO: We are simplifying here, There is a real need for a trait like `MayThrow` that
// would provide a method `may_throw`, since not everything may throw and break the control flow.
return true;
// let paths = algo::all_simple_paths::<Vec<_>, _>(graph, from_graph_id, to_graph_id, 0, None);
// if paths
// .flatten()
// .flat_map(|id| cfg.basic_block(id).instructions())
// .filter_map(|it| match it {
// Instruction { kind: InstructionKind::Statement, node_id: Some(node_id) } => {
// let r = Some(nodes.get_node(*node_id));
// dbg!(&r);
// r
// }
// _ => None,
// })
// .filter(|it| it.id() != to.id())
// .any(|it| {
// // TODO: it.may_throw()
// matches!(
// it.kind(),
// AstKind::ExpressionStatement(ExpressionStatement {
// expression: Expression::CallExpression(_),
// ..
// })
// )
// })
// {
// // return true;
// }
}
// All nodes should be able to reach the hook node, Otherwise we have a conditional/branching flow.
algo::dijkstra(graph, from, Some(to), |e| match e.weight() {
EdgeType::NewFunction => 1,
EdgeType::Jump | EdgeType::Unreachable | EdgeType::Backedge | EdgeType::Normal => 0,
algo::dijkstra(graph, from_graph_id, Some(to_graph_id), |e| match e.weight() {
EdgeType::NewFunction | EdgeType::Error(ErrorEdgeKind::Implicit) => 1,
EdgeType::Error(ErrorEdgeKind::Explicit)
| EdgeType::Join
| EdgeType::Finalize
| EdgeType::Jump
| EdgeType::Unreachable
| EdgeType::Backedge
| EdgeType::Normal => 0,
})
.into_iter()
.filter(|(_, val)| *val == 0)
.any(|(f, _)| {
!cfg.is_reachabale_filtered(f, to, |it| {
!cfg.is_reachabale_filtered(f, to_graph_id, |it| {
if cfg
.basic_block(it)
.instructions()
Expand Down
36 changes: 0 additions & 36 deletions crates/oxc_linter/src/snapshots/empty_brace_spaces.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,6 @@ expression: empty_brace_spaces
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:29]
1 │ try {} catch(foo){} finally { }
· ───
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:4]
1 │ do { } while (foo)
Expand Down Expand Up @@ -268,13 +261,6 @@ expression: empty_brace_spaces
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:29]
1 │ try {} catch(foo){} finally { }
· ─────
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:4]
1 │ do { } while (foo)
Expand Down Expand Up @@ -450,13 +436,6 @@ expression: empty_brace_spaces
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:29]
1 │ try {} catch(foo){} finally { }
· ──────────
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:4]
1 │ do { } while (foo)
Expand Down Expand Up @@ -638,14 +617,6 @@ expression: empty_brace_spaces
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:29]
1 │ ╭─▶ try {} catch(foo){} finally {
2 │ │
3 │ ╰─▶ }
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:4]
1 │ ╭─▶ do {
Expand Down Expand Up @@ -825,13 +796,6 @@ expression: empty_brace_spaces
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:29]
1 │ ╭─▶ try {} catch(foo){} finally {
2 │ ╰─▶ }
╰────
help: There should be no spaces or new lines inside a pair of empty braces as it affects the overall readability of the code.

⚠ eslint-plugin-unicorn(empty-brace-spaces): No spaces inside empty pair of braces allowed
╭─[empty_brace_spaces.tsx:1:4]
1 │ ╭─▶ do {
Expand Down
Loading