From 9e758af12d03f49f98248e0c28c5eb7982076ce1 Mon Sep 17 00:00:00 2001 From: Daniel Frederico Lins Leite Date: Wed, 23 Nov 2022 21:15:49 +0000 Subject: [PATCH] feat(rome_js_analyze): use exhaustive deps support properties (#3581) * support for properties in useExhaustiveDependencies --- .../nursery/use_exhaustive_dependencies.rs | 130 +++++++++++++++--- .../extraDependenciesInvalid.js | 10 ++ .../extraDependenciesInvalid.js.snap | 67 ++++++++- .../missingDependenciesInvalid.js | 9 ++ .../missingDependenciesInvalid.js.snap | 33 +++++ .../useExhaustiveDependencies/valid.js | 16 +++ .../useExhaustiveDependencies/valid.js.snap | 15 ++ 7 files changed, 262 insertions(+), 18 deletions(-) diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs index d916171e925..c90e1c25588 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs +++ b/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), + AddDependency(TextRange, Vec), /// When a dependency needs to be removed. RemoveDependency(TextRange, Vec), + /// 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 { + 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 { @@ -193,7 +209,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 @@ -207,21 +239,74 @@ impl Rule for UseExhaustiveDependencies { }) .collect(); - let mut add_deps: BTreeMap> = BTreeMap::new(); + let mut add_deps: BTreeMap> = BTreeMap::new(); let mut remove_deps: Vec = 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); } } @@ -252,10 +337,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.", ); } @@ -277,6 +361,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) + } } } } diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js index a41f3b0bf66..0f9a09185d3 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js +++ b/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]); +} \ No newline at end of file diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js.snap b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js.snap index cb5c17d9f98..546c47e6c80 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js.snap +++ b/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 │ } + + +``` + diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js index afa679e37eb..6d321604ffe 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js +++ b/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) + }); +} diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js.snap b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js.snap index 22f2b62f671..1fb0532cffd 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js.snap +++ b/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 │ } + + +``` + diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js index f79f7599ab5..5839a6ee95d 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js +++ b/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]); + } \ No newline at end of file diff --git a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js.snap b/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js.snap index 7be87064aa5..047fa9dd406 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js.snap +++ b/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]); + } ```