fix(lint): improve useHookAtTopLevel lint
#7749
Conversation
Before performing hook call recursion checks, walk the call's ancestors searching for `AnyJsFunctionOrMethod` nodes. If one is not encountered, it is assumed the call is at the top of the file (or in a top-level expression statement), and the new `SuggestionKind::TopLevel` lint is generated.
Add additional check that the hook was called from either: - Named function that is a component or hook - An anonymous function (`JsArrowFunctionExpression` or `JsFunctionExpression`) This is a check that occurs after any existing checks, so the previous lints which trace indirect calls will still function as they previously did.
🦋 Changeset detectedLatest commit: 94c4f97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
useHookAtTopLevel lintuseHookAtTopLevel lint
WalkthroughUpdates a changeset to bump the @biomejs/biome minor and documents enhancements to the useHookAtTopLevel lint. The analyzer now detects hook calls at module/top-level, adds a ComponentOrHook scenario, introduces an internal is_function_expression helper and is_top_level_call detection, and extends the SuggestionKind enum with Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js (2)
24-31: Add a case for named function expressions to lock intent.You’re allowing JsFunctionExpression wholesale. Add:
- test("named fn expr", function Named() { useHook(); }); // expected: allowed by current implementation.
This guards the “anonymous” = expression-node heuristic.
14-17: Also test object/class methods that aren’t hooks/components.Please add invalid cases like:
- const o = { notHook() { useHook(); } };
- class C { notHook() { useHook(); } }
These should produce the new “ComponentOrHook” diagnostic.
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs (3)
258-268: Clarify “anonymous” semantics (or enforce it).is_anonymous_function returns true for all JsFunctionExpression, including named function expressions. If you truly mean “anonymous only”, check the function expression’s identifier; otherwise, consider renaming for clarity.
Two options:
- Keep behaviour; rename helper for clarity.
- Enforce anonymity; add an id() check.
Suggested rename (if keeping behaviour):
- fn is_anonymous_function(function: &AnyJsFunctionOrMethod) -> bool { + fn is_function_expression_or_arrow(function: &AnyJsFunctionOrMethod) -> bool {And use site:
- !function.is_react_component_or_hook() && !is_anonymous_function(function) + !function.is_react_component_or_hook() && !is_function_expression_or_arrow(function)Also applies to: 554-566
583-589: Diagnostics: small polish.
- Consider mentioning “function or method” in ComponentOrHook message to reflect class/object methods.
- Update the docs link to the current React site.
Suggested tweaks:
- "This hook is being called from within a function that is not a hook or component." + "This hook is being called from within a function or method that is not a hook or component."- "See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level" + "See https://react.dev/reference/rules/rules-of-hooks#only-call-hooks-at-the-top-level"Also applies to: 618-626, 635-637
28-74: Rule docs: add examples for new cases.Consider adding minimal examples for:
- Invalid: module‑level hook.
- Invalid: hook in a named non‑component/hook function.
- Valid: hook in an arrow/function expression callback.
Keeps rustdoc aligned with behaviour.
.changeset/huge-cycles-dance.md (1)
5-5: Optional: start with the issue reference.Guidelines prefer bug‑fix changesets to start with “Fixed [#…] …”. Consider:
“Fixed #1984. Updated useHookAtTopLevel to better catch invalid hook usage.”
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/huge-cycles-dance.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs(8 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
.changeset/**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/**/*.md: Create changesets using thejust new-changesetcommand; do not author them manually
In changeset markdown, only use headers #### or #####
Changeset descriptions must end every sentence with a full stop (.)
For bug fixes, start the changeset description with a linked issue reference like “Fixed #1234”
Prefer past tense for what was done and present tense for current behavior in changesets
Files:
.changeset/huge-cycles-dance.md
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.jscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.jscrates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.jscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.jscrates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.jscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Before committing, format Rust and TOML files (e.g., via
just f/just format)
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and options via inline rustdoc in Rust source
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js (1)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js (1)
renderHook(116-116)
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs (1)
crates/biome_rowan/src/ast/mod.rs (2)
try_cast(181-187)cast_ref(142-151)
🔇 Additional comments (3)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js (1)
68-96: Renames look good; coverage unchanged.The component examples still exercise top‑level usage correctly. No action.
Also applies to: 80-89
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs (2)
251-256: Top‑level detection is correct and cheap.Using ancestor scan against AnyJsFunctionOrMethod is a clear way to catch module‑scope calls. Nice.
469-475: Early return on module‑level hooks preserves existing flow.Short‑circuiting TopLevel before call‑graph traversal avoids redundant work and duplicate diagnostics. Good call.
CodSpeed Performance ReportMerging #7749 will not alter performanceComparing Summary
Footnotes
|
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
Outdated
Show resolved
Hide resolved
- add `is_function_expression` method to `AnyJsFunctionOrMethod`, implemented with `matches!` (replaces `is_anonymous_function`) - use `can_cast(...)` in place of `try_cast(...).is_ok()` - use `is_some_and(...)` in place of `filter(...).is_some()` - provide additional hook examples in documentation - add test cases for named function expressions, class methods, and methods on objects - alter lint wording to to include `function or method`, and update hint URL to new `react.dev` docs
04f7c37 to
786b693
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/customHook.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidWrapped.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
.changeset/huge-cycles-dance.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs(10 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_to 8000 p_level.rscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Before committing, format Rust and TOML files (e.g., via
just f/just format)
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and options via inline rustdoc in Rust source
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js
.changeset/**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/**/*.md: Create changesets using thejust new-changesetcommand; do not author them manually
In changeset markdown, only use headers #### or #####
Changeset descriptions must end every sentence with a full stop (.)
For bug fixes, start the changeset description with a linked issue reference like “Fixed #1234”
Prefer past tense for what was done and present tense for current behavior in changesets
Files:
.changeset/huge-cycles-dance.md
🧬 Code graph analysis (1)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/hookLocations.js (1)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js (1)
renderHook(116-116)
🪛 LanguageTool
.changeset/huge-cycles-dance.md
[style] ~9-~9: This phrase is redundant. Consider using “outside”.
Context: ...d at the module level (top of the file, outside of any function) - A hook is used within ...
(OUTSIDE_OF)
786b693 to
94c4f97
Compare
|
While looking through the It does run the risk of false-negatives if a test call isn't correctly identified, however the lint's accuracy could improve as |
|
We don't publish crate updates regularly, so the documentation you are referencing is likely outdated regarding
This is a little hard to parse. Could you provide a code sample for what you are talking about? |
|
Given the following code: test("something", () => {
useHook(); // Invalid as it should be called within `renderHook` (or similar) within a test.
renderHook(() => useHook()); // Valid
});In the current implementation, both hook uses are allowed as they exist within a function expression. I'm proposing the lint is updated to perform the following checks:
// Yes:
() => useHook()
// No:
useHook()
// Yes:
() => {
renderHook(() => useHook());
}
someFunc(arg1, arg2, () => {
renderHook(() => useHook());
});
It's certainly more involved than the current lint, and even as I'm typing it out I'm not 100% convinced it's the best move. It just crossed my mind, and I figured someone else may have a better approach/opinion on it! |
Summary
This PR addresses a number of items discussed in #1984, in order to bring detection of invalid react hooks closer in line with eslint's
rules-of-hooks. Specifically:JsArrowFunctionExpressionorJsFunctionExpression).These changes extend the current lint behaviour, so any previous behaviour is retained (ie. hook recursion and indirect hook checks are run before non-component/hook checks).
I've based my implementation form @arendjr's comment:
Based on this same comment I have created a
minorchangeset too.I am open to any suggestions or ideas for how to better apply these lint changes.
Test Plan
I have added additional test cases validating these changes. I have also had to remove one of the previously valid test cases, as this lint will now reject it:
Docs
I don't believe this is applicable, as the existing docs still seem fine.