fix: jsx dependency detection in useexhaustivedependencies (#8917)#8930
fix: jsx dependency detection in useexhaustivedependencies (#8917)#8930ematipico merged 17 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 6c0cdf2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
github-actions
bot
added
A-Linter
ANKANJAGTAP
changed the title
WalkthroughAdds JSX identifier handling to the use_exhaustive_dependencies lint: Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs`:
- Around line 668-681: The AddDependency autofix currently only handles
JsReferenceIdentifier/AnyJsExpression and ignores
AnyExpressionCandidate::JsxReferenceIdentifier, causing no-op fixes for JSX
captures; update the Fix::AddDependency handling to add a branch for
AnyExpressionCandidate::JsxReferenceIdentifier that extracts the JSX
identifier's name token (e.g., reference_identifier.name_token or equivalent),
converts it into a plain identifier expression (same shape used for
JsReferenceIdentifier/AnyJsExpression), and insert that converted identifier
into the dependency list; apply the same change to the duplicate handling block
around the second occurrence (lines referenced 1575-1589) so both places convert
JSX name tokens before creating the AddDependency fix.
| AnyExpressionCandidate::JsxReferenceIdentifier(reference_identifier) => { | ||
| if let Some(binding) = model.binding(reference_identifier) { | ||
| is_stable_binding( | ||
| &binding.tree(), | ||
| None, | ||
| component_function_range, | ||
| model, | ||
| options, | ||
| 0, | ||
| ) | ||
| } else { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Autofix won’t add JSX dependencies.
With JsxReferenceIdentifier now producing missing-dependency diagnostics, the Fix::AddDependency action still only converts JsReferenceIdentifier/AnyJsExpression. JSX captures will therefore produce a no‑op fix. Please add a JSX branch that converts the JSX identifier’s name token into a normal identifier expression before insertion.
Also applies to: 1575-1589
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs`
around lines 668 - 681, The AddDependency autofix currently only handles
JsReferenceIdentifier/AnyJsExpression and ignores
AnyExpressionCandidate::JsxReferenceIdentifier, causing no-op fixes for JSX
captures; update the Fix::AddDependency handling to add a branch for
AnyExpressionCandidate::JsxReferenceIdentifier that extracts the JSX
identifier's name token (e.g., reference_identifier.name_token or equivalent),
converts it into a plain identifier expression (same shape used for
JsReferenceIdentifier/AnyJsExpression), and insert that converted identifier
into the dependency list; apply the same change to the duplicate handling block
around the second occurrence (lines referenced 1575-1589) so both places convert
JSX name tokens before creating the AddDependency fix.
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs`:
- Around line 545-554: Remove the panic call in get_expression_candidates that
triggers on JsxReferenceIdentifier (the panic!("DEBUG: Found likely
JsxReferenceIdentifier: {:?}", jsx_ref)); instead drop the panic and either log
non-fatal debug information with dbg!(jsx_ref) if you need runtime debug output
or simply proceed to push the candidate as originally intended by calling
result.push(AnyExpressionCandidate::JsxReferenceIdentifier(jsx_ref.clone())) and
returning result; ensure no panics remain in get_expression_candidates or other
lint paths so JSX identifiers do not crash the linter.
crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs
Show resolved
Hide resolved
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Please add a changeset @ANKANJAGTAP
.changeset/jsx-exhaustive-deps.md
Outdated
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fix `useExhaustiveDependencies` so JSX component identifiers (e.g. `<Sub />`) are detected as hook dependencies and included in autofix suggestions. |
There was a problem hiding this comment.
| Fix `useExhaustiveDependencies` so JSX component identifiers (e.g. `<Sub />`) are detected as hook dependencies and included in autofix suggestions. | |
| Fixed [#8917](https://github.com/biomejs/biome/issues/8917), [`useExhaustiveDependencies`](https://biomejs.dev/linter/rules/use-exhaustive-dependencies/) now correctly detects JSX component identifiers as hook dependencies. |
perhaps?
|
Tests seem to fail |
|
@ANKANJAGTAP please run the tests yourself and confirm everything passes |
|
I’ve run cargo test -p biome_js_analyze locally on nightly and all tests pass. I also ran the full test suite and didn’t see any failures (only expected ignored tests). |
|
@ANKANJAGTAP please don't forget checking our CONTRIBUTION guide. Specifically this: https://github.com/biomejs/biome/blob/main/crates/biome_analyze/CONTRIBUTING.md Testing a rule: In your case it would be |
|
I ran cargo test -p biome_js_analyze and the full test suite locally on nightly and all tests pass. I attempted to run just test-lintrule useExhaustiveDependencies, but the repo’s justfile uses the working-directory attribute which is not supported by the latest released just (v1.46.0). Happy to rerun if there’s a recommended just version, but analyzer and snapshot tests pass locally. |
Din't know that. We will have to fix it |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Please create valid tests that make sure that adding the unsafe fix result in the rule not being triggered. To be more clear, the new diagnostics show, as unsafe fix, to add the JSX component to the array, hence we need to create fixtures with that code already applied and make sure no diagnostics are emitted. If they are, it means the fix isn't correct.
Add // should not generate diagnostics to the new file
| @@ -0,0 +1,29 @@ | |||
| import { getSubComponent } from "@external" | |||
There was a problem hiding this comment.
Please add // should generate diagnostics at the top of the file
There was a problem hiding this comment.
Added a fixture covering the post-fix state and approved the snapshot.
All tests pass locally.
|
@ANKANJAGTAP thank you for making all the fixes. Could you please also add tests with a member-access in the JSX tag? I.e. |
Added tests covering JSX member access (<MyNS.MyComponent />), including both diagnostic and no-diagnostic cases. All tests pass locally. |
|
@ANKANJAGTAP tests are failing |
|
Please give me some time; I will fix it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/jsxMemberDependenciesFixed.jsx`:
- Line 10: The dependency array for the useCallback that defines render is
incorrect: update the useCallback call (function name render) so the dependency
array includes the namespace MyNS (i.e., change the empty array to include MyNS)
so the exhaustive-deps rule is satisfied for the JSX expression
<MyNS.MyComponent /> referenced inside the callback; ensure the identifier
referenced matches MyNS used in the JSXMember expression.
| }; | ||
|
|
||
| export function Component() { | ||
| const render = useCallback(() => <MyNS.MyComponent />, []); |
There was a problem hiding this comment.
Dependency array appears incorrect for a "fixed" test case.
The comment on line 1 states this should not generate diagnostics, yet the dependency array is empty []. According to the PR objectives and comparing with jsxMemberDependencies.jsx (which is identical and should trigger a warning), the fixed version ought to include [MyNS] to satisfy the exhaustive dependencies rule.
🐛 Proposed fix
- const render = useCallback(() => <MyNS.MyComponent />, []);
+ const render = useCallback(() => <MyNS.MyComponent />, [MyNS]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const render = useCallback(() => <MyNS.MyComponent />, []); | |
| const render = useCallback(() => <MyNS.MyComponent />, [MyNS]); |
🤖 Prompt for AI Agents
In
`@crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/jsxMemberDependenciesFixed.jsx`
at line 10, The dependency array for the useCallback that defines render is
incorrect: update the useCallback call (function name render) so the dependency
array includes the namespace MyNS (i.e., change the empty array to include MyNS)
so the exhaustive-deps rule is satisfied for the JSX expression
<MyNS.MyComponent /> referenced inside the callback; ensure the identifier
referenced matches MyNS used in the JSXMember expression.
|
All requested changes have been addressed and tests are passing locally. |
5 participants
Summary
Fix lint/correctness/useExhaustiveDependencies not detecting JSX component identifiers (e.g. ) as dependencies inside React hooks.
Previously, JSX component bindings were ignored, which could lead to missing dependency warnings and stale closures.
Root Cause:
get_expression_candidates did not handle JsxReferenceIdentifier nodes.
Changes:
Added JsxReferenceIdentifier to AnyExpressionCandidate.
Updated get_expression_candidates to return JSX identifiers.
Updated capture_needs_to_be_in_the_dependency_list to check JSX bindings via the semantic model.
Result:
Locally defined components used in JSX inside hooks are now correctly flagged as dependencies.
Test Plan
Added test where inside useCallback without [Sub] triggers a warning.
Verified no warning when [Sub] is included.
Confirmed no regressions in existing dependency checks.
Docs
Updated rule docs with JSX dependency example.