fix(linter): react rules of hooks false positive#9807
fix(linter): react rules of hooks false positive#9807hulin32 wants to merge 4 commits intooxc-project:mainfrom
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
hulin32
changed the title
CodSpeed Performance ReportMerging #9807 will not alter performanceComparing Summary
|
|
|
||
| // Check if the hook call is part of a condition expression | ||
| // If the hook is used in a condition (like `if (useHook())`) rather than being conditionally called, | ||
| // then it's valid and we should return false | ||
| if is_hook_in_condition_expression(nodes, to) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
hmm while this solution works, i'm not convinced it's the ideal solution?
Ideally, i think we would change the cfg to represent that the test of an if statement is always represented as visited.
We would look to change the code inside visit_if_statement to something like:
iff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs
index 394307c1c..8c99f9391 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 */There was a problem hiding this comment.
This is much much more elegant 👍, I will close my MR, thx for sharing
There was a problem hiding this comment.
Any possible to give a bit more context why this can fix the issue?
I will look into the codes, still trying to understand the whole process in OXC.
Thanks for sharing again
There was a problem hiding this comment.
yeah sure. firstly thanks for trying to fix it, we appreciate the contributions!
so the control flow graph logic of this codebase is (imo, i'm sure others may disagree 🙂) some of the most complex logical code.
From my understanding, the problem was that the "test" of the IfStatement, was being marked as conditionally visited (which is incorrect as the test is always evaluated).
by changing that logic in oxc_semantic, we changed the CFG, to represent that the test expression is always visited.
i'll try and put up a PR tomorrow, with a more detailed explanation on why those exact changes fix this issue
(on mobile atm)
thanks!
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
2 participants
Try to fix #9795 react-hooks/rules-of-hooks false positive