Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): use exhaustive deps support properties #3581

Merged
merged 3 commits into from
Nov 23, 2022
Merged

feat(rome_js_analyze): use exhaustive deps support properties #3581

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
203e319
support for properties in useExhaustiveDependencies
xunilrj Nov 7, 2022
67fb8a9
fmt and clippy issues
xunilrj Nov 7, 2022
ce04744
improving code
xunilrj Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 109 additions & 17 deletions crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -119,9 +118,22 @@ 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(TextRange, TextRange, TextRange),
xunilrj marked this conversation as resolved.
Show resolved Hide resolved
}

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 {
Expand Down Expand Up @@ -200,7 +212,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
Expand All @@ -214,21 +242,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_dependency = capture_text.starts_with(dependency_text);
xunilrj marked this conversation as resolved.
Show resolved Hide resolved
let dependency_deeper_capture = dependency_text.starts_with(capture_text);
match (capture_deeper_dependency, dependency_deeper_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(
result.function_name_range,
*capture_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);
}
}

Expand Down Expand Up @@ -259,10 +340,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.",
);
}
Expand All @@ -284,6 +364,18 @@ impl Rule for UseExhaustiveDependencies {

Some(diag)
}
Fix::DependencyTooDeep(use_effect_range, capture, dependency) => {
let diag = RuleDiagnostic::new(
rule_category!(),
use_effect_range,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, and I don't expect to be implemented in this PR. I was wondering if we should actually use the range that starts from the hook name and finishes at ), because the issue is the whole hook, not just the name.

// from start range of `useEffect`
useEffect(() => {
	return a + 1;
}, [a]);
// to the range of the last token `)`

This is a suggestion for all the diagnostics obviously. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally tend to dislike ranges too big. Because don't focus your attention, and is horrible in the IDE. But we can do, if everyone else prefers.

markup! {
"This hook specifies a dependency more specific that its captures"
},
)
.detail(capture, "This capture is more generic than...")
.detail(dependency, "...this dependency.");
Some(diag)
}
}
}
}
10 changes: 10 additions & 0 deletions ...rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
67 changes: 66 additions & 1 deletion ...js_analyze/tests/specs/nursery/useExhaustiveDependencies/extraDependenciesInvalid.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 │ }


```


9 changes: 9 additions & 0 deletions ...me_js_analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,12 @@ function MyComponent5() {
return () => console.log(a);
}, []);
}

// Capturing an object property

function MyComponent1() {
let someObj = getObj();
useEffect(() => {
console.log(someObj.name)
});
}
33 changes: 33 additions & 0 deletions ..._analyze/tests/specs/nursery/useExhaustiveDependencies/missingDependenciesInvalid.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ function MyComponent5() {
}, []);
}

// Capturing an object property

function MyComponent1() {
let someObj = getObj();
useEffect(() => {
console.log(someObj.name)
});
}

```

# Diagnostics
Expand Down Expand Up @@ -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 │ }


```


Empty file added crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/propertiesInvalid.js
View file
Open in desktop
Empty file.
10 changes: 10 additions & 0 deletions ...s/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/propertiesInvalid.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
expression: propertiesInvalid.js
---
# Input
```js

xunilrj marked this conversation as resolved.
Show resolved Hide resolved
```


16 changes: 16 additions & 0 deletions crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
15 changes: 15 additions & 0 deletions crates/rome_js_analyze/tests/specs/nursery/useExhaustiveDependencies/valid.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
```