[ESLint] Handle optional member chains (#19275)
* Rename internal variables This disambiguates "optional"/"required" because that terminology is taken by optional chaining. * Handle optional member chains * Update comment Co-authored-by: Ricky <rickhanlonii@gmail.com> Co-authored-by: Ricky <rickhanlonii@gmail.com>
Dan Abramov committed
Jul 7, 2020 at 21:34 UTC
7c35cb20efb63c7c0bdd06e00259200a49c2343e
2 files changed
+225
-83
packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+118
-21
@@ -1384,17 +1384,16 @@ const tests = {
1384
errors: [
1385
{
1386
message:
1387
- // TODO: Ideally this would suggest props.foo?.bar.baz instead.
1388
- "React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " +
1387
+ "React Hook useCallback has a missing dependency: 'props.foo?.bar.baz'. " +
1388
'Either include it or remove the dependency array.',
1389
suggestions: [
1390
{
1392
- desc: 'Update the dependencies array to be: [props.foo.bar.baz]',
1391
+ desc: 'Update the dependencies array to be: [props.foo?.bar.baz]',
1392
output: normalizeIndent`
1393
function MyComponent(props) {
1394
useCallback(() => {
1395
console.log(props.foo?.bar.baz);
1397
- }, [props.foo.bar.baz]);
1396
+ }, [props.foo?.bar.baz]);
1397
}
1398
`,
1399
},
@@ -1413,17 +1412,17 @@ const tests = {
1412
errors: [
1413
{
1414
message:
1416
- // TODO: Ideally this would suggest props.foo?.bar?.baz instead.
1417
- "React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " +
1415
+ "React Hook useCallback has a missing dependency: 'props.foo?.bar?.baz'. " +
1416
'Either include it or remove the dependency array.',
1417
suggestions: [
1418
{
1421
- desc: 'Update the dependencies array to be: [props.foo.bar.baz]',
1419
+ desc:
1420
+ 'Update the dependencies array to be: [props.foo?.bar?.baz]',
1421
output: normalizeIndent`
1422
function MyComponent(props) {
1423
useCallback(() => {
1424
console.log(props.foo?.bar?.baz);
1426
- }, [props.foo.bar.baz]);
1425
+ }, [props.foo?.bar?.baz]);
1426
}
1427
`,
1428
},
@@ -1442,17 +1441,16 @@ const tests = {
1441
errors: [
1442
{
1443
message:
1445
- // TODO: Ideally this would suggest props.foo?.bar instead.
1446
- "React Hook useCallback has a missing dependency: 'props.foo.bar'. " +
1444
+ "React Hook useCallback has a missing dependency: 'props.foo?.bar'. " +
1445
'Either include it or remove the dependency array.',
1446
suggestions: [
1447
{
1450
- desc: 'Update the dependencies array to be: [props.foo.bar]',
1448
+ desc: 'Update the dependencies array to be: [props.foo?.bar]',
1449
output: normalizeIndent`
1450
function MyComponent(props) {
1451
useCallback(() => {
1452
console.log(props.foo?.bar.toString());
1455
- }, [props.foo.bar]);
1453
+ }, [props.foo?.bar]);
1454
}
1455
`,
1456
},
@@ -2045,18 +2043,18 @@ const tests = {
2043
errors: [
2044
{
2045
message:
2048
- "React Hook useEffect has a missing dependency: 'history.foo'. " +
2046
+ "React Hook useEffect has a missing dependency: 'history?.foo'. " +
2047
'Either include it or remove the dependency array.',
2048
suggestions: [
2049
{
2052
- desc: 'Update the dependencies array to be: [history.foo]',
2050
+ desc: 'Update the dependencies array to be: [history?.foo]',
2051
output: normalizeIndent`
2052
function MyComponent({ history }) {
2053
useEffect(() => {
2054
return [
2055
history?.foo
2056
];
2059
- }, [history.foo]);
2057
+ }, [history?.foo]);
2058
}
2059
`,
2060
},
@@ -6986,7 +6984,6 @@ const testsTypescript = {
6984
],
6985
},
6986
{
6989
- // https://github.com/facebook/react/issues/19243
6987
code: normalizeIndent`
6988
function MyComponent() {
6989
const pizza = {};
@@ -7000,14 +6997,12 @@ const testsTypescript = {
6997
errors: [
6998
{
6999
message:
7003
- "React Hook useEffect has missing dependencies: 'pizza.crust' and 'pizza.toppings'. " +
7000
+ "React Hook useEffect has missing dependencies: 'pizza.crust' and 'pizza?.toppings'. " +
7001
'Either include them or remove the dependency array.',
7002
suggestions: [
7003
{
7007
- // TODO the description and suggestions should probably also
7008
- // preserve the optional chaining.
7004
desc:
7010
- 'Update the dependencies array to be: [pizza.crust, pizza.toppings]',
7005
+ 'Update the dependencies array to be: [pizza.crust, pizza?.toppings]',
7006
output: normalizeIndent`
7007
function MyComponent() {
7008
const pizza = {};
@@ -7015,7 +7010,109 @@ const testsTypescript = {
7010
useEffect(() => ({
7011
crust: pizza.crust,
7012
toppings: pizza?.toppings,
7018
- }), [pizza.crust, pizza.toppings]);
7013
+ }), [pizza.crust, pizza?.toppings]);
7014
+ }
7015
+ `,
7016
+ },
7017
+ ],
7018
+ },
7019
+ ],
7020
+ },
7021
+ {
7022
+ code: normalizeIndent`
7023
+ function MyComponent() {
7024
+ const pizza = {};
7025
+
7026
+ useEffect(() => ({
7027
+ crust: pizza?.crust,
7028
+ density: pizza.crust.density,
7029
+ }), []);
7030
+ }
7031
+ `,
7032
+ errors: [
7033
+ {
7034
+ message:
7035
+ "React Hook useEffect has a missing dependency: 'pizza.crust'. " +
7036
+ 'Either include it or remove the dependency array.',
7037
+ suggestions: [
7038
+ {
7039
+ desc: 'Update the dependencies array to be: [pizza.crust]',
7040
+ output: normalizeIndent`
7041
+ function MyComponent() {
7042
+ const pizza = {};
7043
+
7044
+ useEffect(() => ({
7045
+ crust: pizza?.crust,
7046
+ density: pizza.crust.density,
7047
+ }), [pizza.crust]);
7048
+ }
7049
+ `,
7050
+ },
7051
+ ],
7052
+ },
7053
+ ],
7054
+ },
7055
+ {
7056
+ code: normalizeIndent`
7057
+ function MyComponent() {
7058
+ const pizza = {};
7059
+
7060
+ useEffect(() => ({
7061
+ crust: pizza.crust,
7062
+ density: pizza?.crust.density,
7063
+ }), []);
7064
+ }
7065
+ `,
7066
+ errors: [
7067
+ {
7068
+ message:
7069
+ "React Hook useEffect has a missing dependency: 'pizza.crust'. " +
7070
+ 'Either include it or remove the dependency array.',
7071
+ suggestions: [
7072
+ {
7073
+ desc: 'Update the dependencies array to be: [pizza.crust]',
7074
+ output: normalizeIndent`
7075
+ function MyComponent() {
7076
+ const pizza = {};
7077
+
7078
+ useEffect(() => ({
7079
+ crust: pizza.crust,
7080
+ density: pizza?.crust.density,
7081
+ }), [pizza.crust]);
7082
+ }
7083
+ `,
7084
+ },
7085
+ ],
7086
+ },
7087
+ ],
7088
+ },
7089
+ {
7090
+ code: normalizeIndent`
7091
+ function MyComponent() {
7092
+ const pizza = {};
7093
+
7094
+ useEffect(() => ({
7095
+ crust: pizza?.crust,
7096
+ density: pizza?.crust.density,
7097
+ }), []);
7098
+ }
7099
+ `,
7100
+ errors: [
7101
+ {
7102
+ message:
7103
+ "React Hook useEffect has a missing dependency: 'pizza?.crust'. " +
7104
+ 'Either include it or remove the dependency array.',
7105
+ suggestions: [
7106
+ {
7107
+ desc: 'Update the dependencies array to be: [pizza?.crust]',
7108
+ output: normalizeIndent`
7109
+ function MyComponent() {
7110
+ const pizza = {};
7111
+
7112
+ useEffect(() => ({
7113
+ crust: pizza?.crust,
7114
+ density: pizza?.crust.density,
7115
+ }), [pizza?.crust]);
7116
}
7117
`,
7118
},
packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+107
-62
@@ -72,7 +72,7 @@ export default {
72
// Should be shared between visitors.
73
const setStateCallSites = new WeakMap();
74
const stateVariables = new WeakSet();
75
- const staticKnownValueCache = new WeakMap();
75
+ const stableKnownValueCache = new WeakMap();
76
const functionWithoutCapturedValueCache = new WeakMap();
77
function memoizeWithWeakMap(fn, map) {
78
return function(arg) {
@@ -296,7 +296,7 @@ export default {
296
// Next we'll define a few helpers that helps us
297
// tell if some values don't have to be declared as deps.
298
299
- // Some are known to be static based on Hook calls.
299
+ // Some are known to be stable based on Hook calls.
300
// const [state, setState] = useState() / React.useState()
301
// ^^^ true for this reference
302
// const [state, dispatch] = useReducer() / React.useReducer()
@@ -304,7 +304,7 @@ export default {
304
// const ref = useRef()
305
// ^^^ true for this reference
306
// False for everything else.
307
- function isStaticKnownHookValue(resolved) {
307
+ function isStableKnownHookValue(resolved) {
308
if (!Array.isArray(resolved.defs)) {
309
return false;
310
}
@@ -343,7 +343,7 @@ export default {
343
typeof init.value === 'number' ||
344
init.value === null)
345
) {
346
- // Definitely static
346
+ // Definitely stable
347
return true;
348
}
349
// Detect known Hook calls
@@ -367,10 +367,10 @@ export default {
367
const id = def.node.id;
368
const {name} = callee;
369
if (name === 'useRef' && id.type === 'Identifier') {
370
- // useRef() return value is static.
370
+ // useRef() return value is stable.
371
return true;
372
} else if (name === 'useState' || name === 'useReducer') {
373
- // Only consider second value in initializing tuple static.
373
+ // Only consider second value in initializing tuple stable.
374
if (
375
id.type === 'ArrayPattern' &&
376
id.elements.length === 2 &&
@@ -387,7 +387,7 @@ export default {
387
);
388
}
389
}
390
- // Setter is static.
390
+ // Setter is stable.
391
return true;
392
} else if (id.elements[0] === resolved.identifiers[0]) {
393
if (name === 'useState') {
@@ -452,22 +452,22 @@ export default {
452
}
453
if (
454
pureScopes.has(ref.resolved.scope) &&
455
- // Static values are fine though,
455
+ // Stable values are fine though,
456
// although we won't check functions deeper.
457
- !memoizedIsStaticKnownHookValue(ref.resolved)
457
+ !memoizedIsStablecKnownHookValue(ref.resolved)
458
) {
459
return false;
460
}
461
}
462
// If we got here, this function doesn't capture anything
463
- // from render--or everything it captures is known static.
463
+ // from render--or everything it captures is known stable.
464
return true;
465
}
466
467
// Remember such values. Avoid re-running extra checks on them.
468
- const memoizedIsStaticKnownHookValue = memoizeWithWeakMap(
469
- isStaticKnownHookValue,
470
- staticKnownValueCache,
468
+ const memoizedIsStablecKnownHookValue = memoizeWithWeakMap(
469
+ isStableKnownHookValue,
470
+ stableKnownValueCache,
471
);
472
const memoizedIsFunctionWithoutCapturedValues = memoizeWithWeakMap(
473
isFunctionWithoutCapturedValues,
@@ -495,8 +495,9 @@ export default {
495
}
496
497
// Get dependencies from all our resolved references in pure scopes.
498
- // Key is dependency string, value is whether it's static.
498
+ // Key is dependency string, value is whether it's stable.
499
const dependencies = new Map();
500
+ const optionalChains = new Map();
501
gatherDependenciesRecursively(scope);
502
503
function gatherDependenciesRecursively(currentScope) {
@@ -517,7 +518,10 @@ export default {
518
reference.identifier,
519
);
520
const dependencyNode = getDependency(referenceNode);
520
- const dependency = toPropertyAccessString(dependencyNode);
521
+ const dependency = analyzePropertyChain(
522
+ dependencyNode,
523
+ optionalChains,
524
+ );
525
526
// Accessing ref.current inside effect cleanup is bad.
527
if (
@@ -540,36 +544,34 @@ export default {
544
}
545
546
const def = reference.resolved.defs[0];
543
-
547
if (def == null) {
548
continue;
549
}
547
-
550
// Ignore references to the function itself as it's not defined yet.
551
if (def.node != null && def.node.init === node.parent) {
552
continue;
553
}
552
-
554
// Ignore Flow type parameters
555
if (def.type === 'TypeParameter') {
556
continue;
557
}
558
559
// Add the dependency to a map so we can make sure it is referenced
559
- // again in our dependencies array. Remember whether it's static.
560
+ // again in our dependencies array. Remember whether it's stable.
561
if (!dependencies.has(dependency)) {
562
const resolved = reference.resolved;
562
- const isStatic =
563
- memoizedIsStaticKnownHookValue(resolved) ||
563
+ const isStable =
564
+ memoizedIsStablecKnownHookValue(resolved) ||
565
memoizedIsFunctionWithoutCapturedValues(resolved);
566
dependencies.set(dependency, {
566
- isStatic,
567
+ isStable,
568
references: [reference],
569
});
570
} else {
571
dependencies.get(dependency).references.push(reference);
572
}
573
}
574
+
575
for (const childScope of currentScope.childScopes) {
576
gatherDependenciesRecursively(childScope);
577
}
@@ -637,11 +639,11 @@ export default {
639
});
640
}
641
640
- // Remember which deps are optional and report bad usage first.
641
- const optionalDependencies = new Set();
642
- dependencies.forEach(({isStatic, references}, key) => {
643
- if (isStatic) {
644
- optionalDependencies.add(key);
642
+ // Remember which deps are stable and report bad usage first.
643
+ const stableDependencies = new Set();
644
+ dependencies.forEach(({isStable, references}, key) => {
645
+ if (isStable) {
646
+ stableDependencies.add(key);
647
}
648
references.forEach(reference => {
649
if (reference.writeExpr) {
@@ -659,7 +661,7 @@ export default {
661
// Check if there are any top-level setState() calls.
662
// Those tend to lead to infinite loops.
663
let setStateInsideEffectWithoutDeps = null;
662
- dependencies.forEach(({isStatic, references}, key) => {
664
+ dependencies.forEach(({isStable, references}, key) => {
665
if (setStateInsideEffectWithoutDeps) {
666
return;
667
}
@@ -689,7 +691,7 @@ export default {
691
const {suggestedDependencies} = collectRecommendations({
692
dependencies,
693
declaredDependencies: [],
692
- optionalDependencies,
694
+ stableDependencies,
695
externalDependencies: new Set(),
696
isEffect: true,
697
});
@@ -755,7 +757,10 @@ export default {
757
// will be thrown. We will catch that error and report an error.
758
let declaredDependency;
759
try {
758
- declaredDependency = toPropertyAccessString(declaredDependencyNode);
760
+ declaredDependency = analyzePropertyChain(
761
+ declaredDependencyNode,
762
+ null,
763
+ );
764
} catch (error) {
765
if (/Unsupported node type/.test(error.message)) {
766
if (declaredDependencyNode.type === 'Literal') {
@@ -822,7 +827,7 @@ export default {
827
} = collectRecommendations({
828
dependencies,
829
declaredDependencies,
825
- optionalDependencies,
830
+ stableDependencies,
831
externalDependencies,
832
isEffect,
833
});
@@ -901,7 +906,7 @@ export default {
906
suggestedDeps = collectRecommendations({
907
dependencies,
908
declaredDependencies: [], // Pretend we don't know
904
- optionalDependencies,
909
+ stableDependencies,
910
externalDependencies,
911
isEffect,
912
}).suggestedDependencies;
@@ -920,6 +925,24 @@ export default {
925
suggestedDeps.sort();
926
}
927
928
+ // Most of our algorithm deals with dependency paths with optional chaining stripped.
929
+ // This function is the last step before printing a dependency, so now is a good time to
930
+ // check whether any members in our path are always used as optional-only. In that case,
931
+ // we will use ?. instead of . to concatenate those parts of the path.
932
+ function formatDependency(path) {
933
+ const members = path.split('.');
934
+ let finalPath = '';
935
+ for (let i = 0; i < members.length; i++) {
936
+ if (i !== 0) {
937
+ const pathSoFar = members.slice(0, i + 1).join('.');
938
+ const isOptional = optionalChains.get(pathSoFar) === true;
939
+ finalPath += isOptional ? '?.' : '.';
940
+ }
941
+ finalPath += members[i];
942
+ }
943
+ return finalPath;
944
+ }
945
+
946
function getWarningMessage(deps, singlePrefix, label, fixVerb) {
947
if (deps.size === 0) {
948
return null;
@@ -933,7 +956,7 @@ export default {
956
joinEnglish(
957
Array.from(deps)
958
.sort()
936
- .map(name => "'" + name + "'"),
959
+ .map(name => "'" + formatDependency(name) + "'"),
960
) +
961
`. Either ${fixVerb} ${
962
deps.size > 1 ? 'them' : 'it'
@@ -1177,14 +1200,14 @@ export default {
1200
extraWarning,
1201
suggest: [
1202
{
1180
- desc: `Update the dependencies array to be: [${suggestedDeps.join(
1181
- ', ',
1182
- )}]`,
1203
+ desc: `Update the dependencies array to be: [${suggestedDeps
1204
+ .map(formatDependency)
1205
+ .join(', ')}]`,
1206
fix(fixer) {
1207
// TODO: consider preserving the comments or formatting?
1208
return fixer.replaceText(
1209
declaredDependenciesNode,
1187
- `[${suggestedDeps.join(', ')}]`,
1210
+ `[${suggestedDeps.map(formatDependency).join(', ')}]`,
1211
);
1212
},
1213
},
@@ -1198,7 +1221,7 @@ export default {
1221
function collectRecommendations({
1222
dependencies,
1223
declaredDependencies,
1201
- optionalDependencies,
1224
+ stableDependencies,
1225
externalDependencies,
1226
isEffect,
1227
}) {
@@ -1214,9 +1237,9 @@ function collectRecommendations({
1237
const depTree = createDepTree();
1238
function createDepTree() {
1239
return {
1217
- isRequired: false, // True if used in code
1240
+ isUsed: false, // True if used in code
1241
isSatisfiedRecursively: false, // True if specified in deps
1219
- hasRequiredNodesBelow: false, // True if something deeper is used by code
1242
+ isSubtreeUsed: false, // True if something deeper is used by code
1243
children: new Map(), // Nodes for properties
1244
};
1245
}
@@ -1225,9 +1248,9 @@ function collectRecommendations({
1248
// Imagine exclamation marks next to each used deep property.
1249
dependencies.forEach((_, key) => {
1250
const node = getOrCreateNodeByPath(depTree, key);
1228
- node.isRequired = true;
1251
+ node.isUsed = true;
1252
markAllParentsByPath(depTree, key, parent => {
1230
- parent.hasRequiredNodesBelow = true;
1253
+ parent.isSubtreeUsed = true;
1254
});
1255
});
1256
@@ -1237,7 +1260,7 @@ function collectRecommendations({
1260
const node = getOrCreateNodeByPath(depTree, key);
1261
node.isSatisfiedRecursively = true;
1262
});
1240
- optionalDependencies.forEach(key => {
1263
+ stableDependencies.forEach(key => {
1264
const node = getOrCreateNodeByPath(depTree, key);
1265
node.isSatisfiedRecursively = true;
1266
});
@@ -1282,7 +1305,7 @@ function collectRecommendations({
1305
node.children.forEach((child, key) => {
1306
const path = keyToPath(key);
1307
if (child.isSatisfiedRecursively) {
1285
- if (child.hasRequiredNodesBelow) {
1308
+ if (child.isSubtreeUsed) {
1309
// Remember this dep actually satisfied something.
1310
satisfyingPaths.add(path);
1311
}
@@ -1291,7 +1314,7 @@ function collectRecommendations({
1314
// `props.foo` is enough if you read `props.foo.id`.
1315
return;
1316
}
1294
- if (child.isRequired) {
1317
+ if (child.isUsed) {
1318
// Remember that no declared deps satisfied this node.
1319
missingPaths.add(path);
1320
// If we got here, nothing in its subtree was satisfied.
@@ -1466,25 +1489,47 @@ function getDependency(node) {
1489
/**
1490
* Assuming () means the passed node.
1491
* (foo) -> 'foo'
1469
- * foo.(bar) -> 'foo.bar'
1470
- * foo.bar.(baz) -> 'foo.bar.baz'
1492
+ * foo(.)bar -> 'foo.bar'
1493
+ * foo.bar(.)baz -> 'foo.bar.baz'
1494
* Otherwise throw.
1495
*/
1473
-function toPropertyAccessString(node) {
1496
+function analyzePropertyChain(node, optionalChains) {
1497
if (node.type === 'Identifier') {
1475
- return node.name;
1476
- } else if (
1477
- (node.type === 'MemberExpression' ||
1478
- node.type === 'OptionalMemberExpression') &&
1479
- !node.computed
1480
- ) {
1481
- const object = toPropertyAccessString(node.object);
1482
- const property = toPropertyAccessString(node.property);
1483
- // Note: we intentionally omit ? even for optional chaining
1484
- // because the returned string represents a path to the node, and
1485
- // is used as a key in Maps where being optional doesn't matter.
1486
- // The result string is not being interpolated in the code output.
1487
- return `${object}.${property}`;
1498
+ const result = node.name;
1499
+ if (optionalChains) {
1500
+ // Mark as required.
1501
+ optionalChains.set(result, false);
1502
+ }
1503
+ return result;
1504
+ } else if (node.type === 'MemberExpression' && !node.computed) {
1505
+ const object = analyzePropertyChain(node.object, optionalChains);
1506
+ const property = analyzePropertyChain(node.property, null);
1507
+ const result = `${object}.${property}`;
1508
+ if (optionalChains) {
1509
+ // Mark as required.
1510
+ optionalChains.set(result, false);
1511
+ }
1512
+ return result;
1513
+ } else if (node.type === 'OptionalMemberExpression' && !node.computed) {
1514
+ const object = analyzePropertyChain(node.object, optionalChains);
1515
+ const property = analyzePropertyChain(node.property, null);
1516
+ const result = `${object}.${property}`;
1517
+ if (optionalChains) {
1518
+ // Note: OptionalMemberExpression doesn't necessarily mean this node is optional.
1519
+ // It just means there is an optional member somewhere inside.
1520
+ // This particular node might still represent a required member, so check .optional field.
1521
+ if (node.optional) {
1522
+ // We only want to consider it optional if *all* usages were optional.
1523
+ if (!optionalChains.has(result)) {
1524
+ // Mark as (maybe) optional. If there's a required usage, this will be overridden.
1525
+ optionalChains.set(result, true);
1526
+ }
1527
+ } else {
1528
+ // Mark as required.
1529
+ optionalChains.set(result, false);
1530
+ }
1531
+ }
1532
+ return result;
1533
} else {
1534
throw new Error(`Unsupported node type: ${node.type}`);
1535
}
@@ -1529,7 +1574,7 @@ function getReactiveHookCallbackIndex(calleeNode, options) {
1574
// target custom reactive hooks.
1575
let name;
1576
try {
1532
- name = toPropertyAccessString(node);
1577
+ name = analyzePropertyChain(node, null);
1578
} catch (error) {
1579
if (/Unsupported node type/.test(error.message)) {
1580
return 0;