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

fix(rome_js_analyze): use exhaustive deps considering external const as stable #3926

Merged
xunilrj merged 4 commits into from
Dec 7, 2022
Merged

fix(rome_js_analyze): use exhaustive deps considering external const as stable #3926

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df51b51
use exhaustive deps considering external const as stable
xunilrj Dec 5, 2022
cec346d
rebasing
xunilrj Dec 5, 2022
b67a84d
fmt and clippy issues
xunilrj Dec 5, 2022
2a1f4d5
updating documentation
xunilrj Dec 5, 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
Next Next commit
use exhaustive deps considering external const as stable
  • Loading branch information
@xunilrj
xunilrj committed Dec 5, 2022
commit df51b51fbb695344763ad3eb1611e49677fa0c56
13 changes: 6 additions & 7 deletions crates/rome_js_analyze/src/react/hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use std::collections::{HashMap, HashSet};

use rome_js_semantic::{Capture, ClosureExtensions, SemanticModel};
use rome_js_syntax::{
AnyJsExpression, JsArrayBindingPattern, JsArrayBindingPatternElementList, JsCallExpression,
JsIdentifierBinding, JsVariableDeclarator, TextRange,
binding_ext::AnyJsIdentifierBinding, AnyJsExpression, JsArrayBindingPattern,
JsArrayBindingPatternElementList, JsCallExpression, JsVariableDeclarator, TextRange,
};
use rome_rowan::AstNode;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -138,11 +138,11 @@ impl StableReactHookConfiguration {
/// }, [name]);
/// ```
pub fn is_binding_react_stable(
binding: &JsIdentifierBinding,
binding: &AnyJsIdentifierBinding,
stable_config: &HashSet<StableReactHookConfiguration>,
) -> bool {
fn array_binding_declarator_index(
binding: &JsIdentifierBinding,
binding: &AnyJsIdentifierBinding,
) -> Option<(JsVariableDeclarator, Option<usize>)> {
let index = binding.syntax().index() / 2;
let declarator = binding
Expand All @@ -153,7 +153,7 @@ pub fn is_binding_react_stable(
}

fn assignment_declarator(
binding: &JsIdentifierBinding,
binding: &AnyJsIdentifierBinding,
) -> Option<(JsVariableDeclarator, Option<usize>)> {
let declarator = binding.parent::<JsVariableDeclarator>()?;
Some((declarator, None))
Expand Down Expand Up @@ -191,7 +191,6 @@ mod test {
use super::*;
use rome_diagnostics::FileId;
use rome_js_syntax::SourceType;
use rome_rowan::SyntaxNodeCast;

#[test]
pub fn ok_react_stable_captures() {
Expand All @@ -206,7 +205,7 @@ mod test {
.filter(|x| x.text_trimmed() == "ref")
.last()
.unwrap();
let set_name = node.cast::<JsIdentifierBinding>().unwrap();
let set_name = AnyJsIdentifierBinding::cast(node).unwrap();

let config = HashSet::from_iter([
StableReactHookConfiguration::new("useRef", None),
Expand Down
223 changes: 139 additions & 84 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 @@ -4,9 +4,10 @@ use rome_analyze::{
context::RuleContext, declare_rule, DeserializableRuleOptions, Rule, RuleDiagnostic,
};
use rome_console::markup;
use rome_js_semantic::{Capture, SemanticModel};
use rome_js_syntax::{
JsCallExpression, JsIdentifierBinding, JsStaticMemberExpression, JsSyntaxKind, JsSyntaxNode,
JsVariableDeclaration, JsVariableDeclarator, TextRange, TsIdentifierBinding,
binding_ext::AnyJsBindingDeclaration, JsCallExpression, JsStaticMemberExpression, JsSyntaxKind,
JsSyntaxNode, JsVariableDeclaration, TextRange,
};
use rome_rowan::{AstNode, SyntaxNodeCast};
use serde::{Deserialize, Serialize};
Expand All @@ -20,56 +21,70 @@ declare_rule! {
/// ### Invalid
///
/// ```js,expect_diagnostic
/// let a = 1;
/// useEffect(() => {
/// console.log(a);
/// })
/// function f() {
/// let a = 1;
/// useEffect(() => {
/// console.log(a);
/// });
/// }
/// ```
///
/// ```js,expect_diagnostic
/// let b = 1;
/// useEffect(() => {
/// }, [b])
/// function f() {
/// let b = 1;
/// useEffect(() => {
/// }, [b]);
/// }
/// ```
///
/// ```js,expect_diagnostic
/// const [name, setName] = useState();
/// useEffect(() => {
/// console.log(name);
/// setName("");
/// }, [name, setName])
/// function f() {
/// const [name, setName] = useState();
/// useEffect(() => {
/// console.log(name);
/// setName("");
/// }, [name, setName]);
/// }
/// ```
///
/// ```js,expect_diagnostic
/// let a = 1;
/// const b = a + 1;
/// useEffect(() => {
/// console.log(b);
/// })
/// function f() {
/// let a = 1;
/// const b = a + 1;
/// useEffect(() => {
/// console.log(b);
/// });
/// }
/// ```
///
/// ## Valid
///
/// ```js
/// let a = 1;
/// useEffect(() => {
/// console.log(a);
/// }, [a]);
/// function f() {
/// let a = 1;
/// useEffect(() => {
/// console.log(a);
/// }, [a]);
/// }
/// ```
///
/// ```js
/// const a = 1;
/// useEffect(() => {
/// console.log(a);
/// });
/// function f() {
/// const a = 1;
/// useEffect(() => {
/// console.log(a);
/// });
/// }
/// ```
///
/// ```js
/// const [name, setName] = useState();
/// useEffect(() => {
/// console.log(name);
/// setName("");
/// }, [name])
/// function f() {
/// const [name, setName] = useState();
/// useEffect(() => {
/// console.log(name);
/// setName("");
/// }, [name]);
/// }
/// ```
///
pub(crate) UseExhaustiveDependencies {
Expand Down Expand Up @@ -167,6 +182,80 @@ fn get_whole_static_member_expression(
root.cast()
}

// Test if a capture needs to be in the dependency list
// of a react hook call
fn capture_needs_to_be_in_the_dependency_list(
capture: Capture,
component_function_range: &TextRange,
model: &SemanticModel,
options: &ReactExtensiveDependenciesOptions,
) -> Option<Capture> {
let binding = capture.binding();

// Ignore if imported
if binding.is_imported() {
return None;
}

match binding.tree().declaration()? {
// These declarations are always stable
AnyJsBindingDeclaration::JsFunctionDeclaration(_)
| AnyJsBindingDeclaration::JsClassDeclaration(_)
| AnyJsBindingDeclaration::TsEnumDeclaration(_)
| AnyJsBindingDeclaration::TsTypeAliasDeclaration(_)
| AnyJsBindingDeclaration::TsInterfaceDeclaration(_)
| AnyJsBindingDeclaration::TsModuleDeclaration(_) => None,

// Variable declarators are stable if ...
AnyJsBindingDeclaration::JsVariableDeclarator(declarator) => {
let declaration = declarator
.syntax()
.ancestors()
.filter_map(JsVariableDeclaration::cast)
.next()?;
let declaration_range = declaration.syntax().text_range();

if declaration.is_const() {
// ... they are `const` and declared outside of the component function
let _ = component_function_range.intersect(declaration_range)?;

// ... they are `const` and their initializer is constant
let initializer = declarator.initializer()?;
let expr = initializer.expression().ok()?;
if model.is_constant(&expr) {
return None;
}
}

// ... they are assign to stable returns of another React function
let not_stable = !is_binding_react_stable(&binding.tree(), &options.stable_config);
not_stable.then_some(capture)
}

// all others need to be in the dependency list
AnyJsBindingDeclaration::JsFormalParameter(_)
| AnyJsBindingDeclaration::JsRestParameter(_)
| AnyJsBindingDeclaration::JsBogusParameter(_)
| AnyJsBindingDeclaration::TsIndexSignatureParameter(_)
| AnyJsBindingDeclaration::TsPropertyParameter(_)
| AnyJsBindingDeclaration::JsFunctionExpression(_)
| AnyJsBindingDeclaration::TsDeclareFunctionDeclaration(_)
| AnyJsBindingDeclaration::JsClassExpression(_)
| AnyJsBindingDeclaration::JsImportDefaultClause(_)
| AnyJsBindingDeclaration::JsImportNamespaceClause(_)
| AnyJsBindingDeclaration::JsShorthandNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsBogusNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsDefaultImportSpecifier(_)
| AnyJsBindingDeclaration::JsNamespaceImportSpecifier(_)
| AnyJsBindingDeclaration::TsImportEqualsDeclaration(_)
| AnyJsBindingDeclaration::JsClassExportDefaultDeclaration(_)
| AnyJsBindingDeclaration::JsFunctionExportDefaultDeclaration(_)
| AnyJsBindingDeclaration::TsDeclareFunctionExportDefaultDeclaration(_)
| AnyJsBindingDeclaration::JsCatchDeclaration(_) => Some(capture),
}
}

impl Rule for UseExhaustiveDependencies {
type Query = Semantic<JsCallExpression>;
type State = Fix;
Expand All @@ -178,63 +267,29 @@ impl Rule for UseExhaustiveDependencies {

let mut signals = vec![];

let node = ctx.query();
if let Some(result) = react_hook_with_dependency(node, &options.hooks_config) {
let call = ctx.query();
if let Some(result) = react_hook_with_dependency(call, &options.hooks_config) {
let model = ctx.model();

let Some(component_function) = call
.syntax()
.ancestors()
.find(|x| matches!(x.kind(), JsSyntaxKind::JS_FUNCTION_DECLARATION))
else {
return vec![]
};

let component_function_range = component_function.text_range();

let captures: Vec<_> = result
.all_captures(model)
.into_iter()
.filter_map(|capture| {
let binding = capture.binding();
let binding_syntax = binding.syntax();
let node = binding_syntax.parent()?;
use JsSyntaxKind::*;
match node.kind() {
JS_FUNCTION_DECLARATION
| JS_CLASS_DECLARATION
| TS_ENUM_DECLARATION
| TS_TYPE_ALIAS_DECLARATION
| TS_DECLARE_FUNCTION_DECLARATION => None,
_ => {
// Ignore if imported
if let Some(true) = binding_syntax
.clone()
.cast::<JsIdentifierBinding>()
.map(|node| model.is_imported(&node))
.or_else(|| {
Some(model.is_imported(&node.cast::<TsIdentifierBinding>()?))
})
{
None
} else {
let binding =
binding.syntax().clone().cast::<JsIdentifierBinding>()?;

// Ignore if constant
if let Some(declarator) = binding.parent::<JsVariableDeclarator>() {
let declaration = declarator
.syntax()
.ancestors()
.filter_map(JsVariableDeclaration::cast)
.next()?;

if declaration.is_const() {
let initializer = declarator.initializer()?;
let expr = initializer.expression().ok()?;
if model.is_constant(&expr) {
return None;
}
}
}

// Ignore if stable
let not_stable =
!is_binding_react_stable(&binding, &options.stable_config);
not_stable.then_some(capture)
}
}
}
capture_needs_to_be_in_the_dependency_list(
capture,
&component_function_range,
model,
options,
)
})
.map(|capture| {
let path = get_whole_static_member_expression(capture.node());
Expand Down
22 changes: 14 additions & 8 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
@@ -1,3 +1,5 @@
/* should not generate diagnostics */

import doSomething from 'a';

// No captures
Expand Down Expand Up @@ -28,7 +30,6 @@ function MyComponent3() {
}

// interaction with other react hooks

function MyComponent4() {
const [name, setName] = useState(0);
const ref = useRef();
Expand Down Expand Up @@ -63,7 +64,6 @@ function MyComponent4() {
}

// all hooks with dependencies

function MyComponent5() {
let a = 1;
useEffect(() => console.log(a), [a]);
Expand All @@ -75,7 +75,6 @@ function MyComponent5() {
}

// inner closures

function MyComponent5() {
let a = 1;
useEffect(() => {
Expand All @@ -84,8 +83,7 @@ function MyComponent5() {
}, [a]);
}

// from import

// from import
function MyComponent6() {
useEffect(() => {
doSomething();
Expand All @@ -101,10 +99,18 @@ function MyComponent7() {
}, [someObj.name, someObj.age]);
}

// Specified dependency cover captures

// Specified dependency cover captures
function MyComponent8({ a }) {
useEffect(() => {
console.log(a.b);
}, [a]);
}
}

// Capturing const outside of component
// https://github.com/rome/tools/issues/3727
const outside = f();
function MyComponent9() {
useEffect(() => {
console.log(outside);
});
}
Loading