From 40228615290a8fa787d84f9e35f40197dbe3c546 Mon Sep 17 00:00:00 2001 From: lin Date: Sun, 16 Mar 2025 08:07:49 +0100 Subject: [PATCH 1/4] fix: initial fix for react-hooks/rules-of-hooks false positive of conditional rule --- .../src/rules/react/rules_of_hooks.rs | 189 ++++++++++++++---- .../src/snapshots/react_rules_of_hooks.snap | 33 +++ 2 files changed, 188 insertions(+), 34 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..a807a0ecc9819 100644 --- a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs +++ b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use oxc_ast::{ AstKind, - ast::{ArrowFunctionExpression, Function}, + ast::{ArrowFunctionExpression, Function, Expression}, }; use oxc_cfg::{ ControlFlowGraph, EdgeType, ErrorEdgeKind, InstructionKind, @@ -11,6 +11,7 @@ use oxc_cfg::{ use oxc_macros::declare_oxc_lint; use oxc_semantic::{AstNodes, NodeId}; use oxc_syntax::operator::AssignmentOperator; +use oxc_span::Span; use crate::{ AstNode, @@ -124,13 +125,12 @@ impl Rule for RulesOfHooks { } let cfg = ctx.cfg(); + let nodes = ctx.nodes(); let span = call.span; let hook_name = call.callee_name().expect("We identify hooks using their names so it should be named."); - let nodes = ctx.nodes(); - let is_use = is_react_function_call(call, "use"); let Some(parent_func) = parent_func(nodes, node) else { @@ -249,7 +249,7 @@ impl Rule for RulesOfHooks { return ctx.diagnostic(diagnostics::loop_hook(span, hook_name)); } - if has_conditional_path_accept_throw(cfg, parent_func, node) { + if has_conditional_path_accept_throw(cfg, nodes, parent_func, node) { #[expect(clippy::needless_return)] return ctx.diagnostic(diagnostics::conditional_hook(span, hook_name)); } @@ -258,12 +258,21 @@ impl Rule for RulesOfHooks { fn has_conditional_path_accept_throw( cfg: &ControlFlowGraph, + nodes: &AstNodes<'_>, from: &AstNode<'_>, to: &AstNode<'_>, ) -> bool { let from_graph_id = from.cfg_id(); let to_graph_id = to.cfg_id(); let graph = cfg.graph(); + + // 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; + } + if graph .edges(to_graph_id) .any(|it| matches!(it.weight(), EdgeType::Error(ErrorEdgeKind::Explicit))) @@ -271,32 +280,6 @@ fn has_conditional_path_accept_throw( // 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::, _>(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_graph_id, Some(to_graph_id), |e| match e.weight() { @@ -327,6 +310,81 @@ fn has_conditional_path_accept_throw( }) } +// Helper function to check if a hook call is part of a condition expression +fn is_hook_in_condition_expression(nodes: &AstNodes<'_>, node: &AstNode<'_>) -> bool { + let node_span = match node.kind() { + AstKind::CallExpression(call_expr) => call_expr.span, + _ => return false, // Not a call expression, so not a hook call + }; + + // Get the parent of the hook call + if let Some(parent_id) = nodes.parent_id(node.id()) { + let parent = nodes.get_node(parent_id); + match parent.kind() { + // Check if the hook is used directly in an if statement condition + AstKind::IfStatement(if_stmt) => { + // Get the span of the test expression + let test_span = match &if_stmt.test { + Expression::CallExpression(call) => call.span, + Expression::Identifier(ident) => ident.span, + Expression::UnaryExpression(unary) => unary.span, + Expression::LogicalExpression(logical) => logical.span, + Expression::BinaryExpression(binary) => binary.span, + _ => return false, + }; + // Check if the hook call is within the test expression of the if statement + is_span_within(node_span, test_span) + }, + // Check if the hook is used in a logical expression (&&, ||) + AstKind::LogicalExpression(logical_expr) => { + // We need to check if the hook is in the left side (condition part) + // of the logical expression, not the right side + if let Expression::CallExpression(call) = &logical_expr.left { + // If the hook is in the left side, it's a valid condition + is_span_within(node_span, call.span) + } else if let Expression::UnaryExpression(unary) = &logical_expr.left { + // Handle cases like !useHook() && something + if let Expression::CallExpression(call) = &unary.argument { + is_span_within(node_span, call.span) + } else { + false + } + } else { + false + } + }, + // Check if the hook is used in a conditional expression (ternary) + AstKind::ConditionalExpression(cond_expr) => { + // Get the span of the test expression + let test_span = match &cond_expr.test { + Expression::CallExpression(call) => call.span, + Expression::Identifier(ident) => ident.span, + Expression::UnaryExpression(unary) => unary.span, + Expression::LogicalExpression(logical) => logical.span, + Expression::BinaryExpression(binary) => binary.span, + _ => return false, + }; + // Check if the hook call is within the test part of the conditional expression + is_span_within(node_span, test_span) + }, + // Check if the hook is used in a unary expression (like !) + AstKind::UnaryExpression(unary_expr) => { + // Check if the hook call is within the unary expression + is_span_within(node_span, unary_expr.span) + }, + // For other cases, check if the parent is part of a condition + _ => is_hook_in_condition_expression(nodes, parent), + } + } else { + false + } +} + +// Helper function to check if one span is within another +fn is_span_within(inner: Span, outer: Span) -> bool { + inner.start >= outer.start && inner.end <= outer.end +} + fn parent_func<'a>(nodes: &'a AstNodes<'a>, node: &AstNode) -> Option<&'a AstNode<'a>> { nodes .ancestor_ids(node.id()) @@ -444,6 +502,51 @@ fn test() { useHook(); } ", + // 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() { @@ -724,8 +827,6 @@ fn test() { if (c) {} else {} if (c) {} else {} if (c) {} else {} - if (c) {} else {} - if (c) {} else {} // 10 hooks useHook(); @@ -803,8 +904,6 @@ fn test() { {FILLER ? FILLER : FILLER} {FILLER ? FILLER : FILLER} {FILLER ? FILLER : FILLER} - {FILLER ? FILLER : FILLER} - {FILLER ? FILLER : FILLER} ); }; @@ -968,6 +1067,28 @@ 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; + } + ", // 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..6e3e6b8fe6e7c 100644 --- a/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap +++ b/crates/oxc_linter/src/snapshots/react_rules_of_hooks.snap @@ -1,5 +1,6 @@ --- source: crates/oxc_linter/src/tester.rs +assertion_line: 362 --- ⚠ eslint-plugin-react-hooks(rules-of-hooks): React Hook "useConditionalHook" is called conditionally. React Hooks must be called in the exact same order in every component render. ╭─[rules_of_hooks.tsx:4:18] @@ -9,6 +10,38 @@ 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 "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 │ From d0b7f5294d68d8fe72148091193e482a59df586a Mon Sep 17 00:00:00 2001 From: lin Date: Sun, 16 Mar 2025 08:29:07 +0100 Subject: [PATCH 2/4] keep current test cases --- .../src/rules/react/rules_of_hooks.rs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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 a807a0ecc9819..2c4928c9e5dfa 100644 --- a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs +++ b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs @@ -280,6 +280,32 @@ fn has_conditional_path_accept_throw( // 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::, _>(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_graph_id, Some(to_graph_id), |e| match e.weight() { @@ -827,6 +853,8 @@ fn test() { if (c) {} else {} if (c) {} else {} if (c) {} else {} + if (c) {} else {} + if (c) {} else {} // 10 hooks useHook(); @@ -904,6 +932,8 @@ fn test() { {FILLER ? FILLER : FILLER} {FILLER ? FILLER : FILLER} {FILLER ? FILLER : FILLER} + {FILLER ? FILLER : FILLER} + {FILLER ? FILLER : FILLER} ); }; From 11f1b766b9a8957cb5d2ea2d0a80f6f527814017 Mon Sep 17 00:00:00 2001 From: lin Date: Sun, 16 Mar 2025 12:37:40 +0100 Subject: [PATCH 3/4] fix: add one more test to cover ternary condition in if clause --- crates/oxc_linter/src/rules/react/rules_of_hooks.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 2c4928c9e5dfa..937405efc0072 100644 --- a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs +++ b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs @@ -528,6 +528,17 @@ 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() { From 93213fda565f4c6768e7ef75c43dd05ee894dbac Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sun, 16 Mar 2025 12:14:13 +0000 Subject: [PATCH 4/4] [autofix.ci] apply automated fixes --- crates/oxc_linter/src/rules/react/rules_of_hooks.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 937405efc0072..8c7254f667db4 100644 --- a/crates/oxc_linter/src/rules/react/rules_of_hooks.rs +++ b/crates/oxc_linter/src/rules/react/rules_of_hooks.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use oxc_ast::{ AstKind, - ast::{ArrowFunctionExpression, Function, Expression}, + ast::{ArrowFunctionExpression, Expression, Function}, }; use oxc_cfg::{ ControlFlowGraph, EdgeType, ErrorEdgeKind, InstructionKind, @@ -10,8 +10,8 @@ use oxc_cfg::{ }; use oxc_macros::declare_oxc_lint; use oxc_semantic::{AstNodes, NodeId}; -use oxc_syntax::operator::AssignmentOperator; use oxc_span::Span; +use oxc_syntax::operator::AssignmentOperator; use crate::{ AstNode, @@ -360,7 +360,7 @@ fn is_hook_in_condition_expression(nodes: &AstNodes<'_>, node: &AstNode<'_>) -> }; // Check if the hook call is within the test expression of the if statement is_span_within(node_span, test_span) - }, + } // Check if the hook is used in a logical expression (&&, ||) AstKind::LogicalExpression(logical_expr) => { // We need to check if the hook is in the left side (condition part) @@ -378,7 +378,7 @@ fn is_hook_in_condition_expression(nodes: &AstNodes<'_>, node: &AstNode<'_>) -> } else { false } - }, + } // Check if the hook is used in a conditional expression (ternary) AstKind::ConditionalExpression(cond_expr) => { // Get the span of the test expression @@ -392,12 +392,12 @@ fn is_hook_in_condition_expression(nodes: &AstNodes<'_>, node: &AstNode<'_>) -> }; // Check if the hook call is within the test part of the conditional expression is_span_within(node_span, test_span) - }, + } // Check if the hook is used in a unary expression (like !) AstKind::UnaryExpression(unary_expr) => { // Check if the hook call is within the unary expression is_span_within(node_span, unary_expr.span) - }, + } // For other cases, check if the parent is part of a condition _ => is_hook_in_condition_expression(nodes, parent), }