Facebook/react
1
0
Fork 0
mirror of https://github.com/facebook/react.git synced 2025-11-01 09:12:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

[compiler] ValidateExhaustiveDeps disallows unnecessary non-reactive deps

Browse Source 
Just to be consistent, we disallow unnecessary deps even if they're known to be non-reactive.
This commit is contained in:
Joe Savona
2025-10-02 09:14:09 -07:00
parent 767ae4970a
commit b4b3863b8f
3 changed files with 115 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts
+69 -21
View File
@@ -22,7 +22,9 @@ import {
Identifier,
IdentifierId,
InstructionKind,
isStableType,
isSubPath,
isUseRefType,
LoadGlobal,
ManualMemoDependency,
Place,
@@ -46,9 +48,10 @@ const DEBUG = false;
* or less times.
*
* TODOs:
* - Better handling of cases where we infer multiple dependencies related to a single
* variable. Eg if the user has dep `x` and we inferred `x.y, x.z`, the user's dep
* is sufficient.
* - Handle cases of mixed optional and non-optional versions of the same path,
* eg referecing both x.y.z and x.y?.z in the same memo block. we should collapse
* this into a single canonical dep that we look for in the manual deps. see the
* existing exhaustive deps rule for implementation.
* - Handle cases where the user deps were not simple identifiers + property chains.
* We try to detect this in ValidateUseMemo but we miss some cases. The problem
* is that invalid forms can be value blocks or function calls that don't get
@@ -108,7 +111,7 @@ export function validateExhaustiveDependencies(
);
visitCandidateDependency(value.decl, temporaries, dependencies, locals);
const inferred: Array<InferredDependency> = Array.from(dependencies);
// Sort dependencies by name, and path, with shorter/non-optional paths first
// Sort dependencies by name and path, with shorter/non-optional paths first
inferred.sort((a, b) => {
if (a.kind === 'Global' && b.kind == 'Global') {
return a.binding.name.localeCompare(b.binding.name);
@@ -205,6 +208,31 @@ export function validateExhaustiveDependencies(
reason: 'Unexpected function dependency',
loc: value.loc,
});
/**
* Dependencies technically only need to include reactive values. However,
* reactivity inference for general values is subtle since it involves all
* of our complex control and data flow analysis. To keep results more
* stable and predictable to developers, we intentionally stay closer to
* the rules of the classic exhaustive-deps rule. Values should be included
* as dependencies if either of the following is true:
* - They're reactive
* - They're non-reactive and not a known-stable value type.
*
* Thus `const ref: Ref = cond ? ref1 : ref2` has to be a dependency
* (assuming `cond` is reactive) since it's reactive despite being a ref.
*
* Similarly, `const x = [1,2,3]` has to be a dependency since even
* though it's non reactive, it's not a known stable type.
*
* TODO: consider reimplementing a simpler form of reactivity inference.
* Ideally we'd consider `const ref: Ref = cond ? ref1 : ref2` as a required
* dependency even if our data/control flow tells us that `cond` is non-reactive.
* It's simpler for developers to reason about based on a more structural/AST
* driven approach.
*/
const isRequiredDependency =
reactive.has(inferredDependency.identifier.id) ||
!isStableType(inferredDependency.identifier);
let hasMatchingManualDependency = false;
for (const manualDependency of manualDependencies) {
if (
@@ -216,19 +244,18 @@ export function validateExhaustiveDependencies(
) {
hasMatchingManualDependency = true;
matched.add(manualDependency);
if (!isRequiredDependency) {
extra.push(manualDependency);
}
}
}
if (!hasMatchingManualDependency) {
if (isRequiredDependency && !hasMatchingManualDependency) {
missing.push(inferredDependency);
}
}
for (const dep of startMemo.deps ?? []) {
if (
matched.has(dep) ||
(dep.root.kind === 'NamedLocal' &&
!reactive.has(dep.root.value.identifier.id))
) {
if (matched.has(dep)) {
continue;
}
extra.push(dep);
@@ -247,19 +274,23 @@ export function validateExhaustiveDependencies(
];
}
if (missing.length !== 0) {
// Error
const diagnostic = CompilerDiagnostic.create({
category: ErrorCategory.PreserveManualMemo,
reason: 'Found non-exhaustive dependencies',
description:
'Missing dependencies can cause a value not to update when those inputs change, ' +
'resulting in stale UI. This memoization cannot be safely rewritten by the compiler.',
'resulting in stale UI',
suggestions,
});
for (const dep of missing) {
let reactiveStableValueHint = '';
if (isStableType(dep.identifier)) {
reactiveStableValueHint =
'. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values';
}
diagnostic.withDetails({
kind: 'error',
message: `Missing dependency \`${printInferredDependency(dep)}\``,
message: `Missing dependency \`${printInferredDependency(dep)}\`${reactiveStableValueHint}`,
loc: dep.loc,
});
}
@@ -270,13 +301,12 @@ export function validateExhaustiveDependencies(
reason: 'Found unnecessary memoization dependencies',
description:
'Unnecessary dependencies can cause a value to update more often than necessary, ' +
'which can cause effects to run more than expected. This memoization cannot be safely ' +
'rewritten by the compiler',
'which can cause effects to run more than expected',
});
diagnostic.withDetails({
kind: 'error',
message: `Unnecessary dependencies ${extra.map(dep => `\`${printManualMemoDependency(dep)}\``).join(', ')}`,
loc: value.loc,
loc: startMemo.depsLoc ?? value.loc,
});
error.pushDiagnostic(diagnostic);
}
@@ -287,10 +317,15 @@ export function validateExhaustiveDependencies(
startMemo = null;
}
collectDependencies(fn, temporaries, {
onStartMemoize,
onFinishMemoize,
});
collectDependencies(
fn,
temporaries,
{
onStartMemoize,
onFinishMemoize,
},
false, // isFunctionExpression
);
return error.asResult();
}
@@ -383,12 +418,20 @@ function collectDependencies(
locals: Set<IdentifierId>,
) => void;
} | null,
isFunctionExpression: boolean,
): Extract<Temporary, {kind: 'Function'}> {
const optionals = findOptionalPlaces(fn);
if (DEBUG) {
console.log(prettyFormat(optionals));
}
const locals: Set<IdentifierId> = new Set();
if (isFunctionExpression) {
for (const param of fn.params) {
const place = param.kind === 'Identifier' ? param : param.place;
locals.add(place.identifier.id);
}
}
const dependencies: Set<InferredDependency> = new Set();
function visit(place: Place): void {
visitCandidateDependency(place, temporaries, dependencies, locals);
@@ -523,7 +566,11 @@ function collectDependencies(
break;
}
case 'PropertyLoad': {
if (typeof value.property === 'number') {
if (
typeof value.property === 'number' ||
(isUseRefType(value.object.identifier) &&
value.property === 'current')
) {
visit(value.object);
break;
}
@@ -553,6 +600,7 @@ function collectDependencies(
value.loweredFunc.func,
temporaries,
null,
true, // isFunctionExpression
);
temporaries.set(lvalue.identifier.id, functionDeps);
addDependency(functionDeps, dependencies, locals);
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md
+36 -19
View File
@@ -26,8 +26,16 @@ function Component({x, y, z}) {
}, [x]);
const f = useMemo(() => {
return [];
}, [x, y.z, z?.y?.a]);
return <Stringify results={[a, b, c, d, e, f]} />;
}, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
const ref1 = useRef(null);
const ref2 = useRef(null);
const ref = z ? ref1 : ref2;
const cb = useMemo(() => {
return () => {
return ref.current;
};
}, []);
return <Stringify results={[a, b, c, d, e, f, cb]} />;
}
```
@@ -36,11 +44,11 @@ function Component({x, y, z}) {
## Error
```
Found 4 errors:
Found 5 errors:
Compilation Skipped: Found non-exhaustive dependencies
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
error.invalid-exhaustive-deps.ts:7:11
5 | function Component({x, y, z}) {
@@ -53,7 +61,7 @@ error.invalid-exhaustive-deps.ts:7:11
Compilation Skipped: Found non-exhaustive dependencies
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
error.invalid-exhaustive-deps.ts:10:11
8 | }, [x?.y.z?.a.b]);
@@ -66,7 +74,7 @@ error.invalid-exhaustive-deps.ts:10:11
Compilation Skipped: Found non-exhaustive dependencies
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI. This memoization cannot be safely rewritten by the compiler..
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
error.invalid-exhaustive-deps.ts:13:11
11 | }, [x.y.z.a]);
@@ -79,20 +87,29 @@ error.invalid-exhaustive-deps.ts:13:11
Compilation Skipped: Found unnecessary memoization dependencies
Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected. This memoization cannot be safely rewritten by the compiler.
Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected.
error.invalid-exhaustive-deps.ts:23:20
21 | return e;
22 | }, [x]);
> 23 | const f = useMemo(() => {
| ^^^^^^^
> 24 | return [];
| ^^^^^^^^^^^^^^
> 25 | }, [x, y.z, z?.y?.a]);
| ^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`
26 | return <Stringify results={[a, b, c, d, e, f]} />;
27 | }
28 |
error.invalid-exhaustive-deps.ts:25:5
23 | const f = useMemo(() => {
24 | return [];
> 25 | }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL`
26 | const ref1 = useRef(null);
27 | const ref2 = useRef(null);
28 | const ref = z ? ref1 : ref2;
Compilation Skipped: Found non-exhaustive dependencies
Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
error.invalid-exhaustive-deps.ts:31:13
29 | const cb = useMemo(() => {
30 | return () => {
> 31 | return ref.current;
| ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values
32 | };
33 | }, []);
34 | return <Stringify results={[a, b, c, d, e, f, cb]} />;
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js
+10 -2
View File
@@ -22,6 +22,14 @@ function Component({x, y, z}) {
}, [x]);
const f = useMemo(() => {
return [];
}, [x, y.z, z?.y?.a]);
return <Stringify results={[a, b, c, d, e, f]} />;
}, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
const ref1 = useRef(null);
const ref2 = useRef(null);
const ref = z ? ref1 : ref2;
const cb = useMemo(() => {
return () => {
return ref.current;
};
}, []);
return <Stringify results={[a, b, c, d, e, f, cb]} />;
}