fix(rome_js_analyze): improve the detection of ReactDOM.render call…

…s in `noRenderReturnValue` (#3626)

* fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue`

* address PR review

crates/rome_js_analyze/src/react.rs

@@ -55,7 +55,8 @@ impl ReactCreateElementCall {
         model: &SemanticModel,
     ) -> Option<Self> {
         let callee = call_expression.callee().ok()?;
-        let is_react_create_element = is_react_call_api(&callee, model, "createElement")?;
+        let is_react_create_element =
+            is_react_call_api(&callee, model, ReactLibrary::React, "createElement")?;
 
         if is_react_create_element {
             let arguments = call_expression.arguments().ok()?.args();
@@ -156,7 +157,8 @@ impl ReactCloneElementCall {
         model: &SemanticModel,
     ) -> Option<Self> {
         let callee = call_expression.callee().ok()?;
-        let is_react_clone_element = is_react_call_api(&callee, model, "cloneElement")?;
+        let is_react_clone_element =
+            is_react_call_api(&callee, model, ReactLibrary::React, "cloneElement")?;
 
         if is_react_clone_element {
             let arguments = call_expression.arguments().ok()?.args();
@@ -216,6 +218,28 @@ impl ReactApiCall for ReactCloneElementCall {
     }
 }
 
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum ReactLibrary {
+    React,
+    ReactDOM,
+}
+
+impl ReactLibrary {
+    const fn import_name(self) -> &'static str {
+        match self {
+            ReactLibrary::React => "react",
+            ReactLibrary::ReactDOM => "react-dom",
+        }
+    }
+
+    const fn global_name(self) -> &'static str {
+        match self {
+            ReactLibrary::React => "React",
+            ReactLibrary::ReactDOM => "ReactDOM",
+        }
+    }
+}
+
 /// List of valid [`React` API]
 ///
 /// [`React` API]: https://reactjs.org/docs/react-api.html
@@ -243,10 +267,13 @@ const VALID_REACT_API: [&str; 14] = [
 pub(crate) fn is_react_call_api(
     expression: &JsAnyExpression,
     model: &SemanticModel,
+    lib: ReactLibrary,
     api_name: &str,
 ) -> Option<bool> {
-    // we bail straight away if the API doesn't exists in React
-    debug_assert!(VALID_REACT_API.contains(&api_name));
+    if matches!(lib, ReactLibrary::React) {
+        // 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) => {
@@ -269,12 +296,12 @@ pub(crate) fn is_react_call_api(
                         .ancestors()
                         .find_map(|ancestor| JsImport::cast_ref(&ancestor))
                     {
-                        js_import.source_is("react").ok()?
+                        js_import.source_is(lib.import_name()).ok()?
                     } else {
                         false
                     }
                 }
-                None => identifier.has_name("React"),
+                None => identifier.has_name(lib.global_name()),
             }
         }
 
@@ -283,7 +310,7 @@ pub(crate) fn is_react_call_api(
 
             model
                 .declaration(&name)
-                .and_then(|binding| is_react_export(binding, api_name))
+                .and_then(|binding| is_react_export(binding, lib, api_name))
                 .unwrap_or(false)
         }
         _ => false,
@@ -335,7 +362,7 @@ pub(crate) fn jsx_reference_identifier_is_fragment(
     model: &SemanticModel,
 ) -> Option<bool> {
     match model.declaration(name) {
-        Some(reference) => is_react_export(reference, "Fragment"),
+        Some(reference) => is_react_export(reference, ReactLibrary::React, "Fragment"),
         None => {
             let value_token = name.value_token().ok()?;
             let is_fragment = value_token.text_trimmed() == "Fragment";
@@ -344,7 +371,7 @@ pub(crate) fn jsx_reference_identifier_is_fragment(
     }
 }
 
-fn is_react_export(binding: Binding, name: &str) -> Option<bool> {
+fn is_react_export(binding: Binding, lib: ReactLibrary, name: &str) -> Option<bool> {
     let ident = JsIdentifierBinding::cast_ref(binding.syntax())?;
     let import_specifier = ident.parent::<JsAnyNamedImportSpecifier>()?;
     let name_token = match &import_specifier {
@@ -366,5 +393,5 @@ fn is_react_export(binding: Binding, name: &str) -> Option<bool> {
     let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
     let import = import_clause.parent::<JsImport>()?;
 
-    import.source_is("react").ok()
+    import.source_is(lib.import_name()).ok()
 }

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

@@ -1,4 +1,4 @@
-use crate::react::{is_react_call_api, ReactApiCall, ReactCloneElementCall};
+use crate::react::{is_react_call_api, ReactApiCall, ReactCloneElementCall, ReactLibrary};
 use crate::semantic_services::Semantic;
 use rome_analyze::context::RuleContext;
 use rome_analyze::{declare_rule, Rule, RuleDiagnostic};
@@ -333,7 +333,7 @@ fn find_react_children_function_argument(
     let object = member_expression.object().ok()?;
 
     // React.Children.forEach/map or Children.forEach/map
-    if is_react_call_api(&object, model, "Children")? {
+    if is_react_call_api(&object, model, ReactLibrary::React, "Children")? {
         let arguments = call_expression.arguments().ok()?;
         let arguments = arguments.args();
         let mut arguments = arguments.into_iter();

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

@@ -1,14 +1,10 @@
+use crate::react::{is_react_call_api, ReactLibrary};
 use crate::semantic_services::Semantic;
 use rome_analyze::context::RuleContext;
 use rome_analyze::{declare_rule, Rule, RuleDiagnostic};
 use rome_console::markup;
-use rome_js_semantic::SemanticModel;
-use rome_js_syntax::JsSyntaxKind::JS_IMPORT;
-use rome_js_syntax::{
-    JsAnyExpression, JsCallExpression, JsExpressionStatement, JsIdentifierBinding,
-    JsIdentifierExpression, JsStaticMemberExpression,
-};
-use rome_rowan::{declare_node_union, AstNode};
+use rome_js_syntax::{JsCallExpression, JsExpressionStatement};
+use rome_rowan::AstNode;
 
 declare_rule! {
     /// Prevent the usage of the return value of `React.render`.
@@ -50,16 +46,7 @@ impl Rule for NoRenderReturnValue {
         let node = ctx.query();
         let callee = node.callee().ok()?;
         let model = ctx.model();
-        let is_react_render = match callee {
-            JsAnyExpression::JsStaticMemberExpression(static_member) => {
-                is_react_render(PossibleReactRender::from(static_member), model)
-            }
-            JsAnyExpression::JsIdentifierExpression(identifier_expression) => {
-                is_react_render(PossibleReactRender::from(identifier_expression), model)
-            }
-            _ => return None,
-        }?;
-        if is_react_render {
+        if is_react_call_api(&callee, model, ReactLibrary::ReactDOM, "render")? {
             let parent = node.syntax().parent()?;
 
             if !JsExpressionStatement::can_cast(parent.kind()) {
@@ -85,60 +72,3 @@ Check the "<Hyperlink href="https://facebook.github.io/react/docs/react-dom.html
         )
     }
 }
-
-declare_node_union! {
-    pub(crate) PossibleReactRender = JsStaticMemberExpression | JsIdentifierExpression
-}
-
-fn is_react_render(node: PossibleReactRender, model: &SemanticModel) -> Option<bool> {
-    let result = match node {
-        PossibleReactRender::JsStaticMemberExpression(node) => {
-            let object = node.object().ok()?;
-            let member = node.member().ok()?;
-            let member = member.as_js_name()?;
-            let identifier = object.as_js_identifier_expression()?.name().ok()?;
-
-            let maybe_from_react = identifier.syntax().text_trimmed() == "ReactDOM"
-                && member.syntax().text_trimmed() == "render";
-
-            if maybe_from_react {
-                let identifier_binding = model.declaration(&identifier);
-                if let Some(binding_identifier) = identifier_binding {
-                    let binding_identifier =
-                        JsIdentifierBinding::cast_ref(binding_identifier.syntax())?;
-                    for ancestor in binding_identifier.syntax().ancestors() {
-                        if ancestor.kind() == JS_IMPORT {
-                            return Some(
-                                binding_identifier.syntax().text_trimmed()
-                                    == identifier.syntax().text_trimmed(),
-                            );
-                        }
-                    }
-                }
-            }
-            maybe_from_react
-        }
-        PossibleReactRender::JsIdentifierExpression(identifier) => {
-            let maybe_react_render = identifier.syntax().text_trimmed() == "render";
-            let name = identifier.name().ok()?;
-            if maybe_react_render {
-                let declaration = model.declaration(&name);
-                if let Some(declaration) = declaration {
-                    let identifier_binding = JsIdentifierBinding::cast_ref(declaration.syntax())?;
-                    for ancestor in identifier_binding.syntax().ancestors() {
-                        if ancestor.kind() == JS_IMPORT {
-                            return Some(
-                                identifier_binding.syntax().text_trimmed()
-                                    == identifier.syntax().text_trimmed(),
-                            );
-                        }
-                    }
-                }
-            }
-
-            maybe_react_render
-        }
-    };
-
-    Some(result)
-}