feat(rome_js_analyze): use exhaustive deps support properties

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

@@ -2,10 +2,9 @@ use crate::react::hooks::*;
 use crate::semantic_services::Semantic;
 use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic};
 use rome_console::markup;
-use rome_js_semantic::Capture;
 use rome_js_syntax::{
-    JsCallExpression, JsIdentifierBinding, JsSyntaxKind, JsVariableDeclaration,
-    JsVariableDeclarator, TextRange, TsIdentifierBinding,
+    JsCallExpression, JsIdentifierBinding, JsStaticMemberExpression, JsSyntaxKind, JsSyntaxNode,
+    JsVariableDeclaration, JsVariableDeclarator, TextRange, TsIdentifierBinding,
 };
 use rome_rowan::{AstNode, SyntaxNodeCast};
 use serde::{Deserialize, Serialize};
@@ -119,9 +118,26 @@ impl ReactExtensiveDependenciesOptions {
 /// Flags the possible fixes that were found
 pub enum Fix {
     /// When a dependency needs to be added.
-    AddDependency(TextRange, Vec<Capture>),
+    AddDependency(TextRange, Vec<TextRange>),
     /// When a dependency needs to be removed.
     RemoveDependency(TextRange, Vec<TextRange>),
+    /// When a dependency is more deep than the capture
+    DependencyTooDeep {
+        function_name_range: TextRange,
+        capture_range: TextRange,
+        dependency_range: TextRange,
+    }
+}
+
+fn get_whole_static_member_expression(
+    reference: &JsSyntaxNode,
+) -> Option<JsStaticMemberExpression> {
+    let root = reference
+        .ancestors()
+        .skip(2) //IDENT and JS_REFERENCE_IDENTIFIER
+        .take_while(|x| x.kind() == JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION)
+        .last()?;
+    root.cast()
 }
 
 impl Rule for UseExhaustiveDependencies {
@@ -200,7 +216,23 @@ impl Rule for UseExhaustiveDependencies {
                         }
                     })
                 })
-                .map(|x| (x.node().text_trimmed().to_string(), x))
+                .map(|capture| {
+                    let path = get_whole_static_member_expression(capture.node());
+
+                    let (text, range) = if let Some(path) = path {
+                        (
+                            path.syntax().text_trimmed().to_string(),
+                            path.syntax().text_trimmed_range(),
+                        )
+                    } else {
+                        (
+                            capture.node().text_trimmed().to_string(),
+                            capture.node().text_trimmed_range(),
+                        )
+                    };
+
+                    (text, range, capture)
+                })
                 .collect();
 
             let deps: Vec<(String, TextRange)> = result
@@ -214,21 +246,74 @@ impl Rule for UseExhaustiveDependencies {
                 })
                 .collect();
 
-            let mut add_deps: BTreeMap<String, Vec<Capture>> = BTreeMap::new();
+            let mut add_deps: BTreeMap<String, Vec<TextRange>> = BTreeMap::new();
             let mut remove_deps: Vec<TextRange> = vec![];
 
-            // Search for captures not in the dependency
-            for (text, capture) in captures.iter() {
-                if !deps.iter().any(|x| &x.0 == text) {
-                    let captures = add_deps.entry(text.clone()).or_default();
-                    captures.push(capture.clone());
+            // Evaluate all the captures
+            for (capture_text, capture_range, _) in captures.iter() {
+                let mut suggested_fix = None;
+                let mut is_captured_covered = false;
+                for (dependency_text, dependency_range) in deps.iter() {
+                    let capture_deeper_than_dependency = capture_text.starts_with(dependency_text);
+                    let dependency_deeper_then_capture = dependency_text.starts_with(capture_text);
+                    match (capture_deeper_than_dependency, dependency_deeper_then_capture) {
+                        // capture == dependency
+                        (true, true) => {
+                            suggested_fix = None;
+                            is_captured_covered = true;
+                            break;
+                        }
+                        // example
+                        // capture: a.b.c
+                        // dependency: a
+                        // this is ok, but we may suggest performance improvements
+                        // in the future
+                        (true, false) => {
+                            // We need to continue, because it may still have a perfect match
+                            // in the dependency list
+                            is_captured_covered = true;
+                        }
+                        // example
+                        // capture: a.b
+                        // dependency: a.b.c
+                        // This can be valid in some cases. We will flag an error nonetheless.
+                        (false, true) => {
+                            // We need to continue, because it may still have a perfect match
+                            // in the dependency list
+                            suggested_fix = Some(Fix::DependencyTooDeep {
+                                function_name_range: result.function_name_range,
+                                capture_range: *capture_range,
+                                dependency_range: *dependency_range,
+                            });
+                        }
+                        _ => {}
+                    }
+                }
+
+                if let Some(fix) = suggested_fix {
+                    signals.push(fix);
+                }
+
+                if !is_captured_covered {
+                    let captures = add_deps.entry(capture_text.clone()).or_default();
+                    captures.push(*capture_range);
                 }
             }
 
             // Search for dependencies not captured
-            for dep in deps {
-                if !captures.iter().any(|x| x.0 == dep.0) {
-                    remove_deps.push(dep.1);
+            for (dependency_text, dep_range) in deps {
+                let mut covers_any_capture = false;
+                for (capture_text, _, _) in captures.iter() {
+                    let capture_deeper_dependency = capture_text.starts_with(&dependency_text);
+                    let dependency_deeper_capture = dependency_text.starts_with(capture_text);
+                    if capture_deeper_dependency || dependency_deeper_capture {
+                        covers_any_capture = true;
+                        break;
+                    }
+                }
+
+                if !covers_any_capture {
+                    remove_deps.push(dep_range);
                 }
             }
 
@@ -259,10 +344,9 @@ impl Rule for UseExhaustiveDependencies {
                     },
                 );
 
-                for capture in captures.iter() {
-                    let node = capture.node();
+                for range in captures.iter() {
                     diag = diag.detail(
-                        node.text_trimmed_range(),
+                        range,
                         "This dependency is not specified in the hook dependency list.",
                     );
                 }
@@ -284,6 +368,18 @@ impl Rule for UseExhaustiveDependencies {
 
                 Some(diag)
             }
+            Fix::DependencyTooDeep { function_name_range, capture_range, dependency_range } => {
+                let diag = RuleDiagnostic::new(
+                    rule_category!(),
+                    function_name_range,
+                    markup! {
+                        "This hook specifies a dependency more specific that its captures"
+                    },
+                )
+                .detail(capture_range, "This capture is more generic than...")
+                .detail(dependency_range, "...this dependency.");
+                Some(diag)
+            }
         }
     }
 }

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

@@ -17,3 +17,13 @@ function MyComponent2() {
   useEffect(() => {}, [a]);
 }
 
+// dependency more deep than capture
+// Note: This can be a valid case, but there is
+// no way for the lint rule to know
+
+function MyComponent1() {
+  let someObj = getObj();
+  useEffect(() => {
+      console.log(someObj)
+  }, [someObj.id]);
+}

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

@@ -23,7 +23,16 @@ function MyComponent2() {
   useEffect(() => {}, [a]);
 }
 
-
+// dependency more deep than capture
+// Note: This can be a valid case, but there is
+// no way for the lint rule to know
+
+function MyComponent1() {
+  let someObj = getObj();
+  useEffect(() => {
+      console.log(someObj)
+  }, [someObj.id]);
+}
 ```
 
 # Diagnostics
@@ -108,4 +117,60 @@ extraDependenciesInvalid.js:17:3 lint/nursery/useExhaustiveDependencies ━━
 
 ```
 
+```
+extraDependenciesInvalid.js:26:3 lint/nursery/useExhaustiveDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This hook specifies a dependency more specific that its captures
+
+    24 │ function MyComponent1() {
+    25 │   let someObj = getObj();
+  > 26 │   useEffect(() => {
+       │   ^^^^^^^^^
+    27 │       console.log(someObj)
+    28 │   }, [someObj.id]);
+
+  i This capture is more generic than...
+
+    25 │   let someObj = getObj();
+    26 │   useEffect(() => {
+  > 27 │       console.log(someObj)
+       │                   ^^^^^^^
+    28 │   }, [someObj.id]);
+    29 │ }
+
+  i ...this dependency.
+
+    26 │   useEffect(() => {
+    27 │       console.log(someObj)
+  > 28 │   }, [someObj.id]);
+       │       ^^^^^^^^^^
+    29 │ }
+
+
+```
+
+```
+extraDependenciesInvalid.js:26:3 lint/nursery/useExhaustiveDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This hook do not specify all of its dependencies.
+
+    24 │ function MyComponent1() {
+    25 │   let someObj = getObj();
+  > 26 │   useEffect(() => {
+       │   ^^^^^^^^^
+    27 │       console.log(someObj)
+    28 │   }, [someObj.id]);
+
+  i This dependency is not specified in the hook dependency list.
+
+    25 │   let someObj = getObj();
+    26 │   useEffect(() => {
+  > 27 │       console.log(someObj)
+       │                   ^^^^^^^
+    28 │   }, [someObj.id]);
+    29 │ }
+
+
+```
+
 

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

@@ -61,3 +61,12 @@ function MyComponent5() {
     return () => console.log(a);
   }, []);
 }
+
+// Capturing an object property
+
+function MyComponent1() {
+  let someObj = getObj();
+  useEffect(() => {
+      console.log(someObj.name)
+  });
+}

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

@@ -68,6 +68,15 @@ function MyComponent5() {
   }, []);
 }
 
+// Capturing an object property
+
+function MyComponent1() {
+  let someObj = getObj();
+  useEffect(() => {
+      console.log(someObj.name)
+  });
+}
+
 ```
 
 # Diagnostics
@@ -463,4 +472,28 @@ missingDependenciesInvalid.js:59:3 lint/nursery/useExhaustiveDependencies ━━
 
 ```
 
+```
+missingDependenciesInvalid.js:69:3 lint/nursery/useExhaustiveDependencies ━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This hook do not specify all of its dependencies.
+
+    67 │ function MyComponent1() {
+    68 │   let someObj = getObj();
+  > 69 │   useEffect(() => {
+       │   ^^^^^^^^^
+    70 │       console.log(someObj.name)
+    71 │   });
+
+  i This dependency is not specified in the hook dependency list.
+
+    68 │   let someObj = getObj();
+    69 │   useEffect(() => {
+  > 70 │       console.log(someObj.name)
+       │                   ^^^^^^^^^^^^
+    71 │   });
+    72 │ }
+
+
+```
+
 

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

@@ -92,3 +92,19 @@ function MyComponent6() {
     });
 }
 
+// Capturing an object property
+function MyComponent7() {
+    let someObj = getObj();
+    useEffect(() => {
+        console.log(someObj.name);
+        console.log(someObj.age)
+    }, [someObj.name, someObj.age]);
+}
+
+// Specified dependency cover captures 
+
+function MyComponent8({ a }) {
+    useEffect(() => {
+      console.log(a.b);
+    }, [a]);
+  }

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

@@ -98,7 +98,22 @@ function MyComponent6() {
     });
 }
 
+// Capturing an object property
+function MyComponent7() {
+    let someObj = getObj();
+    useEffect(() => {
+        console.log(someObj.name);
+        console.log(someObj.age)
+    }, [someObj.name, someObj.age]);
+}
+
+// Specified dependency cover captures 
 
+function MyComponent8({ a }) {
+    useEffect(() => {
+      console.log(a.b);
+    }, [a]);
+  }
 ```