From 3d4c5f3a20ed4d61c10a8dea844cf3708f67313c Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Wed, 19 Mar 2025 01:07:07 +0000 Subject: [PATCH] fix(semantic): correctly visit `IfStmt` `test` when building cfg (#9864) This PR fixes the construction of the cfg within `oxc_semantic`, by marking the `test` of `IfStatement` as unconditionally visited. Given the following: ```ts if (foo) { bar(); } else { baz(); } ``` Would produce the following graph: ```mermaid graph TD 0["bb0"] 1["bb1 IfStatement"] 2["bb2 Condition(IdentifierReference(foo))"] 3["bb3 BlockStatement ExpressionStatement"] 4["bb4 BlockStatement ExpressionStatement"] 5["bb5"] 1 -- "Error(Implicit)" --> 0 2 -- "Error(Implicit)" --> 0 3 -- "Error(Implicit)" --> 0 4 -- "Error(Implicit)" --> 0 5 -- "Error(Implicit)" --> 0 1 -- "Normal" --> 2 1 -- "Normal" --> 4 3 -- "Normal" --> 5 2 -- "Jump" --> 3 4 -- "Normal" --> 5 ``` After this change, it produces the following graph: ```mermaid graph TD 0["bb0"] 1["bb1 IfStatement"] 2["bb2 Condition(IdentifierReference(foo))"] 3["bb3 BlockStatement ExpressionStatement"] 4["bb4 BlockStatement ExpressionStatement"] 5["bb5"] 1 -- "Error(Implicit)" --> 0 2 -- "Error(Implicit)" --> 0 3 -- "Error(Implicit)" --> 0 4 -- "Error(Implicit)" --> 0 5 -- "Error(Implicit)" --> 0 1 -- "Normal" --> 2 3 -- "Normal" --> 5 2 -- "Jump" --> 3 2 -- "Jump" --> 4 4 -- "Normal" --> 5 ``` Rather than jumping from the if statment (`bb1\nIfStatement`) directly to the consequent (`bb4 BlockStatement ExpressionStatement`), it now unconditionally visits the `test` of the if statement. This can be seen in the after graph as the jump to `bb4 BlockStatement ExpressionStatement"` comes **from** `bb2 Condition(IdentifierReference(foo))` rather than from `bb1 IfStatement`. As a result of this change, `rules_of_hooks`, does not have false positives when a hook is in a position such as: ```ts if (useMe) { ``` As the cfg would previously think that `useMe` was called conditionally when in fact it was not fixes https://github.com/oxc-project/oxc/issues/9795 related #9807 --- .../src/rules/react/rules_of_hooks.rs | 88 +++++++++++++++++++ .../src/snapshots/react_rules_of_hooks.snap | 40 +++++++++ crates/oxc_semantic/src/builder.rs | 12 +-- .../tests/integration/snapshots/if_else.snap | 4 +- .../snapshots/if_stmt_in_for_in.snap | 8 +- .../snapshots/labeled_block_break.snap | 4 +- .../logical_expressions_short_circuit.snap | 12 +-- 7 files changed, 146 insertions(+), 22 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs index 1f74ff95f0633..8041150d8cb28 100644 --- a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs +++ b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs @@ -444,6 +444,62 @@ fn test() { useHook(); } ", + + + // Valid because hooks can be used in condition expressions beginning with a ternary condition + " + function ComponentWithConditionalHook() { + if (useHook() ? good() : bad()) { + check(); + } + } + ", + + // Valid because hooks can be used in condition expressions + " + function Component() { + if (!useHasPermission()) { + return null; + } + return ; + } + ", + // Valid because hooks can be used in ternary condition expressions + " + function Component() { + return useHasPermission() ? : null; + } + ", + // Valid because hooks can be used in logical expressions (left side) + " + function Component() { + return useHasPermission() && ; + } + ", + // Valid because hooks can be used with negation in condition expressions + " + function Component() { + if (!useHasPermission()) { + return null; + } + return ; + } + ", + // Valid because hooks can be used in complex condition expressions + " + function Component() { + if (useHasPermission() && isAdmin()) { + return ; + } + return ; + } + ", + // Valid because hooks can be used in nested condition expressions + " + function Component() { + return (useHasPermission() && isAdmin()) ? : ; + } + ", // Valid because components can use hooks. " function createComponentWithHook() { @@ -968,6 +1024,38 @@ fn test() { } } ", + // Invalid because hooks are used in the right side of logical expressions + " + function useHook() { + a && useHook1(); + b && useHook2(); + } + ", + // Invalid because hooks are used conditionally after a condition + " + function Component() { + if (condition) { + // This is invalid because the hook is called conditionally + useHook(); + } + } + ", + // Invalid because hooks are used in the right side of ternary expressions + " + function Component() { + condition ? useHook() : null; + } + ", + " + function Component() { + if (Math.random()) { + return null; + } else if (!useHasPermission()) { + return + } + return ; + } + ", // Invalid because hooks can only be called inside of a component. // errors: [ // topLevelError('Hook.useState'), diff --git a/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap b/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap index de971c9841845..015062a5f6117 100644 --- a/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap +++ b/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap @@ -9,6 +9,46 @@ source: crates/oxc_linter/src/tester.rs 5 │ } ╰──── + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useHook1" is called conditionally. React Hooks must be called in the exact same order in every component render. + ╭─[rules_of_hooks.tsx:3:22] + 2 │ function useHook() { + 3 │ a && useHook1(); + · ────────── + 4 │ b && useHook2(); + ╰──── + + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useHook2" is called conditionally. React Hooks must be called in the exact same order in every component render. + ╭─[rules_of_hooks.tsx:4:22] + 3 │ a && useHook1(); + 4 │ b && useHook2(); + · ────────── + 5 │ } + ╰──── + + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useHook" is called conditionally. React Hooks must be called in the exact same order in every component render. + ╭─[rules_of_hooks.tsx:5:21] + 4 │ // This is invalid because the hook is called conditionally + 5 │ useHook(); + · ───────── + 6 │ } + ╰──── + + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useHook" is called conditionally. React Hooks must be called in the exact same order in every component render. + ╭─[rules_of_hooks.tsx:3:29] + 2 │ function Component() { + 3 │ condition ? useHook() : null; + · ───────── + 4 │ } + ╰──── + + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useHasPermission" is called conditionally. React Hooks must be called in the exact same order in every component render. + ╭─[rules_of_hooks.tsx:5:23] + 4 │ return null; + 5 │ } else if (!useHasPermission()) { + · ────────────────── + 6 │ return + ╰──── + ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useState" cannot be called at the top level. React Hooks must be called in a React function component or a custom React Hook function. ╭─[rules_of_hooks.tsx:2:13] 1 │ diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index bb9ec3db8fc14..8e8f60cd59168 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -1183,21 +1183,17 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> { cfg.add_edge(before_if_stmt_graph_ix, start_of_condition_graph_ix, EdgeType::Normal); - cfg.add_edge(after_consequent_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal); - cfg.add_edge(after_test_graph_ix, before_consequent_stmt_graph_ix, EdgeType::Jump); + cfg.add_edge(after_consequent_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal); + if let Some((start_of_alternate_stmt_graph_ix, after_alternate_stmt_graph_ix)) = else_graph_ix { - cfg.add_edge( - before_if_stmt_graph_ix, - start_of_alternate_stmt_graph_ix, - EdgeType::Normal, - ); + cfg.add_edge(after_test_graph_ix, start_of_alternate_stmt_graph_ix, EdgeType::Jump); cfg.add_edge(after_alternate_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal); } else { - cfg.add_edge(before_if_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal); + cfg.add_edge(after_test_graph_ix, after_if_graph_ix, EdgeType::Jump); } }); /* cfg */ diff --git a/crates/oxc_semantic/tests/integration/snapshots/if_else.snap b/crates/oxc_semantic/tests/integration/snapshots/if_else.snap index e2a961ede9ce5..6d2504ae6fc4e 100644 --- a/crates/oxc_semantic/tests/integration/snapshots/if_else.snap +++ b/crates/oxc_semantic/tests/integration/snapshots/if_else.snap @@ -103,9 +103,9 @@ unreachable" shape = box] 9 -> 10 [ label="Unreachable", style="dotted"] 11 -> 2 [ label="Error(Implicit)", style=dashed, color=red] 3 -> 4 [ label="Normal"] - 8 -> 11 [ label="Normal", style="dotted"] 6 -> 7 [ label="Jump", color=green] - 3 -> 9 [ label="Normal"] + 8 -> 11 [ label="Normal", style="dotted"] + 6 -> 9 [ label="Jump", color=green] 10 -> 11 [ label="Normal", style="dotted"] 12 -> 2 [ label="Error(Implicit)", style=dashed, color=red] 11 -> 12 [ label="Unreachable", style="dotted"] diff --git a/crates/oxc_semantic/tests/integration/snapshots/if_stmt_in_for_in.snap b/crates/oxc_semantic/tests/integration/snapshots/if_stmt_in_for_in.snap index 08c6daa1cd22e..50e9298673fba 100644 --- a/crates/oxc_semantic/tests/integration/snapshots/if_stmt_in_for_in.snap +++ b/crates/oxc_semantic/tests/integration/snapshots/if_stmt_in_for_in.snap @@ -139,14 +139,14 @@ return" shape = box] 12 -> 13 [ label="Unreachable", style="dotted"] 14 -> 2 [ label="Error(Implicit)", color=red, style=dashed] 10 -> 11 [ label="Normal"] - 13 -> 14 [ label="Normal", style="dotted"] 11 -> 12 [ label="Jump", color=green] - 10 -> 14 [ label="Normal"] + 13 -> 14 [ label="Normal", style="dotted"] + 11 -> 14 [ label="Jump", color=green] 15 -> 2 [ label="Error(Implicit)", color=red, style=dashed] 6 -> 7 [ label="Normal"] - 9 -> 15 [ label="Normal", style="dotted"] 7 -> 8 [ label="Jump", color=green] - 6 -> 10 [ label="Normal"] + 9 -> 15 [ label="Normal", style="dotted"] + 7 -> 10 [ label="Jump", color=green] 14 -> 15 [ label="Normal"] 16 -> 2 [ label="Error(Implicit)", color=red, style=dashed] 3 -> 4 [ label="Normal"] diff --git a/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap b/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap index a2232bd5551de..1ec5323500ad7 100644 --- a/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap +++ b/crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap @@ -83,9 +83,9 @@ unreachable" shape = box] 6 -> 7 [ label="Unreachable", style="dotted"] 8 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 4 -> 5 [ label="Normal"] - 7 -> 8 [ label="Normal", style="dotted"] 5 -> 6 [ label="Jump", color=green] - 4 -> 8 [ label="Normal"] + 7 -> 8 [ label="Normal", style="dotted"] + 5 -> 8 [ label="Jump", color=green] 9 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 8 -> 9 [ label="Normal"] 6 -> 9 [ label="Jump", color=green] diff --git a/crates/oxc_semantic/tests/integration/snapshots/logical_expressions_short_circuit.snap b/crates/oxc_semantic/tests/integration/snapshots/logical_expressions_short_circuit.snap index 0995d38c64adc..76ac964c991d0 100644 --- a/crates/oxc_semantic/tests/integration/snapshots/logical_expressions_short_circuit.snap +++ b/crates/oxc_semantic/tests/integration/snapshots/logical_expressions_short_circuit.snap @@ -110,9 +110,9 @@ ExpressionStatement" shape = box] 5 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 6 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 1 -> 2 [ label="Normal"] - 5 -> 6 [ label="Normal"] 4 -> 5 [ label="Jump", color=green] - 1 -> 6 [ label="Normal"] + 5 -> 6 [ label="Normal"] + 4 -> 6 [ label="Jump", color=green] 7 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 8 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 9 -> 0 [ label="Error(Implicit)", color=red, style=dashed] @@ -122,9 +122,9 @@ ExpressionStatement" shape = box] 10 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 11 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 6 -> 7 [ label="Normal"] - 10 -> 11 [ label="Normal"] 9 -> 10 [ label="Jump", color=green] - 6 -> 11 [ label="Normal"] + 10 -> 11 [ label="Normal"] + 9 -> 11 [ label="Jump", color=green] 12 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 13 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 14 -> 0 [ label="Error(Implicit)", color=red, style=dashed] @@ -134,7 +134,7 @@ ExpressionStatement" shape = box] 15 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 16 -> 0 [ label="Error(Implicit)", color=red, style=dashed] 11 -> 12 [ label="Normal"] - 15 -> 16 [ label="Normal"] 14 -> 15 [ label="Jump", color=green] - 11 -> 16 [ label="Normal"] + 15 -> 16 [ label="Normal"] + 14 -> 16 [ label="Jump", color=green] }