8000 fix(semantic): correctly visit `IfStmt` `test` when building cfg (#9864) · oxc-project/oxc@3d4c5f3 · GitHub
[go: up one dir, main page]
Skip to content

Commit 3d4c5f3

Browse files
camc314camc314
committed
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 #9795 related #9807
1 parent 544a090 commit 3d4c5f3

File tree

7 files changed

+146
-22
lines changed
  • crates

7 files changed

+146
-22
lines changed

‎crates/oxc_linter/src/rules/react/rules_of_hooks.rs

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,62 @@ fn test() {
444444
useHook();
445445
}
446446
",
447+
448+
449+
// Valid because hooks can be used in condition expressions beginning with a ternary condition
450+
"
451+
function ComponentWithConditionalHook() {
452+
if (useHook() ? good() : bad()) {
453+
check();
454+
}
455+
}
456+
",
457+
458+
// Valid because hooks can be used in condition expressions
459+
"
460+
function Component() {
461+
if (!useHasPermission()) {
462+
return null;
463+
}
464+
return <Content />;
465+
}
466+
",
467+
// Valid because hooks can be used in ternary condition expressions
468+
"
469+
function Component() {
470+
return useHasPermission() ? <Content /> : null;
471+
}
472+
",
473+
// Valid because hooks can be used in logical expressions (left side)
474+
"
475+
function Component() {
476+
return useHasPermission() && <Content />;
477+
}
478+
",
479+
// Valid because hooks can be used with negation in condition expressions
480+
"
481+
function Component() {
482+
if (!useHasPermission()) {
483+
return null;
484+
}
485+
return <Content />;
486+
}
487+
",
488+
// Valid because hooks can be used in complex condition expressions
489+
"
490+
function Component() {
491+
if (useHasPermission() && isAdmin()) {
492+
return <AdminContent />;
493+
}
494+
return <Content />;
495+
}
496+
",
497+
// Valid because hooks can be used in nested condition expressions
498+
"
499+
function Component() {
500+
return (useHasPermission() && isAdmin()) ? <AdminContent /> : <Content />;
501+
}
502+
",
447503
// Valid because components can use hooks.
448504
"
449505
function createComponentWithHook() {
@@ -968,6 +1024,38 @@ fn test() {
9681024
}
9691025
}
9701026
",
1027+
// Invalid because hooks are used in the right side of logical expressions
1028+
"
1029+
function useHook() {
1030+
a && useHook1();
1031+
b && useHook2();
1032+
}
1033+
",
1034+
// Invalid because hooks are used conditionally after a condition
1035+
"
1036+
function Component() {
1037+
if (condition) {
1038+
// This is invalid because the hook is called conditionally
1039+
useHook();
1040+
}
1041+
}
1042+
",
1043+
// Invalid because hooks are used in the right side of ternary expressions
1044+
"
1045+
function Component() {
1046+
condition ? useHook() : null;
1047+
}
1048+
",
1049+
"
1050+
function Component() {
1051+
if (Math.random()) {
1052+
return null;
1053+
} else if (!useHasPermission()) {
1054+
return <Foo />
1055+
}
1056+
return <Content />;
1057+
}
1058+
",
9711059
// Invalid because hooks can only be called inside of a component.
9721060
// errors: [
9731061
// topLevelError('Hook.useState'),

‎crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,46 @@ source: crates/oxc_linter/src/tester.rs
99
5 │ }
1010
╰────
1111

12+
⚠ 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.
13+
╭─[rules_of_hooks.tsx:3:22]
14+
2 │ function useHook() {
15+
3 │ a && useHook1();
16+
· ──────────
17+
4 │ b && useHook2();
18+
╰────
19+
20+
⚠ 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.
21+
╭─[rules_of_hooks.tsx:4:22]
22+
3 │ a && useHook1();
23+
4 │ b && useHook2();
24+
· ──────────
25+
5 │ }
26+
╰────
27+
28+
⚠ 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.
29+
╭─[rules_of_hooks.tsx:5:21]
30+
4 │ // This is invalid because the hook is called conditionally
31+
5 │ useHook();
32+
· ─────────
33+
6 │ }
34+
╰────
35+
36+
⚠ 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.
37+
╭─[rules_of_hooks.tsx:3:29]
38+
2 │ function Component() {
39+
3 │ condition ? useHook() : null;
40+
· ─────────
41+
4 │ }
42+
╰────
43+
44+
⚠ 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.
45+
╭─[rules_of_hooks.tsx:5:23]
46+
4 │ return null;
47+
5 │ } else if (!useHasPermission()) {
48+
· ──────────────────
49+
6 │ return <Foo />
50+
╰────
51+
1252
⚠ 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.
1353
╭─[rules_of_hooks.tsx:2:13]
1454
1 │

‎crates/oxc_semantic/src/builder.rs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,21 +1183,17 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
11831183

11841184
cfg.add_edge(before_if_stmt_graph_ix, start_of_condition_graph_ix, EdgeType::Normal);
11851185

1186-
cfg.add_edge(after_consequent_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal);
1187-
11881186
cfg.add_edge(after_test_graph_ix, before_consequent_stmt_graph_ix, EdgeType::Jump);
11891187

1188+
cfg.add_edge(after_consequent_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal);
1189+
11901190
if let Some((start_of_alternate_stmt_graph_ix, after_alternate_stmt_graph_ix)) =
11911191
else_graph_ix
11921192
{
1193-
cfg.add_edge(
1194-
before_if_stmt_graph_ix,
1195-
start_of_alternate_stmt_graph_ix,
1196-
EdgeType::Normal,
1197-
);
1193+
cfg.add_edge(after_test_graph_ix, start_of_alternate_stmt_graph_ix, EdgeType::Jump);
11981194
cfg.add_edge(after_alternate_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal);
11991195
} else {
1200-
cfg.add_edge(before_if_stmt_graph_ix, after_if_graph_ix, EdgeType::Normal);
1196+
cfg.add_edge(after_test_graph_ix, after_if_graph_ix, EdgeType::Jump);
12011197
}
12021198
});
12031199
/* cfg */

‎crates/oxc_semantic/tests/integration/snapshots/if_else.snap

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,9 @@ unreachable" shape = box]
103103
9 -> 10 [ label="Unreachable", style="dotted"]
104104
11 -> 2 [ label="Error(Implicit)", style=dashed, color=red]
105105
3 -> 4 [ label="Normal"]
106-
8 -> 11 [ label="Normal", style="dotted"]
107106
6 -> 7 [ label="Jump", color=green]
108-
3 -> 9 [ label="Normal"]
107+
8 -> 11 [ label="Normal", style="dotted"]
108+
6 -> 9 [ label="Jump", color=green]
109109
10 -> 11 [ label="Normal", style="dotted"]
110110
12 -> 2 [ label="Error(Implicit)", style=dashed, color=red]
111111
11 -> 12 [ label="Unreachable", style="dotted"]

‎crates/oxc_semantic/tests/integration/snapshots/if_stmt_in_for_in.snap

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,14 +139,14 @@ return" shape = box]
139139
12 -> 13 [ label="Unreachable", style="dotted"]
140140
14 -> 2 [ label="Error(Implicit)", color=red, style=dashed]
141141
10 -> 11 [ label="Normal"]
142-
13 -> 14 [ label="Normal", style="dotted"]
143142
11 -> 12 [ label="Jump", color=green]
144-
10 -> 14 [ label="Normal"]
143+
13 -> 14 [ label="Normal", style="dotted"]
144+
11 -> 14 [ label="Jump", color=green]
145145
15 -> 2 [ label="Error(Implicit)", color=red, style=dashed]
146146
6 -> 7 [ label="Normal"]
147-
9 -> 15 [ label="Normal", style="dotted"]
148147
7 -> 8 [ label="Jump", color=green]
149-
6 -> 10 [ label="Normal"]
148+
9 -> 15 [ label="Normal", style="dotted"]
149+
7 -> 10 [ label="Jump", color=green]
150150
14 -> 15 [ label="Normal"]
151151
16 -> 2 [ label="Error(Implicit)", color=red, style=dashed]
152152
3 -> 4 [ label="Normal"]

‎crates/oxc_semantic/tests/integration/snapshots/labeled_block_break.snap

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,9 @@ unreachable" shape = box]
8383
6 -> 7 [ label="Unreachable", style="dotted"]
8484
8 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
8585
4 -> 5 [ label="Normal"]
86-
7 -> 8 [ label="Normal", style="dotted"]
8786
5 -> 6 [ label="Jump", color=green]
88-
4 -> 8 [ label="Normal"]
87+
7 -> 8 [ label="Normal", style="dotted"]
88+
5 -> 8 [ label="Jump", color=green]
8989
9 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
9090
8 -> 9 [ label="Normal"]
9191
6 -> 9 [ label="Jump", color=green]

‎crates/oxc_semantic/tests/integration/snapshots/logical_expressions_short_circuit.snap

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,9 +110,9 @@ ExpressionStatement" shape = box]
110110
5 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
111111
6 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
112112
1 -> 2 [ label="Normal"]
113-
5 -> 6 [ label="Normal"]
114113
4 -> 5 [ label="Jump", color=green]
115-
1 -> 6 [ label="Normal"]
114+
5 -> 6 [ label="Normal"]
115+
4 -> 6 [ label="Jump", color=green]
116116
7 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
117117
8 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
118118
9 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
@@ -122,9 +122,9 @@ ExpressionStatement" shape = box]
122122
10 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
123123
11 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
124124
6 -> 7 [ label="Normal"]
125-
10 -> 11 [ label="Normal"]
126125
9 -> 10 [ label="Jump", color=green]
127-
6 -> 11 [ label="Normal"]
126+
10 -> 11 [ label="Normal"]
127+
9 -> 11 [ label="Jump", color=green]
128128
12 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
129129
13 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
130130
14 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
@@ -134,7 +134,7 @@ ExpressionStatement" shape = box]
134134
15 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
135135
16 -> 0 [ label="Error(Implicit)", color=red, style=dashed]
136136
11 -> 12 [ label="Normal"]
137-
15 -> 16 [ label="Normal"]
138137
14 -> 15 [ label="Jump", color=green]
139-
11 -> 16 [ label="Normal"]
138+
15 -> 16 [ label="Normal"]
139+
14 -> 16 [ label="Jump", color=green]
140140
}

0 commit comments

Comments
 (0)
0