fix(linter): handle re-ordered index in template string for noArrayIndexKey#9237
fix(linter): handle re-ordered index in template string for noArrayIndexKey#9237slegarraga wants to merge 1 commit intobiomejs:mainfrom
Conversation
…dexKey
Previously, the rule only detected array index usage in template literals when
the index appeared as the last interpolation (e.g. `${item}-${index}`).
When the index appeared earlier (e.g. `${index}-${item}`), it was missed
because each template element overwrote the previous candidate.
Now all identifier expressions in template literals are collected and each is
checked against the array index parameter binding, catching the false negative.
Closes biomejs#8812
|
github-actions
bot
added
A-Linter
WalkthroughThis change refactors the Suggested labels
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 |
![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
🤖 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 172-177: cap_array_index_value only returns a single identifier,
so when handling AnyJsExpression::JsBinaryExpression you miss index identifiers
if other identifiers appear later; update the logic to collect all identifier
captures from the binary expression (not just one) and push each into
candidate_references. Concretely: change cap_array_index_value (or add a new
helper) to traverse the binary expression tree and return a Vec of captures
(instead of a single Option), then in the AnyJsExpression::JsBinaryExpression
arm iterate over those captures and push each reference into
candidate_references (references: AnyJsExpression::JsBinaryExpression,
cap_array_index_value, capture_array_index, candidate_references).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/suspicious/no_array_index_key.rscrates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx
| AnyJsExpression::JsBinaryExpression(binary_expression) => { | ||
| let mut capture_array_index = None; | ||
| let _ = cap_array_index_value(&binary_expression, &mut capture_array_index); | ||
| if let Some(reference) = capture_array_index { | ||
| candidate_references.push(reference); | ||
| } |
There was a problem hiding this comment.
Binary-expression capture is still order-dependent.
cap_array_index_value returns only one identifier, so keys like index + "-" + item can still miss the index when another identifier appears later. That leaves a false-negative path similar to the one this PR fixes for template literals.
💡 Proposed patch
- AnyJsExpression::JsBinaryExpression(binary_expression) => {
- let mut capture_array_index = None;
- let _ = cap_array_index_value(&binary_expression, &mut capture_array_index);
- if let Some(reference) = capture_array_index {
- candidate_references.push(reference);
- }
- }
+ AnyJsExpression::JsBinaryExpression(binary_expression) => {
+ let mut captured_references = Vec::new();
+ let _ = cap_array_index_values(&binary_expression, &mut captured_references);
+ candidate_references.extend(captured_references);
+ }-fn cap_array_index_value(
+fn cap_array_index_values(
binary_expression: &JsBinaryExpression,
- capture_array_index: &mut Option<JsReferenceIdentifier>,
+ captured_references: &mut Vec<JsReferenceIdentifier>,
) -> Option<()> {
@@
- if let Some(left_binary) = left.as_js_binary_expression() {
- cap_array_index_value(left_binary, capture_array_index);
+ if let Some(left_binary) = left.as_js_binary_expression() {
+ cap_array_index_values(left_binary, captured_references);
};
@@
- if let Some(right_binary) = right.as_js_binary_expression() {
- cap_array_index_value(right_binary, capture_array_index);
+ if let Some(right_binary) = right.as_js_binary_expression() {
+ cap_array_index_values(right_binary, captured_references);
};
@@
- if let Some(left_expression) = left.as_js_identifier_expression() {
- *capture_array_index = left_expression.name().ok();
+ if let Some(left_expression) = left.as_js_identifier_expression() {
+ if let Ok(reference) = left_expression.name() {
+ captured_references.push(reference);
+ }
};
@@
- if let Some(right_expression) = right.as_js_identifier_expression() {
- *capture_array_index = right_expression.name().ok();
+ if let Some(right_expression) = right.as_js_identifier_expression() {
+ if let Ok(reference) = right_expression.name() {
+ captured_references.push(reference);
+ }
};📝 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.
| AnyJsExpression::JsBinaryExpression(binary_expression) => { | |
| let mut capture_array_index = None; | |
| let _ = cap_array_index_value(&binary_expression, &mut capture_array_index); | |
| if let Some(reference) = capture_array_index { | |
| candidate_references.push(reference); | |
| } | |
| AnyJsExpression::JsBinaryExpression(binary_expression) => { | |
| let mut captured_references = Vec::new(); | |
| let _ = cap_array_index_values(&binary_expression, &mut captured_references); | |
| candidate_references.extend(captured_references); | |
| } |
🤖 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 172 - 177, cap_array_index_value only returns a single identifier, so when
handling AnyJsExpression::JsBinaryExpression you miss index identifiers if other
identifiers appear later; update the logic to collect all identifier captures
from the binary expression (not just one) and push each into
candidate_references. Concretely: change cap_array_index_value (or add a new
helper) to traverse the binary expression tree and return a Vec of captures
(instead of a single Option), then in the AnyJsExpression::JsBinaryExpression
arm iterate over those captures and push each reference into
candidate_references (references: AnyJsExpression::JsBinaryExpression,
cap_array_index_value, capture_array_index, candidate_references).
There was a problem hiding this comment.
@slegarraga please address this comment. Is that correct?
| let reference = node.as_js_expression()?; | ||
|
|
||
| let mut capture_array_index = None; | ||
| let mut candidate_references: Vec<JsReferenceIdentifier> = Vec::new(); |
There was a problem hiding this comment.
| let mut candidate_references: Vec<JsReferenceIdentifier> = Vec::new(); | |
| let mut candidate_references = Vec::new(); |
I think you don't need an explicit type
| AnyJsExpression::JsBinaryExpression(binary_expression) => { | ||
| let mut capture_array_index = None; | ||
| let _ = cap_array_index_value(&binary_expression, &mut capture_array_index); | ||
| if let Some(reference) = capture_array_index { | ||
| candidate_references.push(reference); | ||
| } |
There was a problem hiding this comment.
@slegarraga please address this comment. Is that correct?
| .and_then(|arguments| arguments.parent::<JsCallExpression>())?; | ||
| is_array_method_index(¶m, &call_expr) | ||
| }) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
| .unwrap_or(false) | |
| .unwrap_or_default() |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Closing in favor of #8968 |
3 participants
Summary
Fixes #8812
The
noArrayIndexKeyrule had a false negative when the array index appeared before other interpolations in a template literal. For example:Root cause
The template expression handler iterated over all template elements but kept overwriting
capture_array_indexwith each one. Only the last interpolation was checked. Whenindexappeared first, it was overwritten byitem, which doesn't resolve to an array index parameter.Fix
Instead of keeping only the last identifier, collect all identifier expressions from template elements and check each one against the array index parameter binding. The first match is used.
Test Plan
${index}-${item}ordering ininvalid.jsx