fix(rome_js_analyze): Verify method name of React API calls (#3619)

crates/rome_js_analyze/src/react.rs

@@ -2,7 +2,7 @@
 
 pub mod hooks;
 
-use rome_js_semantic::SemanticModel;
+use rome_js_semantic::{Binding, SemanticModel};
 use rome_js_syntax::{
     JsAnyCallArgument, JsAnyExpression, JsAnyNamedImportSpecifier, JsCallExpression,
     JsIdentifierBinding, JsImport, JsImportNamedClause, JsNamedImportSpecifierList,
@@ -247,46 +247,46 @@ pub(crate) fn is_react_call_api(
 ) -> Option<bool> {
     // we bail straight away if the API doesn't exists in React
     debug_assert!(VALID_REACT_API.contains(&api_name));
+
     Some(match expression {
         JsAnyExpression::JsStaticMemberExpression(node) => {
-            let object = node.object().ok()?;
             let member = node.member().ok()?;
             let member = member.as_js_name()?;
+
+            if member.value_token().ok()?.text_trimmed() != api_name {
+                return Some(false);
+            }
+
+            let object = node.object().ok()?;
             let identifier = object.as_js_identifier_expression()?.name().ok()?;
 
-            let mut maybe_from_react = identifier.syntax().text_trimmed() == "React"
-                && member.syntax().text_trimmed() == api_name;
-
-            if let Some(binding_identifier) = model.declaration(&identifier) {
-                let binding_identifier =
-                    JsIdentifierBinding::cast_ref(binding_identifier.syntax())?;
-                if let Some(js_import) = binding_identifier
-                    .syntax()
-                    .ancestors()
-                    .find_map(|ancestor| JsImport::cast_ref(&ancestor))
-                {
-                    maybe_from_react = js_import.source_is("react").ok()?;
+            match model.declaration(&identifier) {
+                Some(binding) => {
+                    let binding_identifier = JsIdentifierBinding::cast_ref(binding.syntax())?;
+
+                    if let Some(js_import) = binding_identifier
+                        .syntax()
+                        .ancestors()
+                        .find_map(|ancestor| JsImport::cast_ref(&ancestor))
+                    {
+                        js_import.source_is("react").ok()?
+                    } else {
+                        false
+                    }
                 }
+                None => identifier.has_name("React"),
             }
-            maybe_from_react
         }
+
         JsAnyExpression::JsIdentifierExpression(identifier) => {
             let name = identifier.name().ok()?;
-            let mut maybe_react = identifier.syntax().text_trimmed() == api_name;
-            if let Some(identifier_binding) = model.declaration(&name) {
-                let binding_identifier =
-                    JsIdentifierBinding::cast_ref(identifier_binding.syntax())?;
-                if let Some(js_import) = binding_identifier
-                    .syntax()
-                    .ancestors()
-                    .find_map(|ancestor| JsImport::cast_ref(&ancestor))
-                {
-                    maybe_react = js_import.source_is("react").ok()?;
-                }
-            }
-            maybe_react
+
+            model
+                .declaration(&name)
+                .and_then(|binding| is_react_export(binding, api_name))
+                .unwrap_or(false)
         }
-        _ => return None,
+        _ => false,
     })
 }
 
@@ -303,24 +303,25 @@ pub(crate) fn jsx_member_name_is_react_fragment(
     let object = member_name.object().ok()?;
     let member = member_name.member().ok()?;
     let object = object.as_jsx_reference_identifier()?;
-    let mut maybe_react_fragment = object.value_token().ok()?.text_trimmed() == "React"
-        && member.value_token().ok()?.text_trimmed() == "Fragment";
-    if let Some(reference) = model.declaration(object) {
-        if let Some(js_import) = reference
-            .syntax()
-            .ancestors()
-            .find_map(|ancestor| JsImport::cast_ref(&ancestor))
-        {
-            let source_is_react = js_import.source_is("react").ok()?;
-            maybe_react_fragment =
-                source_is_react && member.value_token().ok()?.text_trimmed() == "Fragment";
-        } else {
-            // `React.Fragment` is a binding but it doesn't come from the "react" package
-            maybe_react_fragment = false;
-        }
+
+    if member.value_token().ok()?.text_trimmed() != "Fragment" {
+        return Some(false);
     }
 
-    Some(maybe_react_fragment)
+    match model.declaration(object) {
+        Some(declaration) => {
+            if let Some(js_import) = declaration
+                .syntax()
+                .ancestors()
+                .find_map(|ancestor| JsImport::cast_ref(&ancestor))
+            {
+                js_import.source_is("react").ok()
+            } else {
+                Some(false)
+            }
+        }
+        None => Some(object.value_token().ok()?.text_trimmed() == "React"),
+    }
 }
 
 /// Checks if the node `JsxReferenceIdentifier` is a react fragment.
@@ -334,37 +335,36 @@ pub(crate) fn jsx_reference_identifier_is_fragment(
     model: &SemanticModel,
 ) -> Option<bool> {
     match model.declaration(name) {
-        Some(reference) => {
-            let ident = JsIdentifierBinding::cast_ref(reference.syntax())?;
-
-            let import_specifier = ident.parent::<JsAnyNamedImportSpecifier>()?;
-            let name_token = match &import_specifier {
-                JsAnyNamedImportSpecifier::JsNamedImportSpecifier(named_import) => {
-                    named_import.name().ok()?.value().ok()?
-                }
-                JsAnyNamedImportSpecifier::JsShorthandNamedImportSpecifier(_) => {
-                    ident.name_token().ok()?
-                }
-                JsAnyNamedImportSpecifier::JsUnknownNamedImportSpecifier(_) => {
-                    return None;
-                }
-            };
-
-            if name_token.text_trimmed() != "Fragment" {
-                return Some(false);
-            }
-
-            let import_specifier_list = import_specifier.parent::<JsNamedImportSpecifierList>()?;
-            let import_specifiers = import_specifier_list.parent::<JsNamedImportSpecifiers>()?;
-            let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
-            let import = import_clause.parent::<JsImport>()?;
-            import.source_is("react").ok()
-        }
-
+        Some(reference) => is_react_export(reference, "Fragment"),
         None => {
             let value_token = name.value_token().ok()?;
             let is_fragment = value_token.text_trimmed() == "Fragment";
             Some(is_fragment)
         }
     }
 }
+
+fn is_react_export(binding: Binding, name: &str) -> Option<bool> {
+    let ident = JsIdentifierBinding::cast_ref(binding.syntax())?;
+    let import_specifier = ident.parent::<JsAnyNamedImportSpecifier>()?;
+    let name_token = match &import_specifier {
+        JsAnyNamedImportSpecifier::JsNamedImportSpecifier(named_import) => {
+            named_import.name().ok()?.value().ok()?
+        }
+        JsAnyNamedImportSpecifier::JsShorthandNamedImportSpecifier(_) => ident.name_token().ok()?,
+        JsAnyNamedImportSpecifier::JsUnknownNamedImportSpecifier(_) => {
+            return Some(false);
+        }
+    };
+
+    if name_token.text_trimmed() != name {
+        return Some(false);
+    }
+
+    let import_specifier_list = import_specifier.parent::<JsNamedImportSpecifierList>()?;
+    let import_specifiers = import_specifier_list.parent::<JsNamedImportSpecifiers>()?;
+    let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
+    let import = import_clause.parent::<JsImport>()?;
+
+    import.source_is("react").ok()
+}

crates/rome_js_analyze/src/semantic_analyzers/correctness/no_array_index_key.rs

@@ -5,8 +5,8 @@ use rome_analyze::{declare_rule, Rule, RuleDiagnostic};
 use rome_console::markup;
 use rome_js_semantic::SemanticModel;
 use rome_js_syntax::{
-    JsArrowFunctionExpression, JsCallExpression, JsExpressionStatement, JsFunctionDeclaration,
-    JsFunctionExpression, JsIdentifierBinding, JsIdentifierExpression, JsMethodClassMember,
+    JsAnyCallArgument, JsArrowFunctionExpression, JsCallExpression, JsExpressionStatement,
+    JsFunctionDeclaration, JsFunctionExpression, JsIdentifierBinding, JsMethodClassMember,
     JsMethodObjectMember, JsParameterList, JsPropertyObjectMember, JsReferenceIdentifier,
     JsxAttribute, JsxOpeningElement, JsxSelfClosingElement,
 };
@@ -326,28 +326,24 @@ fn find_react_children_function_argument(
         "forEach" | "map"
     );
 
-    let object = member_expression.object().ok()?;
-
-    let mut is_react_children = false;
-    // case we have `Children`
-    if let Some(identifier) = JsIdentifierExpression::cast_ref(object.syntax()) {
-        if identifier.name().ok()?.value_token().ok()?.text_trimmed() == "Children" {
-            is_react_children = array_call;
-        }
-    } else {
-        // case we have `React.Children`
-        is_react_children = is_react_call_api(&object, model, "Children")? && array_call;
+    if !array_call {
+        return None;
     }
 
-    if is_react_children {
+    let object = member_expression.object().ok()?;
+
+    // React.Children.forEach/map or Children.forEach/map
+    if is_react_call_api(&object, model, "Children")? {
         let arguments = call_expression.arguments().ok()?;
         let arguments = arguments.args();
         let mut arguments = arguments.into_iter();
-        let _ = arguments.next()?.ok()?;
-        let second_argument = arguments.next()?.ok()?;
-        let second_argument = second_argument.as_js_any_expression()?;
 
-        FunctionLike::cast(second_argument.clone().into_syntax())
+        match (arguments.next(), arguments.next(), arguments.next()) {
+            (Some(_), Some(Ok(JsAnyCallArgument::JsAnyExpression(second_argument))), None) => {
+                FunctionLike::cast(second_argument.into_syntax())
+            }
+            _ => None,
+        }
     } else {
         None
     }

crates/rome_js_analyze/tests/specs/correctness/noArrayIndexKey.jsx

@@ -1,3 +1,5 @@
+import { Children, cloneElement } from "react";
+
 // invalid
 something.forEach((Element, index) => {
     <Component key={index} >foo</Component>
@@ -57,4 +59,4 @@ something.forEach((element, index) => {
 something.forEach((element, index) => {
     <Component key={index + "something"} />
 
-});
+});