Skip to content

Commit

Permalink
fix(lint/noArrayIndexKey): false negative in template literals (biome…
Browse files Browse the repository at this point in the history 
…js#1586)
  • Loading branch information
@vasucp1207 @ematipico
vasucp1207 authored and ematipico committed Jan 24, 2024
1 parent a15397c commit e39e1b1
Show file tree
Hide file tree
Showing 8 changed files with 553 additions and 270 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom

#### Bug fixes

- Fix [#1575](https://github.com/biomejs/biome/issues/1575). [noArrayIndexKey](https://biomejs.dev/linter/rules/no-array-index-key/) now captures array index value inside template literals and with string concatination. Contributed by @vasucp1207

### Parser


Expand Down
104 changes: 90 additions & 14 deletions crates/biome_js_analyze/src/semantic_analyzers/suspicious/no_array_index_key.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ use biome_analyze::context::RuleContext;
use biome_analyze::{declare_rule, Rule, RuleDiagnostic, RuleSource};
use biome_console::markup;
use biome_js_syntax::{
AnyJsFunction, AnyJsMemberExpression, JsCallArgumentList, JsCallArguments, JsCallExpression,
JsFormalParameter, JsObjectExpression, JsObjectMemberList, JsParameterList, JsParameters,
JsPropertyObjectMember, JsReferenceIdentifier, JsxAttribute,
AnyJsExpression, AnyJsFunction, AnyJsMemberExpression, AnyJsTemplateElement,
JsBinaryExpression, JsCallArgumentList, JsCallArguments, JsCallExpression, JsFormalParameter,
JsObjectExpression, JsObjectMemberList, JsParameterList, JsParameters, JsPropertyObjectMember,
JsReferenceIdentifier, JsxAttribute,
};
use biome_rowan::{declare_node_union, AstNode, TextRange};

Expand Down Expand Up @@ -36,6 +37,32 @@ declare_rule! {
/// React.cloneElement(child, { key: index })
/// ))
/// ```
///
/// ```jsx,expect_diagnostic
/// something.forEach((Element, index) => {
/// <Component key={`test-key-${index}`} >foo</Component>
/// });
/// ```
///
/// ```jsx,expect_diagnostic
/// something.forEach((Element, index) => {
/// <Component key={"test" + index} >foo</Component>
/// });
/// ```
///
/// ### Valid
/// ```jsx
/// something.forEach((item) => {
/// <Component key={item.id} >foo</Component>
/// });
/// ```
///
/// ```jsx
/// something.forEach((item) => {
/// <Component key={item.baz.foo} >foo</Component>
/// });
/// ```
///
pub(crate) NoArrayIndexKey {
version: "1.0.0",
name: "noArrayIndexKey",
Expand Down Expand Up @@ -71,24 +98,18 @@ impl NoArrayIndexKeyQuery {
}

/// Extracts the reference from the possible invalid prop
fn as_js_reference_identifier(&self) -> Option<JsReferenceIdentifier> {
fn as_js_expression(&self) -> Option<AnyJsExpression> {
match self {
NoArrayIndexKeyQuery::JsxAttribute(attribute) => attribute
.initializer()?
.value()
.ok()?
.as_jsx_expression_attribute_value()?
.expression()
.ok()?
.as_js_identifier_expression()?
.name()
.ok(),
NoArrayIndexKeyQuery::JsPropertyObjectMember(object_member) => object_member
.value()
.ok()?
.as_js_identifier_expression()?
.name()
.ok(),
NoArrayIndexKeyQuery::JsPropertyObjectMember(object_member) => {
object_member.value().ok()
}
}
}
}
Expand All @@ -114,7 +135,35 @@ impl Rule for NoArrayIndexKey {
}

let model = ctx.model();
let reference = node.as_js_reference_identifier()?;
let reference = node.as_js_expression()?;

let mut capture_array_index = None;

match reference {
AnyJsExpression::JsIdentifierExpression(identifier_expression) => {
capture_array_index = Some(identifier_expression.name().ok()?);
}
AnyJsExpression::JsTemplateExpression(template_expression) => {
let template_elements = template_expression.elements();
for element in template_elements {
if let AnyJsTemplateElement::JsTemplateElement(template_element) = element {
let cap_index_value = template_element
.expression()
.ok()?
.as_js_identifier_expression()?
.name()
.ok();
capture_array_index = cap_index_value;
}
}
}
AnyJsExpression::JsBinaryExpression(binary_expression) => {
let _ = cap_array_index_value(&binary_expression, &mut capture_array_index);
}
_ => {}
};

let reference = capture_array_index?;

// Given the reference identifier retrieved from the key property,
// find the declaration and ensure it resolves to the parameter of a function,
Expand Down Expand Up @@ -227,3 +276,30 @@ fn is_array_method_index(
None
}
}

fn cap_array_index_value(
binary_expression: &JsBinaryExpression,
capture_array_index: &mut Option<JsReferenceIdentifier>,
) -> Option<()> {
let left = binary_expression.left().ok()?;
let right = binary_expression.right().ok()?;

// recursive call if left or right again are binary_expressions
if let Some(left_binary) = left.as_js_binary_expression() {
cap_array_index_value(left_binary, capture_array_index);
};

if let Some(right_binary) = right.as_js_binary_expression() {
cap_array_index_value(right_binary, capture_array_index);
};

if let Some(left_expression) = left.as_js_identifier_expression() {
*capture_array_index = left_expression.name().ok();
};

if let Some(right_expression) = right.as_js_identifier_expression() {
*capture_array_index = right_expression.name().ok();
};

Some(())
}
108 changes: 66 additions & 42 deletions crates/biome_js_analyze/tests/specs/suspicious/noArrayIndexKey/invalid.jsx
View file
Original file line number Diff line number Diff line change
@@ -1,110 +1,134 @@
import { Children, cloneElement } from "react";

something.forEach((Element, index) => {
<Component key={index}>foo</Component>;
<Component key={index}>foo</Component>;
});
something.forEach((element, index, array) => {
<Component key={index}>foo</Component>;
<Component key={index}>foo</Component>;
});
things.filter((thing, index) => {
otherThings.push(<Hello key={index}>foo</Hello>);
otherThings.push(<Hello key={index}>foo</Hello>);
});

something.forEach((Element, index) => {
<Component key={index} />;
<Component key={index} />;
});
something.forEach((element, index, array) => {
<Component key={index} />;
<Component key={index} />;
});
things.filter((thing, index) => {
otherThings.push(<Hello key={index} />);
otherThings.push(<Hello key={index} />);
});
things.reduce(
(collection, thing, index) => collection.concat(<Hello key={index} />),
[]
(collection, thing, index) => collection.concat(<Hello key={index} />),
[]
);

React.Children.map(this.props.children, (child, index) =>
React.cloneElement(child, { key: index })
React.cloneElement(child, { key: index })
);

React.Children.forEach(this.props.children, function (child, index) {
return React.cloneElement(child, { key: index });
return React.cloneElement(child, { key: index });
});

Children.map(this.props.children, (child, index) =>
cloneElement(child, { key: index })
cloneElement(child, { key: index })
);

Children.forEach(this.props.children, function (child, index) {
return cloneElement(child, { key: index });
return cloneElement(child, { key: index });
});

Children.forEach(this.props.children, function (child, index) {
const foo = cloneElement(child, { key: index });
return foo;
const foo = cloneElement(child, { key: index });
return foo;
});

function Test(props) {
return Children.map(props.children, function (child, index) {
return cloneElement(child, { key: index });
});
return Children.map(props.children, function (child, index) {
return cloneElement(child, { key: index });
});
}

things.map((thing, index) => React.cloneElement(thing, { key: index }));

things.flatMap((thing, index) => {
return <Component key={index} />;
return <Component key={index} />;
});

Array.from(things, (thing, index) => {
return <Component key={index} />;
return <Component key={index} />;
});

const mapping = {
foo: () => things.map((_, index) => <Component key={index} />),
foo: () => things.map((_, index) => <Component key={index} />),
};

class A extends React.Component {
renderThings = () => things.map((_, index) => <Component key={index} />);
renderThings = () => things.map((_, index) => <Component key={index} />);
}

const Component1 = () => things.map((_, index) => <Component key={index} />);

const Component2 = () => things.map((_, index) => <Component key={index} />);

function Component3() {
return things.map((_, index) => <Component key={index} />);
return things.map((_, index) => <Component key={index} />);
}

function Component4() {
let elements = things.map((_, index) => <Component key={index} />);
if (condition) {
elements = others.map((_, index) => <Component key={index} />);
}
return elements;
let elements = things.map((_, index) => <Component key={index} />);
if (condition) {
elements = others.map((_, index) => <Component key={index} />);
}
return elements;
}

function Component5({ things }) {
const elements = useMemo(
() => things.map((_, index) => <Component key={index} />),
[things]
);
return elements;
const elements = useMemo(
() => things.map((_, index) => <Component key={index} />),
[things]
);
return elements;
}

function Component6({ things }) {
const elements = useMemo(
() => things.map((_, index) => <Component key={index} />),
[things]
);
return elements;
const elements = useMemo(
() => things.map((_, index) => <Component key={index} />),
[things]
);
return elements;
}

function Component7() {
return (
<HoC>
{({ things }) => things.map((_, index) => <Component key={index} />)}
</HoC>
);
return (
<HoC>
{({ things }) => things.map((_, index) => <Component key={index} />)}
</HoC>
);
}

function Component8() {
return (
<HoC>
{({ things }) => things.map((_, index) => <Component key={`test-key-${index}`} />)}
</HoC>
);
}

function Component9() {
return (
<HoC>
{({ things }) => things.map((_, index) => <Component key={"test" + index} />)}
</HoC>
);
}

function Component10() {
return (
<HoC>
{({ things }) => things.map((_, index) => <Component key={"test" + index + "test"} />)}
</HoC>
);
}
Loading

0 comments on commit e39e1b1

Please sign in to comment.