fix(rome_js_analyze): useExhaustiveDependencies support function synt…

…ax, or React.use* hooks and more. (#4571)

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

CHANGELOG.md

@@ -57,12 +57,18 @@ parameter decorators:
   The _Rome_ formatter takes care of removing extra semicolons.
   Thus, there is no need for this rule.
 
+
 #### Other changes
 
 - [`noRedeclare`](https://docs.rome.tools/lint/rules/noredeclare/): allow redeclare of index signatures are in different type members [#4478](https://github.com/rome/tools/issues/4478)
 
 - Fix a crash in the [`NoParameterAssign`](https://docs.rome.tools/lint/rules/noparameterassign/) rule that occurred when there was a bogus binding. [#4323](https://github.com/rome/tools/issues/4323)
 
+- Fix [`useExhaustiveDependencies`](https://docs.rome.tools/lint/rules/useexhaustivedependencies/) rule in the following cases [#4330](https://github.com/rome/tools/issues/4330)
+  - when the first argument of hooks is a named function
+  - inside an export default function
+  - for React.use* hooks
+
 - Improve the diagnostic and the code action of [`useDefaultParameterLast`](https://docs.rome.tools/lint/rules/usedefaultparameterlast/).
 
   The diagnostic now reports the last required parameter which should precede optional and default parameters.

crates/rome_js_analyze/src/react.rs

@@ -142,7 +142,7 @@ impl ReactLibrary {
 /// List of valid [`React` API]
 ///
 /// [`React` API]: https://reactjs.org/docs/react-api.html
-const VALID_REACT_API: [&str; 14] = [
+const VALID_REACT_API: [&str; 29] = [
     "Component",
     "PureComponent",
     "memo",
@@ -157,6 +157,21 @@ const VALID_REACT_API: [&str; 14] = [
     "Suspense",
     "startTransition",
     "Children",
+    "useEffect",
+    "useLayoutEffect",
+    "useInsertionEffect",
+    "useCallback",
+    "useMemo",
+    "useImperativeHandle",
+    "useState",
+    "useContext",
+    "useReducer",
+    "useRef",
+    "useDebugValue",
+    "useDeferredValue",
+    "useTransition",
+    "useId",
+    "useSyncExternalStore",
 ];
 
 /// Checks if the current [JsCallExpression] is a potential [`React` API].

crates/rome_js_analyze/src/react/hooks.rs

@@ -1,9 +1,11 @@
+use crate::react::{is_react_call_api, ReactLibrary};
 use std::collections::{HashMap, HashSet};
 
-use rome_js_semantic::{Capture, ClosureExtensions, SemanticModel};
+use rome_js_semantic::{Capture, Closure, ClosureExtensions, SemanticModel};
 use rome_js_syntax::{
     binding_ext::AnyJsIdentifierBinding, AnyJsExpression, JsArrayBindingPattern,
-    JsArrayBindingPatternElementList, JsCallExpression, JsVariableDeclarator, TextRange,
+    JsArrayBindingPatternElementList, JsArrowFunctionExpression, JsCallExpression,
+    JsFunctionExpression, JsVariableDeclarator, TextRange,
 };
 use rome_rowan::AstNode;
 use serde::{Deserialize, Serialize};
@@ -15,15 +17,45 @@ pub(crate) struct ReactCallWithDependencyResult {
     pub(crate) dependencies_node: Option<AnyJsExpression>,
 }
 
+pub enum AnyJsFunctionExpression {
+    JsArrowFunctionExpression(JsArrowFunctionExpression),
+    JsFunctionExpression(JsFunctionExpression),
+}
+
+impl AnyJsFunctionExpression {
+    fn closure(&self, model: &SemanticModel) -> Closure {
+        match self {
+            Self::JsArrowFunctionExpression(arrow_function) => arrow_function.closure(model),
+            Self::JsFunctionExpression(function) => function.closure(model),
+        }
+    }
+}
+
+impl TryFrom<AnyJsExpression> for AnyJsFunctionExpression {
+    type Error = ();
+
+    fn try_from(expression: AnyJsExpression) -> Result<Self, Self::Error> {
+        match expression {
+            AnyJsExpression::JsArrowFunctionExpression(arrow_function) => {
+                Ok(Self::JsArrowFunctionExpression(arrow_function))
+            }
+            AnyJsExpression::JsFunctionExpression(function) => {
+                Ok(Self::JsFunctionExpression(function))
+            }
+            _ => Err(()),
+        }
+    }
+}
+
 impl ReactCallWithDependencyResult {
     /// Returns all [Reference] captured by the closure argument of the React hook.
     /// See [react_hook_with_dependency].
     pub fn all_captures(&self, model: &SemanticModel) -> impl Iterator<Item = Capture> {
         self.closure_node
             .as_ref()
-            .and_then(|node| node.as_js_arrow_function_expression())
-            .map(|arrow_function| {
-                let closure = arrow_function.closure(model);
+            .and_then(|node| AnyJsFunctionExpression::try_from(node.clone()).ok())
+            .map(|function_expression| {
+                let closure = function_expression.closure(model);
                 let range = *closure.closure_range();
                 closure
                     .descendents()
@@ -78,6 +110,15 @@ pub(crate) fn react_hook_configuration<'a>(
     hooks.get(name)
 }
 
+const HOOKS_WITH_DEPS_API: [&str; 6] = [
+    "useEffect",
+    "useLayoutEffect",
+    "useInsertionEffect",
+    "useCallback",
+    "useMemo",
+    "useImperativeHandle",
+];
+
 /// Returns the [TextRange] of the hook name; the node of the
 /// expression of the argument that correspond to the closure of
 /// the hook; and the node of the dependency list of the hook.
@@ -95,18 +136,28 @@ pub(crate) fn react_hook_configuration<'a>(
 pub(crate) fn react_hook_with_dependency(
     call: &JsCallExpression,
     hooks: &HashMap<String, ReactHookConfiguration>,
+    model: &SemanticModel,
 ) -> Option<ReactCallWithDependencyResult> {
-    let name = call
-        .callee()
-        .ok()?
-        .as_js_identifier_expression()?
-        .name()
-        .ok()?
-        .value_token()
-        .ok()?;
+    let expression = call.callee().ok()?;
+    let name = match &expression {
+        AnyJsExpression::JsIdentifierExpression(identifier) => {
+            Some(identifier.name().ok()?.value_token().ok()?)
+        }
+        AnyJsExpression::JsStaticMemberExpression(member) => {
+            Some(member.member().ok()?.as_js_name()?.value_token().ok()?)
+        }
+        _ => None,
+    }?;
     let function_name_range = name.text_trimmed_range();
     let name = name.text_trimmed();
 
+    // check if the hooks api is imported from the react library
+    if HOOKS_WITH_DEPS_API.contains(&name)
+        && !is_react_call_api(expression, model, ReactLibrary::React, name)
+    {
+        return None;
+    }
+
     let hook = hooks.get(name)?;
     let closure_index = hook.closure_index?;
     let dependencies_index = hook.dependencies_index?;

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs

@@ -27,6 +27,8 @@ declare_rule! {
     /// ### Invalid
     ///
     /// ```js,expect_diagnostic
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     let a = 1;
     ///     useEffect(() => {
@@ -36,6 +38,8 @@ declare_rule! {
     /// ```
     ///
     /// ```js,expect_diagnostic
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     let b = 1;
     ///     useEffect(() => {
@@ -44,6 +48,8 @@ declare_rule! {
     /// ```
     ///
     /// ```js,expect_diagnostic
+    /// import { useEffect, useState } from "react";
+    ///
     /// function component() {
     ///     const [name, setName] = useState();
     ///     useEffect(() => {
@@ -54,6 +60,8 @@ declare_rule! {
     /// ```
     ///
     /// ```js,expect_diagnostic
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     let a = 1;
     ///     const b = a + 1;
@@ -66,6 +74,8 @@ declare_rule! {
     /// ## Valid
     ///
     /// ```js
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     let a = 1;
     ///     useEffect(() => {
@@ -75,6 +85,8 @@ declare_rule! {
     /// ```
     ///
     /// ```js
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     const a = 1;
     ///     useEffect(() => {
@@ -84,6 +96,8 @@ declare_rule! {
     /// ```
     ///
     /// ```js
+    /// import { useEffect } from "react";
+    ///
     /// function component() {
     ///     const [name, setName] = useState();
     ///     useEffect(() => {
@@ -489,6 +503,7 @@ fn function_of_hook_call(call: &JsCallExpression) -> Option<JsSyntaxNode> {
         matches!(
             x.kind(),
             JsSyntaxKind::JS_FUNCTION_DECLARATION
+                | JsSyntaxKind::JS_FUNCTION_EXPORT_DEFAULT_DECLARATION
                 | JsSyntaxKind::JS_FUNCTION_EXPRESSION
                 | JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION
         )
@@ -508,9 +523,9 @@ impl Rule for UseExhaustiveDependencies {
         let mut signals = vec![];
 
         let call = ctx.query();
-        if let Some(result) = react_hook_with_dependency(call, &options.hooks_config) {
-            let model = ctx.model();
+        let model = ctx.model();
 
+        if let Some(result) = react_hook_with_dependency(call, &options.hooks_config, model) {
             let Some(component_function) = function_of_hook_call(call) else {
                 return vec![]
             };

crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/checkHooksImportedFromReact.js

@@ -0,0 +1,20 @@
+function MyComponent1() {
+  let a = 1;
+  React.useEffect(() => {
+      console.log(a);
+  });
+
+  // the rule doesn't show the warnings because the hooks are not imported from react.
+  useEffect(() => {
+    console.log(a);
+  });
+}
+
+function MyComponent2() {
+  let a = 1;
+  const React = { useEffect() {} }
+  // the rule doesn't show the warnings because `React` is defined by the user.
+  React.useEffect(() => {
+      console.log(a);
+  });
+}

crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/checkHooksImportedFromReact.js.snap

@@ -0,0 +1,55 @@
+---
+source: crates/rome_js_analyze/tests/spec_tests.rs
+expression: checkHooksImportedFromReact.js
+---
+# Input
+```js
+function MyComponent1() {
+  let a = 1;
+  React.useEffect(() => {
+      console.log(a);
+  });
+
+  // the rule doesn't show the warnings because the hooks are not imported from react.
+  useEffect(() => {
+    console.log(a);
+  });
+}
+
+function MyComponent2() {
+  let a = 1;
+  const React = { useEffect() {} }
+  // the rule doesn't show the warnings because `React` is defined by the user.
+  React.useEffect(() => {
+      console.log(a);
+  });
+}
+
+```
+
+# Diagnostics
+```
+checkHooksImportedFromReact.js:3:9 lint/nursery/useExhaustiveDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This hook do not specify all of its dependencies.
+  
+    1 │ function MyComponent1() {
+    2 │   let a = 1;
+  > 3 │   React.useEffect(() => {
+      │         ^^^^^^^^^
+    4 │       console.log(a);
+    5 │   });
+  
+  i This dependency is not specified in the hook dependency list.
+  
+    2 │   let a = 1;
+    3 │   React.useEffect(() => {
+  > 4 │       console.log(a);
+      │                   ^
+    5 │   });
+    6 │ 
+  
+
+```
+
+

crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/customHook.js

@@ -1,3 +1,5 @@
+import { useEffect } from "react";
+
 function MyComponent() {
     let a = 1;
     useEffect(() => {
@@ -6,4 +8,4 @@ function MyComponent() {
     useMyEffect(() => {
         console.log(a);
     });
-}
+}