Skip to content

Commit a6adc0c

Browse files
DonIsaacDonIsaac
authored
fix(linter/exhaustive-deps): handle destructuring inside hooks (#11598)
## What This PR Does Updates `react-hooks/exhaustive-deps` to handle object/array destructuring within hooks. Before, this rule would report `props.foo` as an unnecessary dependency ```ts function MyComponent(props) { useEffect(() => { const { foo } = props; console.log(foo); }, [props.foo]); } ```
1 parent cf0b7d5 commit a6adc0c

File tree

2 files changed

+304
-36
lines changed

2 files changed

+304
-36
lines changed

‎crates/oxc_linter/src/rules/react/exhaustive_deps.rs‎

Lines changed: 259 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
1-
use std::hash::Hash;
1+
use std::{borrow::Cow, hash::Hash};
22

33
use itertools::Itertools;
44
use rustc_hash::FxHashSet;
55

66
use oxc_ast::{
77
AstKind, AstType,
88
ast::{
9-
Argument, ArrayExpressionElement, ArrowFunctionExpression, BindingPatternKind,
10-
CallExpression, ChainElement, Expression, FormalParameters, Function, FunctionBody,
11-
IdentifierReference, MemberExpression, StaticMemberExpression, TSTypeAnnotation,
12-
TSTypeParameter, TSTypeReference, VariableDeclarationKind,
9+
Argument, ArrayExpressionElement, ArrowFunctionExpression, BindingPattern,
10+
BindingPatternKind, CallExpression, ChainElement, Expression, FormalParameters, Function,
11+
FunctionBody, IdentifierReference, MemberExpression, StaticMemberExpression,
12+
TSTypeAnnotation, TSTypeParameter, TSTypeReference, VariableDeclarationKind,
13+
VariableDeclarator,
1314
},
1415
match_expression,
1516
};
@@ -97,7 +98,8 @@ fn dependency_array_not_array_literal_diagnostic(hook_name: &str, span: Span) ->
9798
"React Hook {hook_name} was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies."
9899
))
99100
.with_label(span)
100-
.with_help("Use an array literal as the second argument.") .with_error_code_scope(SCOPE)
101+
.with_help("Use an array literal as the second argument.")
102+
.with_error_code_scope(SCOPE)
101103
}
102104

103105
fn literal_in_dependency_array_diagnostic(span: Span) -> OxcDiagnostic {
@@ -665,9 +667,7 @@ impl GetSpan for CallbackNode<'_> {
665667

666668
// https://github.com/facebook/react/blob/1b0132c05acabae5aebd32c2cadddfb16bda70bc/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1789
667669
fn get_reactive_hook_callback_index(node: &CallExpression) -> Option<usize> {
668-
let node_name = get_node_name_without_react_namespace(&node.callee);
669-
670-
let hook_name = node_name?;
670+
let hook_name = get_node_name_without_react_namespace(&node.callee)?;
671671

672672
match hook_name.as_str() {
673673
"useEffect" | "useLayoutEffect" | "useCallback" | "useMemo" => Some(0),
@@ -1011,7 +1011,7 @@ fn func_call_without_react_namespace<'a>(
10111011
return None;
10121012
};
10131013

1014-
let Some(reference) = &member.object.get_identifier_reference() else { return None };
1014+
let reference = member.object.get_identifier_reference()?;
10151015

10161016
if reference.name == "React" {
10171017
return Some(&member.property.name);
@@ -1023,6 +1023,12 @@ fn func_call_without_react_namespace<'a>(
10231023
struct ExhaustiveDepsVisitor<'a, 'b> {
10241024
semantic: &'b Semantic<'a>,
10251025
stack: Vec<AstType>,
1026+
/// Variable declarations above the current node. Only populated in initializers.
1027+
///
1028+
/// NOTE: I don't expect this stack to ever have more than 1 element, since
1029+
/// variable declarators cannot be nested. However, having this as a stack
1030+
/// is definitely safer.
1031+
decl_stack: Vec<&'a VariableDeclarator<'a>>,
10261032
skip_reporting_dependency: bool,
10271033
set_state_call: bool,
10281034
found_dependencies: FxHashSet<Dependency<'a>>,
@@ -1034,6 +1040,7 @@ impl<'a, 'b> ExhaustiveDepsVisitor<'a, 'b> {
10341040
Self {
10351041
semantic,
10361042
stack: vec![],
1043+
decl_stack: vec![],
10371044
skip_reporting_dependency: false,
10381045
set_state_call: false,
10391046
found_dependencies: FxHashSet::default(),
@@ -1044,6 +1051,60 @@ impl<'a, 'b> ExhaustiveDepsVisitor<'a, 'b> {
10441051
fn visit_function_body_root(&mut self, function_body: &FunctionBody<'a>) {
10451052
walk_function_body(self, function_body);
10461053
}
1054+
1055+
fn iter_destructure_bindings<F>(&self, mut cb: F) -> Option<bool>
1056+
where
1057+
F: FnMut(std::borrow::Cow<'a, str>),
1058+
{
1059+
// check for object destructuring
1060+
// `const { foo } = props;`
1061+
// allow `props.foo` to be a dependency
1062+
let Some(VariableDeclarator {
1063+
id: BindingPattern { kind: BindingPatternKind::ObjectPattern(obj), .. },
1064+
..
1065+
}) = self.decl_stack.last()
1066+
else {
1067+
return None;
1068+
};
1069+
1070+
if obj.rest.is_some() {
1071+
return Some(true);
1072+
}
1073+
1074+
let mut needs_full_identifier = false;
1075+
for prop in &obj.properties {
1076+
if prop.computed {
1077+
needs_full_identifier = true;
1078+
continue;
1079+
}
1080+
match &prop.value.kind {
1081+
BindingPatternKind::BindingIdentifier(id) => {
1082+
cb(id.name.into());
1083+
}
1084+
BindingPatternKind::AssignmentPattern(pat) => {
1085+
if let Some(id) = pat.left.get_binding_identifier() {
1086+
cb(id.name.into());
1087+
} else {
1088+
// `const { idk: { thing } = { } } = props;`
1089+
// not sure what to do
1090+
needs_full_identifier = true;
1091+
}
1092+
}
1093+
BindingPatternKind::ArrayPattern(_) | BindingPatternKind::ObjectPattern(_) => {
1094+
// `const { foo: [bar] } = props;`
1095+
// `const { foo: { bar } } = props;`
1096+
// foo.bar is sufficient as a dependency
1097+
if let Some(key) = prop.key.name() {
1098+
cb(key);
1099+
} else {
1100+
needs_full_identifier = true;
1101+
}
1102+
}
1103+
}
1104+
}
1105+
1106+
Some(needs_full_identifier)
1107+
}
10471108
}
10481109

10491110
impl<'a> Visit<'a> for ExhaustiveDepsVisitor<'a, '_> {
@@ -1067,6 +1128,29 @@ impl<'a> Visit<'a> for ExhaustiveDepsVisitor<'a, '_> {
10671128
// noop
10681129
}
10691130

1131+
fn visit_variable_declarator(&mut self, decl: &VariableDeclarator<'a>) {
1132+
self.stack.push(AstType::VariableDeclarator);
1133+
// NOTE: decl_stack is only appended when visiting initializer
1134+
// expression.
1135+
self.visit_binding_pattern(&decl.id);
1136+
if let Some(init) = &decl.init {
1137+
// SAFETY:
1138+
// 1. All nodes live inside the arena, which has a lifetime of 'a.
1139+
// The arena lives longer than any Rule pass, so this visitor
1140+
// will drop before the node does.
1141+
// 2. This visitor is read-only, and it drops all references after
1142+
// visiting the node. Therefore, no mutable references will be
1143+
// created before this stack is dropped.
1144+
let decl = unsafe {
1145+
std::mem::transmute::<&VariableDeclarator<'_>, &'a VariableDeclarator<'a>>(decl)
1146+
};
1147+
self.decl_stack.push(decl);
1148+
self.visit_expression(init);
1149+
self.decl_stack.pop();
1150+
}
1151+
self.stack.pop();
1152+
}
1153+
10701154
fn visit_static_member_expression(&mut self, it: &StaticMemberExpression<'a>) {
10711155
if it.property.name == "current" && is_inside_effect_cleanup(&self.stack) {
10721156
// Safety: this is safe
@@ -1097,17 +1181,45 @@ impl<'a> Visit<'a> for ExhaustiveDepsVisitor<'a, '_> {
10971181
self.found_dependencies.insert(source);
10981182
} else {
10991183
let new_chain = Vec::from([it.property.name]);
1100-
self.found_dependencies.insert(Dependency {
1101-
name: source.name,
1102-
reference_id: source.reference_id,
1103-
span: source.span,
1104-
chain: [source.chain.clone(), new_chain].concat(),
1105-
symbol_id: self
1106-
.semantic
1107-
.scoping()
1108-
.get_reference(source.reference_id)
1109-
.symbol_id(),
1110-
});
1184+
1185+
let mut destructured_props: Vec<Atom<'a>> = vec![];
1186+
let mut did_see_ref = false;
1187+
let needs_full_chain = self
1188+
.iter_destructure_bindings(|id| {
1189+
if let Cow::Borrowed(id) = id {
1190+
if id == "current" {
1191+
did_see_ref = true;
1192+
} else {
1193+
destructured_props.push(id.into());
1194+
}
1195+
} else {
1196+
// todo
1197+
}
1198+
})
1199+
.unwrap_or(true);
1200+
1201+
let symbol_id =
1202+
self.semantic.scoping().get_reference(source.reference_id).symbol_id();
1203+
if needs_full_chain || (destructured_props.is_empty() && !did_see_ref) {
1204+
self.found_dependencies.insert(Dependency {
1205+
name: source.name,
1206+
reference_id: source.reference_id,
1207+
span: source.span,
1208+
chain: [source.chain.clone(), new_chain].concat(),
1209+
symbol_id,
1210+
});
1211+
} else {
1212+
for prop in destructured_props {
1213+
self.found_dependencies.insert(Dependency {
1214+
name: source.name,
1215+
reference_id: source.reference_id,
1216+
span: source.span,
1217+
chain: [source.chain.clone(), new_chain.clone(), vec![prop]]
1218+
.concat(),
1219+
symbol_id,
1220+
});
1221+
}
1222+
}
11111223
}
11121224
}
11131225

@@ -1129,13 +1241,43 @@ impl<'a> Visit<'a> for ExhaustiveDepsVisitor<'a, '_> {
11291241
if self.skip_reporting_dependency {
11301242
return;
11311243
}
1132-
self.found_dependencies.insert(Dependency {
1133-
name: ident.name,
1134-
reference_id: ident.reference_id(),
1135-
span: ident.span,
1136-
chain: vec![],
1137-
symbol_id: self.semantic.scoping().get_reference(ident.reference_id()).symbol_id(),
1138-
});
1244+
let reference_id = ident.reference_id();
1245+
let symbol_id = self.semantic.scoping().get_reference(reference_id).symbol_id();
1246+
1247+
let mut destructured_props: Vec<Atom<'a>> = vec![];
1248+
let mut did_see_ref = false;
1249+
let needs_full_identifier = self
1250+
.iter_destructure_bindings(|id| {
1251+
if let Cow::Borrowed(id) = id {
1252+
if id == "current" {
1253+
did_see_ref = true;
1254+
} else {
1255+
destructured_props.push(id.into());
1256+
}
1257+
} else {
1258+
// todo: arena allocate
1259+
}
1260+
})
1261+
.unwrap_or(true);
1262+
if needs_full_identifier || (destructured_props.is_empty() && !did_see_ref) {
1263+
self.found_dependencies.insert(Dependency {
1264+
name: ident.name,
1265+
reference_id,
1266+
span: ident.span,
1267+
chain: vec![],
1268+
symbol_id,
1269+
});
1270+
} else {
1271+
for prop in destructured_props {
1272+
self.found_dependencies.insert(Dependency {
1273+
name: ident.name,
1274+
reference_id,
1275+
span: ident.span,
1276+
chain: vec![prop],
1277+
symbol_id,
1278+
});
1279+
}
1280+
}
11391281

11401282
if let Some(decl) = get_declaration_of_variable(ident, self.semantic) {
11411283
let is_set_state_call = match decl.kind() {
@@ -1167,19 +1309,16 @@ impl<'a> Visit<'a> for ExhaustiveDepsVisitor<'a, '_> {
11671309

11681310
fn is_inside_effect_cleanup(stack: &[AstType]) -> bool {
11691311
let mut iter = stack.iter().rev();
1170-
let mut is_in_returned_function = false;
11711312

11721313
while let Some(&cur) = iter.next() {
1173-
if matches!(cur, AstType::Function | AstType::ArrowFunctionExpression) {
1174-
if let Some(&parent) = iter.next() {
1175-
if parent == AstType::ReturnStatement {
1176-
is_in_returned_function = true;
1177-
}
1178-
}
1314+
if matches!(cur, AstType::Function | AstType::ArrowFunctionExpression)
1315+
&& iter.next() == Some(&AstType::ReturnStatement)
1316+
{
1317+
return true;
11791318
}
11801319
}
11811320

1182-
is_in_returned_function
1321+
false
11831322
}
11841323

11851324
#[test]
@@ -1317,11 +1456,74 @@ fn test() {
13171456
console.log(color);
13181457
}, [props.foo, props.foo.bar.baz, color]);
13191458
}",
1459+
// destructuring
1460+
r"function MyComponent(props) {
1461+
useEffect(() => {
1462+
const { foo, bar } = props;
1463+
console.log(foo);
1464+
console.log(bar);
1465+
}, [props.foo, props.bar]);
1466+
}",
1467+
r"function MyComponent(props) {
1468+
useEffect(() => {
1469+
const { bar } = props.foo;
1470+
console.log(bar);
1471+
}, [props.foo.bar]);
1472+
}",
1473+
r"function MyComponent(props) {
1474+
useEffect(() => {
1475+
const { foo, bar } = props;
1476+
console.log(foo);
1477+
console.log(bar);
1478+
}, [props]);
1479+
}",
1480+
r"function MyComponent(props) {
1481+
useEffect(() => {
1482+
const { foo: { bar } } = props;
1483+
console.log(bar);
1484+
}, [props.foo]);
1485+
}",
1486+
r"function MyComponent(props) {
1487+
useEffect(() => {
1488+
const [bar] = props.foo;
1489+
console.log(bar);
1490+
}, [props.foo]);
1491+
}",
1492+
r"function MyComponent(props) {
1493+
const foo = useRef()
1494+
useEffect(() => {
1495+
const { current: bar } = foo;
1496+
console.log(bar);
1497+
}, []);
1498+
}",
1499+
r"function MyComponent(props) {
1500+
const bar = props.bar;
1501+
useEffect(() => {
1502+
const { bar } = foo();
1503+
console.log(bar);
1504+
}, [props.foo]);
1505+
}",
13201506
r"function MyComponent(props) {
13211507
useEffect(() => {
13221508
console.log(props.foo?.bar?.baz ?? null);
13231509
}, [props.foo]);
13241510
}",
1511+
r"function MyComponent(props) {
1512+
useEffect(() => {
1513+
console.log(props.foo);
1514+
}, [props.foo!]);
1515+
}",
1516+
// FIXME(@DonIsaac): these should pass
1517+
// r"function MyComponent(props) {
1518+
// useEffect(() => {
1519+
// console.log(props.foo!.bar);
1520+
// }, [props.foo!.bar]);
1521+
// }",
1522+
// r"function MyComponent(props) {
1523+
// useEffect(() => {
1524+
// console.log(props.foo!.bar!);
1525+
// }, [props.foo!.bar!]);
1526+
// }",
13251527
r"function MyComponent(props) {
13261528
useEffect(() => {
13271529
console.log(props.foo?.bar);
@@ -2436,6 +2638,27 @@ fn test() {
24362638
console.log(props.bar);
24372639
}, []);
24382640
}",
2641+
// destructuring
2642+
r"function MyComponent(props) {
2643+
useCallback(() => {
2644+
const { foo } = props;
2645+
console.log(foo);
2646+
}, [props.bar]);
2647+
}",
2648+
// FIXME: this should pass
2649+
r"function MyComponent(props) {
2650+
useCallback(() => {
2651+
const { foo: { bar } } = props;
2652+
console.log(bar);
2653+
}, [props.foo.bar]);
2654+
}",
2655+
r"function MyComponent(props) {
2656+
const foo = props.foo;
2657+
useEffect(() => {
2658+
const { bar } = foo();
2659+
console.log(bar);
2660+
}, [props.foo.bar]);
2661+
}",
24392662
r"function MyComponent() {
24402663
const local = {id: 42};
24412664
useEffect(() => {

0 commit comments

Comments
 (0)