diff --git a/.changeset/poor-horses-smash.md b/.changeset/poor-horses-smash.md new file mode 100644 index 000000000000..1fd97d4af80d --- /dev/null +++ b/.changeset/poor-horses-smash.md @@ -0,0 +1,16 @@ +--- +"@biomejs/biome": patch +--- + +Fixed an issue (#6393) where the [useHookAtTopLevel](https://biomejs.dev/linter/rules/use-hook-at-top-level/) rule reported excessive diagnostics for nested hook calls. + +The rule now reports only the offending top-level call site, not sub-hooks of composite hooks. + +```js +// Before: reported twice (useFoo and useBar). +function useFoo() { return useBar(); } +function Component() { + if (cond) useFoo(); +} +// After: reported once at the call to useFoo(). +``` diff --git a/crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs b/crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs index a0e2de998faa..8f4a73af41ec 100644 --- a/crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs +++ b/crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs @@ -422,6 +422,7 @@ impl Queryable for FunctionCall { #[derive(Debug)] pub struct CallPath { call: JsCallExpression, + is_enclosed_in_component_or_hook: bool, path: Vec, } @@ -449,10 +450,16 @@ impl Rule for UseHookAtTopLevel { let root = CallPath { call: call.clone(), path: vec![], + is_enclosed_in_component_or_hook: false, }; let mut calls = vec![root]; - while let Some(CallPath { call, mut path }) = calls.pop() { + while let Some(CallPath { + call, + mut path, + is_enclosed_in_component_or_hook, + }) = calls.pop() + { let range = call.syntax().text_range_with_trivia(); if path.contains(&range) { @@ -485,6 +492,9 @@ impl Rule for UseHookAtTopLevel { }); } + let enclosed = is_enclosed_in_component_or_hook + || enclosing_function.is_react_component_or_hook(); + if let AnyJsFunctionOrMethod::AnyJsFunction(function) = enclosing_function && let Some(calls_iter) = function.all_calls(model) { @@ -492,10 +502,17 @@ impl Rule for UseHookAtTopLevel { calls.push(CallPath { call: call.tree(), path: path.clone(), + is_enclosed_in_component_or_hook: enclosed, }); } } } else { + // Avoid duplicate diagnostics if this path already passed through + // a component/hook. We still keep previously enqueued paths to + // allow recursion detection elsewhere. + if is_enclosed_in_component_or_hook { + continue; + } return Some(Suggestion { hook_name_range: get_hook_name_range()?, path, diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js index 460e6b19e2c3..0b26e3a10851 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js @@ -68,13 +68,13 @@ export default function Component5() { } }; -const Component6 = () => { +const useHook6 = () => { useEffect(); }; const Component7 = () => { if (a == 1) { - Component6(); + useHook6(); } }; @@ -189,4 +189,4 @@ function useRecursiveHookA() { function useRecursiveHookB() { useRecursiveHookA(); -} \ No newline at end of file +} diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap index 8bcc8f8565d4..f876c827e257 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap @@ -1,5 +1,6 @@ --- source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 expression: invalid.js --- # Input @@ -74,13 +75,13 @@ export default function Component5() { } }; -const Component6 = () => { +const useHook6 = () => { useEffect(); }; const Component7 = () => { if (a == 1) { - Component6(); + useHook6(); } }; @@ -196,6 +197,7 @@ function useRecursiveHookA() { function useRecursiveHookB() { useRecursiveHookA(); } + ``` # Diagnostics @@ -463,23 +465,14 @@ invalid.js:67:9 lint/correctness/useHookAtTopLevel ━━━━━━━━━ ``` ``` -invalid.js:72:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.js:77:9 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. - - 71 │ const Component6 = () => { - > 72 │ useEffect(); - │ ^^^^^^^^^ - 73 │ }; - 74 │ - - i This is the call path until the hook. + × This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. 75 │ const Component7 = () => { - > 76 │ if (a == 1) { - │ - > 77 │ Component6(); - │ ^^^^^^^^^^^^ + 76 │ if (a == 1) { + > 77 │ useHook6(); + │ ^^^^^^^^ 78 │ } 79 │ }; @@ -881,6 +874,7 @@ invalid.js:187:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━ > 191 │ useRecursiveHookA(); │ ^^^^^^^^^^^^^^^^^^^ 192 │ } + 193 │ i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. @@ -900,6 +894,7 @@ invalid.js:191:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━ > 191 │ useRecursiveHookA(); │ ^^^^^^^^^^^^^^^^^ 192 │ } + 193 │ i This is the call path until the hook. diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts index 17cf6cdc96da..6936e2e5c0ae 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts @@ -1,9 +1,9 @@ -const Component1 = () => { +const useHook = () => { useEffect() as []; }; const Component2 = () => { if (a == 1) { - Component1(); + useHook(); } }; diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snap b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snap index 78cd1dbd5580..62a2e67c5eb8 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snap +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snap @@ -1,17 +1,17 @@ --- source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 expression: invalid.ts -snapshot_kind: text --- # Input ```ts -const Component1 = () => { +const useHook = () => { useEffect() as []; }; const Component2 = () => { if (a == 1) { - Component1(); + useHook(); } }; @@ -19,23 +19,14 @@ const Component2 = () => { # Diagnostics ``` -invalid.ts:2:3 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +invalid.ts:7:7 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. - - 1 │ const Component1 = () => { - > 2 │ useEffect() as []; - │ ^^^^^^^^^ - 3 │ }; - 4 │ - - i This is the call path until the hook. + × This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. 5 │ const Component2 = () => { - > 6 │ if (a == 1) { - │ - > 7 │ Component1(); - │ ^^^^^^^^^^^^ + 6 │ if (a == 1) { + > 7 │ useHook(); + │ ^^^^^^^ 8 │ } 9 │ }; diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js new file mode 100644 index 000000000000..169547281921 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js @@ -0,0 +1,11 @@ +function useFoo() { + return useBar(); +} + +function Component() { + if (condition) { + // This call should be reported just once. + // See https://github.com/biomejs/biome/issues/6393 + return useFoo(); + } +} diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js.snap b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js.snap new file mode 100644 index 000000000000..9e2165f77dd7 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js.snap @@ -0,0 +1,40 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 +expression: invalidCompositeHook.js +--- +# Input +```js +function useFoo() { + return useBar(); +} + +function Component() { + if (condition) { + // This call should be reported just once. + // See https://github.com/biomejs/biome/issues/6393 + return useFoo(); + } +} + +``` + +# Diagnostics +``` +invalidCompositeHook.js:9:16 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. + + 7 │ // This call should be reported just once. + 8 │ // See https://github.com/biomejs/biome/issues/6393 + > 9 │ return useFoo(); + │ ^^^^^^ + 10 │ } + 11 │ } + + i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. + + i See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level + + +```