fix(lint/useJsxKeyInIterable): handle fragments more comprehensively

biomejs · May 6, 2024 · cc6d7f2 · cc6d7f2
1 parent a8b3d8c
commit cc6d7f2
Show file tree

Hide file tree

Showing 5 changed files with 185 additions and 16 deletions.
diff --git a/crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs b/crates/biome_js_analyze/src/lint/correctness/use_jsx_key_in_iterable.rs
@@ -5,9 +5,7 @@ use biome_analyze::{RuleSource, RuleSourceKind};
 use biome_console::markup;
 use biome_js_semantic::SemanticModel;
 use biome_js_syntax::{
-    AnyJsExpression, AnyJsFunctionBody, AnyJsMemberExpression, AnyJsObjectMember, AnyJsxAttribute,
-    AnyJsxTag, JsArrayExpression, JsCallExpression, JsObjectExpression, JsxAttributeList,
-    JsxExpressionChild, JsxTagExpression,
+    AnyJsExpression, AnyJsFunctionBody, AnyJsMemberExpression, AnyJsObjectMember, AnyJsxAttribute, AnyJsxChild, JsArrayExpression, JsCallExpression, JsObjectExpression, JsxAttributeList, JsxExpressionChild, JsxTagExpression
 };
 use biome_rowan::{declare_node_union, AstNode, AstSeparatedList, TextRange};
 
@@ -221,24 +219,24 @@ fn handle_potential_react_component(
         }
         if let Some(node) = ReactComponentExpression::cast_ref(node.syntax()) {
             let range = handle_react_component(node, model)?;
-            Some(vec![range])
+            Some(range)
         } else {
             None
         }
     } else {
         let range =
             handle_react_component(ReactComponentExpression::cast_ref(node.syntax())?, model)?;
-        Some(vec![range])
+        Some(range)
     }
 }
 
 fn handle_react_component(
     node: ReactComponentExpression,
     model: &SemanticModel,
-) -> Option<TextRange> {
+) -> Option<Vec<TextRange>> {
     match node {
-        ReactComponentExpression::JsxTagExpression(node) => handle_jsx_tag(&node),
-        ReactComponentExpression::JsCallExpression(node) => handle_react_non_jsx(&node, model),
+        ReactComponentExpression::JsxTagExpression(node) => handle_jsx_tag(&node, model),
+        ReactComponentExpression::JsCallExpression(node) => handle_react_non_jsx(&node, model).map(|r| vec![r]),
     }
 }
 
@@ -249,25 +247,61 @@ fn handle_react_component(
 /// ```js
 /// <Hello></Hello>
 /// ```
-fn handle_jsx_tag(node: &JsxTagExpression) -> Option<TextRange> {
+fn handle_jsx_tag(node: &JsxTagExpression, model: &SemanticModel) -> Option<Vec<TextRange>> {
     let tag = node.tag().ok()?;
-    match tag {
-        AnyJsxTag::JsxElement(node) => {
+    let tag = AnyJsxChild::cast_ref(&tag.syntax())?;
+    handle_jsx_child(&tag, model)
+}
+
+fn handle_jsx_child(node: &AnyJsxChild, model: &SemanticModel) -> Option<Vec<TextRange>> {
+    match node {
+        AnyJsxChild::JsxElement(node) => {
             let open_node = node.opening_element().ok()?;
             if !has_key_attribute(&open_node.attributes()) {
-                Some(open_node.range())
+                Some(vec![open_node.range()])
             } else {
                 None
             }
         }
-        AnyJsxTag::JsxSelfClosingElement(node) => {
+        AnyJsxChild::JsxSelfClosingElement(node) => {
             if !has_key_attribute(&node.attributes()) {
-                Some(node.range())
+                Some(vec![node.range()])
+            } else {
+                None
+            }
+        }
+        AnyJsxChild::JsxExpressionChild(node) => {
+            let expr = node.expression()?;
+            handle_potential_react_component(expr, model, true)
+        }
+        AnyJsxChild::JsxFragment(node) => {
+            let mut any_tags_found = false;
+            let ranges: Vec<_> = node.children().into_iter().filter_map(|child| {
+                match &child {
+                    AnyJsxChild::JsxElement(_) | AnyJsxChild::JsxSelfClosingElement(_) => {
+                        any_tags_found = true;
+                    }
+                    // HACK: don't flag the entire fragment if there's a conditional expression
+                    AnyJsxChild::JsxExpressionChild(node) => {
+                        let expr = node.expression()?;
+                        if expr.as_js_conditional_expression().is_some() {
+                            any_tags_found = true;
+                        }
+                    }
+                    _ => {}
+                }
+                handle_jsx_child(&child, model)
+            }).flatten().collect();
+            if !ranges.is_empty() {
+                Some(ranges)
+            } else if !any_tags_found {
+                Some(vec![node.range()])
             } else {
                 None
             }
         }
-        AnyJsxTag::JsxFragment(node) => Some(node.range()),
+        AnyJsxChild::JsxText(_) => None,
+        AnyJsxChild::JsxSpreadChild(_) => None,
     }
 }
 

diff --git a/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx b/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx
@@ -38,4 +38,12 @@ React.Children.map(c => React.cloneElement(c));
 
 (<h1>{data.map(c => (<h1></h1>))}</h1>)
 
-(<h1>{data.map(c => {return (<h1></h1>)})}</h1>)
+(<h1>{data.map(c => {return (<h1></h1>)})}</h1>)
+
+[].map((item) => {
+	return <><div /><div>{item}</div></>;
+});
+
+[].map((item) => {
+	return <>{item.condition ? <div /> : <div>foo</div>}</>;
+});
diff --git a/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx.snap b/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/invalid.jsx.snap
@@ -45,6 +45,15 @@ React.Children.map(c => React.cloneElement(c));
 (<h1>{data.map(c => (<h1></h1>))}</h1>)
 
 (<h1>{data.map(c => {return (<h1></h1>)})}</h1>)
+
+[].map((item) => {
+	return <><div /><div>{item}</div></>;
+});
+
+[].map((item) => {
+	return <>{item.condition ? <div /> : <div>foo</div>}</>;
+});
+
 ```
 
 # Diagnostics
@@ -425,6 +434,42 @@ invalid.jsx:27:43 lint/correctness/useJsxKeyInIterable ━━━━━━━━
   i Check the React documentation. 
   
 
+```
+
+```
+invalid.jsx:44:11 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Missing key property for this element in iterable.
+  
+    43 │ [].map((item) => {
+  > 44 │ 	return <><div /><div>{item}</div></>;
+       │ 	         ^^^^^^^
+    45 │ });
+    46 │ 
+  
+  i The order of the items may change, and having a key can help React identify which item was moved.
+  
+  i Check the React documentation. 
+  
+
+```
+
+```
+invalid.jsx:44:18 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Missing key property for this element in iterable.
+  
+    43 │ [].map((item) => {
+  > 44 │ 	return <><div /><div>{item}</div></>;
+       │ 	                ^^^^^
+    45 │ });
+    46 │ 
+  
+  i The order of the items may change, and having a key can help React identify which item was moved.
+  
+  i Check the React documentation. 
+  
+
 ```
 
 ```
@@ -588,6 +633,44 @@ invalid.jsx:41:30 lint/correctness/useJsxKeyInIterable ━━━━━━━━
     40 │ 
   > 41 │ (<h1>{data.map(c => {return (<h1></h1>)})}</h1>)
        │                              ^^^^
+    42 │ 
+    43 │ [].map((item) => {
+  
+  i The order of the items may change, and having a key can help React identify which item was moved.
+  
+  i Check the React documentation. 
+  
+
+```
+
+```
+invalid.jsx:48:29 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Missing key property for this element in iterable.
+  
+    47 │ [].map((item) => {
+  > 48 │ 	return <>{item.condition ? <div /> : <div>foo</div>}</>;
+       │ 	                           ^^^^^^^
+    49 │ });
+    50 │ 
+  
+  i The order of the items may change, and having a key can help React identify which item was moved.
+  
+  i Check the React documentation. 
+  
+
+```
+
+```
+invalid.jsx:48:39 lint/correctness/useJsxKeyInIterable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Missing key property for this element in iterable.
+  
+    47 │ [].map((item) => {
+  > 48 │ 	return <>{item.condition ? <div /> : <div>foo</div>}</>;
+       │ 	                                     ^^^^^
+    49 │ });
+    50 │ 
   
   i The order of the items may change, and having a key can help React identify which item was moved.
   

diff --git a/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx b/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx
@@ -54,3 +54,25 @@ React.Children.map(c => React.cloneElement(c, {key: c}));
 <>{data.reduce((a, b) => a > b ? a : b, 0)}</>
 
 <>{data.map(a => a > 4 ? <h1 key={a}>{a}</h1> : <h2 key={a}>{a}</h2>)}</>
+
+[].map((item) => {
+	return <><div  key={item.id} /><div key={item.id}>{item}</div></>;
+});
+
+[].map((item) => {
+	const div = <div key={item.id} />;
+	return <>{div}<div key={item.id}>{item}</div></>;
+});
+
+[].map((item) => {
+	return <><div key={item.id}>foo</div></>;
+});
+
+[].map((item) => {
+	return <>{item.condition ? <div key={item.id} /> : <div key={item.id}>foo</div>}</>;
+});
+
+[].map((item) => {
+	const div = <div key={item.id} />;
+	return <>{item.condition ? div : <div key={item.id}>{div}</div>}</>;
+});
diff --git a/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx.snap b/crates/biome_js_analyze/tests/specs/correctness/useJsxKeyInIterable/valid.jsx.snap
@@ -61,4 +61,26 @@ React.Children.map(c => React.cloneElement(c, {key: c}));
 
 <>{data.map(a => a > 4 ? <h1 key={a}>{a}</h1> : <h2 key={a}>{a}</h2>)}</>
 
+[].map((item) => {
+	return <><div  key={item.id} /><div key={item.id}>{item}</div></>;
+});
+
+[].map((item) => {
+	const div = <div key={item.id} />;
+	return <>{div}<div key={item.id}>{item}</div></>;
+});
+
+[].map((item) => {
+	return <><div key={item.id}>foo</div></>;
+});
+
+[].map((item) => {
+	return <>{item.condition ? <div key={item.id} /> : <div key={item.id}>foo</div>}</>;
+});
+
+[].map((item) => {
+	const div = <div key={item.id} />;
+	return <>{item.condition ? div : <div key={item.id}>{div}</div>}</>;
+});
+
 ```