fix(lint): noArrayIndexKey should flag index anywhere in template#8968
fix(lint): noArrayIndexKey should flag index anywhere in template#8968dyc3 merged 4 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: eee9cf9 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration 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)
WalkthroughReworks the noArrayIndexKey lint to gather multiple candidate identifiers from key expressions (identifiers, template literals, binary expressions, parenthesised expressions) via a new collect_reference_identifiers routine. Each candidate is resolved to a function parameter and traced to its containing call expression to check whether it represents an array index (is_array_method_index). Retains the React.cloneElement diagnostic path, removes the previous recursive cap_array_index_value traversal, simplifies control flow with early continues, and adds tests covering template and binary key expressions that include the index. Adds a changelog entry for the fix. Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
Please add a changeset and restore our PR template |
Merging this PR will not alter performance
Comparing Footnotes
|
|
Thanks for the reminder. I’ll add a changeset and restore the PR template format in this PR. |
|
Addressed both points: I added a changeset in this branch and restored the PR description structure using the project PR template sections (Summary/Test Plan/Docs). |
dyc3
left a comment
There was a problem hiding this comment.
Looks good. Just fix the lints from clippy
|
Thanks, I’ll take care of the remaining Clippy lints in a follow-up commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rs`:
- Around line 175-176: The code currently uses the `?` operator on
`is_array_method_index(¶meter, &call_expression)` inside `run`, which causes
`run` to return early when that helper yields `None` (non-array contexts) and
skips later candidates; change this to explicitly handle the Option: call
`is_array_method_index(¶meter, &call_expression)` and `match`/`if let
Some(is_index) = ...` (or `if let None = ... { continue; }`) and only bail out
when it returns `Some(false)`/`true` per your logic, otherwise `continue` to the
next candidate so non-array contexts do not abort the whole analysis. Ensure you
update the branch around the current `if !is_array_method_index(...)? {
continue; }` to use this explicit handling.
| if !is_array_method_index(¶meter, &call_expression)? { | ||
| continue; |
There was a problem hiding this comment.
Avoid bailing out the whole analysis on non-array candidates.
At Line 175, is_array_method_index(...)? returns None for non-array call contexts and exits run early, so later candidates in the same key expression are never checked. That can still miss a real index reference.
Suggested fix
- if !is_array_method_index(¶meter, &call_expression)? {
- continue;
- }
+ let Some(is_index_parameter) = is_array_method_index(¶meter, &call_expression)
+ else {
+ continue;
+ };
+ if !is_index_parameter {
+ continue;
+ }📝 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.
| if !is_array_method_index(¶meter, &call_expression)? { | |
| continue; | |
| let Some(is_index_parameter) = is_array_method_index(¶meter, &call_expression) | |
| else { | |
| continue; | |
| }; | |
| if !is_index_parameter { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rs` around
lines 175 - 176, The code currently uses the `?` operator on
`is_array_method_index(¶meter, &call_expression)` inside `run`, which causes
`run` to return early when that helper yields `None` (non-array contexts) and
skips later candidates; change this to explicitly handle the Option: call
`is_array_method_index(¶meter, &call_expression)` and `match`/`if let
Some(is_index) = ...` (or `if let None = ... { continue; }`) and only bail out
when it returns `Some(false)`/`true` per your logic, otherwise `continue` to the
next candidate so non-array contexts do not abort the whole analysis. Ensure you
update the branch around the current `if !is_array_method_index(...)? {
continue; }` to use this explicit handling.
Summary
Fixes #8812.
lint/suspicious/noArrayIndexKeypreviously only tracked the last identifier found in a template/binary expression (and could skip reporting when templates mixed identifier and non-identifier expressions).This update collects all reference identifiers from
keyexpressions (template, binary, parenthesized) and reports when any of them resolves to the array-index parameter.Added test coverage for cases like:
key={`${index}-${item}`}key={`${item.title}-${index}`}key={index + item}AI assistance notice: this PR was prepared with AI assistance and manually verified before submission.
Test Plan
cargo test -p biome_js_analyze -- no_array_index_keycargo test -p biome_js_formatterDocs
No docs change required; this is a behavior fix for an existing lint rule.