Skip to content

Commit e6c512a

Browse files
DerTimoniusematipico
DerTimonius
and
ematipico
authored
fix(lint): mark whole fragment as incorrect if without key (#5773)
Co-authored-by: Emanuele Stoppa <[email protected]>
1 parent a7a0116 commit e6c512a

File tree

11 files changed

+1098
-367
lines changed

11 files changed

+1098
-367
lines changed

‎.changeset/silent-fans-battle.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@biomejs/biome": patch
3+
---
4+
5+
Updates the [`useJsxKeyInIterable`](https://biomejs.dev/linter/rules/use-jsx-key-in-iterable/) rule to more closely match the behavior of the ESLint plugin (e.g. mark the whole fragment as incorrect when no key is present). This also adds the option to check shorthand fragments (`<></>`)

‎crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs‎

Lines changed: 84 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ use crate::services::semantic::Semantic;
33
use biome_analyze::{Rule, RuleDiagnostic, RuleDomain, context::RuleContext, declare_lint_rule};
44
use biome_analyze::{RuleSource, RuleSourceKind};
55
use biome_console::markup;
6+
use biome_deserialize_macros::Deserializable;
67
use biome_diagnostics::Severity;
78
use biome_js_semantic::SemanticModel;
89
use biome_js_syntax::{
@@ -11,6 +12,7 @@ use biome_js_syntax::{
1112
JsxAttributeList, JsxExpressionChild, JsxTagExpression,
1213
};
1314
use biome_rowan::{AstNode, AstNodeList, AstSeparatedList, TextRange, declare_node_union};
15+
use serde::{Deserialize, Serialize};
1416

1517
declare_lint_rule! {
1618
/// Disallow missing key props in iterators/collection literals.
@@ -36,6 +38,24 @@ declare_lint_rule! {
3638
/// data.map((x) => <Hello key={x.id}>{x}</Hello>);
3739
/// ```
3840
///
41+
/// ## Options
42+
///
43+
/// ### checkShorthandFragments
44+
///
45+
/// React fragments can not only be created with `<React.Fragment>`, but also with shorthand
46+
/// fragments (`<></>`). To also check if those require a key, pass `true` to this option.
47+
///
48+
/// ```json,options
49+
/// {
50+
/// "options": {
51+
/// "checkShorthandFragments": true
52+
/// }
53+
/// }
54+
/// ```
55+
/// ```jsx,expect_diagnostic,use_options
56+
/// data.map((x) => <>{x}</>);
57+
/// ```
58+
///
3959
pub UseJsxKeyInIterable {
4060
version: "1.6.0",
4161
name: "useJsxKeyInIterable",
@@ -56,19 +76,30 @@ declare_node_union! {
5676
pub ReactComponentExpression = JsxTagExpression | JsCallExpression
5777
}
5878

79+
#[derive(Debug, Default, Clone, Serialize, Deserialize, Deserializable, Eq, PartialEq)]
80+
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
81+
#[serde(rename_all = "camelCase", deny_unknown_fields, default)]
82+
pub struct UseJsxKeyInIterableOptions {
83+
/// Set to `true` to check shorthand fragments (`<></>`)
84+
check_shorthand_fragments: bool,
85+
}
86+
5987
impl Rule for UseJsxKeyInIterable {
6088
type Query = Semantic<UseJsxKeyInIterableQuery>;
6189
type State = TextRange;
6290
type Signals = Box<[Self::State]>;
63-
type Options = ();
91+
type Options = UseJsxKeyInIterableOptions;
6492

6593
fn run(ctx: &RuleContext<Self>) -> Self::Signals {
6694
let node = ctx.query();
6795
let model = ctx.model();
96+
let options = ctx.options();
6897
match node {
69-
UseJsxKeyInIterableQuery::JsArrayExpression(node) => handle_collections(node, model),
98+
UseJsxKeyInIterableQuery::JsArrayExpression(node) => {
99+
handle_collections(node, model, options)
100+
}
70101
UseJsxKeyInIterableQuery::JsCallExpression(node) => {
71-
handle_iterators(node, model).unwrap_or_default()
102+
handle_iterators(node, model, options).unwrap_or_default()
72103
}
73104
}
74105
.into_boxed_slice()
@@ -98,7 +129,11 @@ impl Rule for UseJsxKeyInIterable {
98129
/// ```jsx
99130
/// [<h1></h1>, <h1></h1>]
100131
/// ```
101-
fn handle_collections(node: &JsArrayExpression, model: &SemanticModel) -> Vec<TextRange> {
132+
fn handle_collections(
133+
node: &JsArrayExpression,
134+
model: &SemanticModel,
135+
options: &UseJsxKeyInIterableOptions,
136+
) -> Vec<TextRange> {
102137
let is_inside_jsx = node.parent::<JsxExpressionChild>().is_some();
103138
node.elements()
104139
.iter()
@@ -107,7 +142,7 @@ fn handle_collections(node: &JsArrayExpression, model: &SemanticModel) -> Vec<Te
107142
// no need to handle spread case, if the spread argument is itself a list it
108143
// will be handled during list declaration
109144
let node = AnyJsExpression::cast(node.into_syntax())?;
110-
handle_potential_react_component(node, model, is_inside_jsx)
145+
handle_potential_react_component(node, model, is_inside_jsx, options)
111146
})
112147
.flatten()
113148
.collect()
@@ -120,7 +155,11 @@ fn handle_collections(node: &JsArrayExpression, model: &SemanticModel) -> Vec<Te
120155
/// ```jsx
121156
/// data.map(x => <h1>{x}</h1>)
122157
/// ```
123-
fn handle_iterators(node: &JsCallExpression, model: &SemanticModel) -> Option<Vec<TextRange>> {
158+
fn handle_iterators(
159+
node: &JsCallExpression,
160+
model: &SemanticModel,
161+
options: &UseJsxKeyInIterableOptions,
162+
) -> Option<Vec<TextRange>> {
124163
let callee = node.callee().ok()?;
125164
let member_expression = AnyJsMemberExpression::cast(callee.into_syntax())?;
126165
let arguments = node.arguments().ok()?;
@@ -164,16 +203,16 @@ fn handle_iterators(node: &JsCallExpression, model: &SemanticModel) -> Option<Ve
164203
match callback_argument {
165204
AnyJsExpression::JsFunctionExpression(callback) => {
166205
let body = callback.body().ok()?;
167-
Some(handle_function_body(&body, model, is_inside_jsx))
206+
Some(handle_function_body(&body, model, is_inside_jsx, options))
168207
}
169208
AnyJsExpression::JsArrowFunctionExpression(callback) => {
170209
let body = callback.body().ok()?;
171210
match body {
172211
AnyJsFunctionBody::AnyJsExpression(expr) => {
173-
handle_potential_react_component(expr, model, is_inside_jsx)
212+
handle_potential_react_component(expr, model, is_inside_jsx, options)
174213
}
175214
AnyJsFunctionBody::JsFunctionBody(body) => {
176-
Some(handle_function_body(&body, model, is_inside_jsx))
215+
Some(handle_function_body(&body, model, is_inside_jsx, options))
177216
}
178217
}
179218
}
@@ -186,6 +225,7 @@ fn handle_function_body(
186225
node: &JsFunctionBody,
187226
model: &SemanticModel,
188227
is_inside_jsx: bool,
228+
options: &UseJsxKeyInIterableOptions,
189229
) -> Vec<TextRange> {
190230
// if the return statement definitely has a key prop, don't need to check the rest of the function
191231
let return_statement = node
@@ -204,7 +244,7 @@ fn handle_function_body(
204244
.unwrap_or_default();
205245
let ranges = return_statement.and_then(|ret| {
206246
let returned_value = ret.argument()?;
207-
handle_potential_react_component(returned_value, model, is_inside_jsx)
247+
handle_potential_react_component(returned_value, model, is_inside_jsx, options)
208248
});
209249
if ranges.is_none() && is_return_component {
210250
return vec![];
@@ -222,14 +262,14 @@ fn handle_function_body(
222262
.filter_map(|declarator| {
223263
let decl = declarator.ok()?;
224264
let init = decl.initializer()?.expression().ok()?;
225-
handle_potential_react_component(init, model, is_inside_jsx)
265+
handle_potential_react_component(init, model, is_inside_jsx, options)
226266
})
227267
.flatten()
228268
.collect(),
229269
)
230270
} else if let Some(statement) = statement.as_js_return_statement() {
231271
let returned_value = statement.argument()?;
232-
handle_potential_react_component(returned_value, model, is_inside_jsx)
272+
handle_potential_react_component(returned_value, model, is_inside_jsx, options)
233273
} else {
234274
None
235275
}
@@ -242,14 +282,19 @@ fn handle_potential_react_component(
242282
node: AnyJsExpression,
243283
model: &SemanticModel,
244284
is_inside_jsx: bool,
285+
options: &UseJsxKeyInIterableOptions,
245286
) -> Option<Vec<TextRange>> {
246287
let node = unwrap_parenthesis(node)?;
247288

248289
if let AnyJsExpression::JsConditionalExpression(node) = node {
249-
let consequent =
250-
handle_potential_react_component(node.consequent().ok()?, model, is_inside_jsx);
290+
let consequent = handle_potential_react_component(
291+
node.consequent().ok()?,
292+
model,
293+
is_inside_jsx,
294+
options,
295+
);
251296
let alternate =
252-
handle_potential_react_component(node.alternate().ok()?, model, is_inside_jsx);
297+
handle_potential_react_component(node.alternate().ok()?, model, is_inside_jsx, options);
253298

254299
return match (consequent, alternate) {
255300
(Some(consequent), Some(alternate)) => Some([consequent, alternate].concat()),
@@ -261,24 +306,28 @@ fn handle_potential_react_component(
261306

262307
if is_inside_jsx {
263308
if let Some(node) = ReactComponentExpression::cast(node.into_syntax()) {
264-
let range = handle_react_component(node, model)?;
309+
let range = handle_react_component(node, model, options)?;
265310
Some(range)
266311
} else {
267312
None
268313
}
269314
} else {
270-
let range =
271-
handle_react_component(ReactComponentExpression::cast(node.into_syntax())?, model)?;
315+
let range = handle_react_component(
316+
ReactComponentExpression::cast(node.into_syntax())?,
317+
model,
318+
options,
319+
)?;
272320
Some(range)
273321
}
274322
}
275323

276324
fn handle_react_component(
277325
node: ReactComponentExpression,
278326
model: &SemanticModel,
327+
options: &UseJsxKeyInIterableOptions,
279328
) -> Option<Vec<TextRange>> {
280329
match node {
281-
ReactComponentExpression::JsxTagExpression(node) => handle_jsx_tag(&node, model),
330+
ReactComponentExpression::JsxTagExpression(node) => handle_jsx_tag(&node, model, options),
282331
ReactComponentExpression::JsCallExpression(node) => {
283332
handle_react_non_jsx(&node, model).map(|r| vec![r])
284333
}
@@ -292,13 +341,21 @@ fn handle_react_component(
292341
/// ```jsx
293342
/// <Hello></Hello>
294343
/// ```
295-
fn handle_jsx_tag(node: &JsxTagExpression, model: &SemanticModel) -> Option<Vec<TextRange>> {
344+
fn handle_jsx_tag(
345+
node: &JsxTagExpression,
346+
model: &SemanticModel,
347+
options: &UseJsxKeyInIterableOptions,
348+
) -> Option<Vec<TextRange>> {
296349
let tag = node.tag().ok()?;
297350
let tag = AnyJsxChild::cast(tag.into_syntax())?;
298-
handle_jsx_child(&tag, model)
351+
handle_jsx_child(&tag, model, options)
299352
}
300353

301-
fn handle_jsx_child(node: &AnyJsxChild, model: &SemanticModel) -> Option<Vec<TextRange>> {
354+
fn handle_jsx_child(
355+
node: &AnyJsxChild,
356+
model: &SemanticModel,
357+
options: &UseJsxKeyInIterableOptions,
358+
) -> Option<Vec<TextRange>> {
302359
let mut stack: Vec<AnyJsxChild> = vec![node.clone()];
303360
let mut ranges: Vec<TextRange> = vec![];
304361

@@ -317,26 +374,16 @@ fn handle_jsx_child(node: &AnyJsxChild, model: &SemanticModel) -> Option<Vec<Tex
317374
}
318375
AnyJsxChild::JsxExpressionChild(node) => {
319376
let expr = node.expression()?;
320-
if let Some(child_ranges) = handle_potential_react_component(expr, model, true) {
377+
if let Some(child_ranges) =
378+
handle_potential_react_component(expr, model, true, options)
379+
{
321380
ranges.extend(child_ranges);
322381
}
323382
}
324383
AnyJsxChild::JsxFragment(node) => {
325-
let has_any_tags = node.children().iter().any(|child| match &child {
326-
AnyJsxChild::JsxElement(_) | AnyJsxChild::JsxSelfClosingElement(_) => true,
327-
// HACK: don't flag the entire fragment if there's a conditional expression
328-
AnyJsxChild::JsxExpressionChild(node) => node
329-
.expression()
330-
.is_some_and(|n| n.as_js_conditional_expression().is_some()),
331-
_ => false,
332-
});
333-
334-
if !has_any_tags {
335-
ranges.push(node.range());
336-
break;
384+
if options.check_shorthand_fragments {
385+
ranges.push(node.range())
337386
}
338-
339-
stack.extend(node.children());
340387
}
341388
_ => {}
342389
}

‎crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@ import React from "react";
66

77
[<Hello />, xyz ? <Hello />: <Hello />, <Hello />];
88

9-
[<></>, <></>, <></>];
10-
119
data.map(x => <Hello>{x}</Hello>);
1210

1311
data.map(x => <>{x}</>);
@@ -44,14 +42,6 @@ React.Children.map(c => React.cloneElement(c));
4442
return item.condition ? <div /> : <div>foo</div>;
4543
});
4644

47-
[].map((item) => {
48-
return <><div /><div>{item}</div></>;
49-
});
50-
51-
[].map((item) => {
52-
return <>{item.condition ? <div /> : <div>foo</div>}</>;
53-
});
54-
5545
[].map((item) => {
5646
const x = 5;
5747
const div = <div>{x}</div>;
@@ -63,3 +53,5 @@ React.Children.map(c => React.cloneElement(c));
6353
const div = <div>{x}</div>;
6454
return div;
6555
});
56+
57+
data.map((item) => <React.Fragment><p>{item}</p></React.Fragment>)

0 commit comments

Comments
 (0)