-
Notifications
You must be signed in to change notification settings - Fork 50k
[compiler] Distingush optional/extraneous deps #35204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { | |
| Identifier, | ||
| IdentifierId, | ||
| InstructionKind, | ||
| isPrimitiveType, | ||
| isStableType, | ||
| isSubPath, | ||
| isSubPathIgnoringOptionals, | ||
|
|
@@ -53,20 +54,18 @@ const DEBUG = false; | |
| * - If the manual dependencies had extraneous deps, then auto memoization | ||
| * will remove them and cause the value to update *less* frequently. | ||
| * | ||
| * We consider a value V as missing if ALL of the following conditions are met: | ||
| * - V is reactive | ||
| * - There is no manual dependency path P such that whenever V would change, | ||
| * P would also change. If V is `x.y.z`, this means there must be some | ||
| * path P that is either `x.y.z`, `x.y`, or `x`. Note that we assume no | ||
| * interior mutability, such that a shorter path "covers" changes to longer | ||
| * more precise paths. | ||
| * | ||
| * We consider a value V extraneous if either of the folowing are true: | ||
| * - V is a reactive local that is unreferenced | ||
| * - V is a global that is unreferenced | ||
| * | ||
| * In other words, we allow extraneous non-reactive values since we know they cannot | ||
| * impact how often the memoization would run. | ||
| * The implementation compares the manual dependencies against the values | ||
| * actually used within the memoization function | ||
| * - For each value V referenced in the memo function, either: | ||
| * - If the value is non-reactive *and* a known stable type, then the | ||
| * value may optionally be specified as an exact dependency. | ||
| * - Otherwise, report an error unless there is a manual dependency that will | ||
| * invalidate whenever V invalidates. If `x.y.z` is referenced, there must | ||
| * be a manual dependency for `x.y.z`, `x.y`, or `x`. Note that we assume | ||
| * no interior mutability, ie we assume that any changes to inner paths must | ||
| * always cause the other path to change as well. | ||
| * - Any dependencies that do not correspond to a value referenced in the memo | ||
| * function are considered extraneous and throw an error | ||
| * | ||
| * ## TODO: Invalid, Complex Deps | ||
| * | ||
|
|
@@ -226,9 +225,6 @@ export function validateExhaustiveDependencies( | |
| reason: 'Unexpected function dependency', | ||
| loc: value.loc, | ||
| }); | ||
| const isRequiredDependency = reactive.has( | ||
| inferredDependency.identifier.id, | ||
| ); | ||
| let hasMatchingManualDependency = false; | ||
| for (const manualDependency of manualDependencies) { | ||
| if ( | ||
|
|
@@ -243,32 +239,40 @@ export function validateExhaustiveDependencies( | |
| ) { | ||
| hasMatchingManualDependency = true; | ||
| matched.add(manualDependency); | ||
| if (!isRequiredDependency) { | ||
| extra.push(manualDependency); | ||
| } | ||
| } | ||
| } | ||
| if (isRequiredDependency && !hasMatchingManualDependency) { | ||
| missing.push(inferredDependency); | ||
| const isOptionalDependency = | ||
| !reactive.has(inferredDependency.identifier.id) && | ||
| (isStableType(inferredDependency.identifier) || | ||
| isPrimitiveType(inferredDependency.identifier)); | ||
| if (hasMatchingManualDependency || isOptionalDependency) { | ||
| continue; | ||
| } | ||
| missing.push(inferredDependency); | ||
| } | ||
|
|
||
| for (const dep of startMemo.deps ?? []) { | ||
| if (matched.has(dep)) { | ||
| continue; | ||
| } | ||
| if (dep.root.kind === 'NamedLocal' && dep.root.constant) { | ||
| CompilerError.simpleInvariant( | ||
| !dep.root.value.reactive && | ||
| isPrimitiveType(dep.root.value.identifier), | ||
| { | ||
| reason: 'Expected constant-folded dependency to be non-reactive', | ||
| loc: dep.root.value.loc, | ||
| }, | ||
| ); | ||
| /* | ||
| * Constant primitives can get constant-folded, which means we won't | ||
| * see a LoadLocal for the value within the memo function. | ||
| */ | ||
|
Comment on lines
+267
to
+270
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, thanks for the explanation |
||
| continue; | ||
| } | ||
| extra.push(dep); | ||
| } | ||
|
|
||
| /** | ||
| * Per docblock, we only consider dependencies as extraneous if | ||
| * they are unused globals or reactive locals. Notably, this allows | ||
| * non-reactive locals. | ||
| */ | ||
| retainWhere(extra, dep => { | ||
| return dep.root.kind === 'Global' || dep.root.value.reactive; | ||
| }); | ||
|
|
||
| if (missing.length !== 0 || extra.length !== 0) { | ||
| let suggestions: Array<CompilerSuggestion> | null = null; | ||
| if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ function Component(props) { | |
|
|
||
| Component = useMemo(() => { | ||
| return Component; | ||
| }); | ||
| }, [Component]); | ||
|
|
||
| return <Component {...props} />; | ||
| } | ||
|
|
@@ -36,6 +36,7 @@ function Component(props) { | |
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
| Component = Stringify; | ||
|
|
||
| Component; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like an existing bug where DCE isn't cleaning this up |
||
| Component = Component; | ||
| $[0] = Component; | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @validateExhaustiveMemoizationDependencies | ||
|
|
||
| import {useState} from 'react'; | ||
| import {Stringify} from 'shared-runtime'; | ||
|
|
||
| function Component() { | ||
| const [state, setState] = useState(0); | ||
| const x = useMemo(() => { | ||
| return [state]; | ||
| // error: `setState` is a stable type, but not actually referenced | ||
| }, [state, setState]); | ||
|
|
||
| return 'oops'; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
|
|
||
| ## Error | ||
|
|
||
| ``` | ||
| Found 1 error: | ||
|
|
||
| Error: Found unnecessary memoization dependencies | ||
|
|
||
| Unnecessary dependencies can cause a value to update more often than necessary, causing performance regressions and effects to fire more often than expected. | ||
|
|
||
| error.invalid-exhaustive-deps-disallow-unused-stable-types.ts:11:5 | ||
| 9 | return [state]; | ||
| 10 | // error: `setState` is a stable type, but not actually referenced | ||
| > 11 | }, [state, setState]); | ||
| | ^^^^^^^^^^^^^^^^^ Unnecessary dependencies `setState` | ||
| 12 | | ||
| 13 | return 'oops'; | ||
| 14 | } | ||
| ``` | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // @validateExhaustiveMemoizationDependencies | ||
|
|
||
| import {useState} from 'react'; | ||
| import {Stringify} from 'shared-runtime'; | ||
|
|
||
| function Component() { | ||
| const [state, setState] = useState(0); | ||
| const x = useMemo(() => { | ||
| return [state]; | ||
| // error: `setState` is a stable type, but not actually referenced | ||
| }, [state, setState]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! |
||
|
|
||
| return 'oops'; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| // @validateExhaustiveMemoizationDependencies | ||
|
|
||
| function Component() { | ||
| const x = 0; | ||
| const y = useMemo(() => { | ||
| return [x]; | ||
| // x gets constant-folded but shouldn't count as extraneous, | ||
| // it was referenced in the memo block | ||
| }, [x]); | ||
| return y; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ## Code | ||
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies | ||
|
|
||
| function Component() { | ||
| const $ = _c(1); | ||
| const x = 0; | ||
| let t0; | ||
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
| t0 = [0]; | ||
| $[0] = t0; | ||
| } else { | ||
| t0 = $[0]; | ||
| } | ||
| const y = t0; | ||
| return y; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ### Eval output | ||
| (kind: exception) Fixture not implemented |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // @validateExhaustiveMemoizationDependencies | ||
|
|
||
| function Component() { | ||
| const x = 0; | ||
| const y = useMemo(() => { | ||
| return [x]; | ||
| // x gets constant-folded but shouldn't count as extraneous, | ||
| // it was referenced in the memo block | ||
| }, [x]); | ||
| return y; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this switches us back to special casing non-reactive deps allowed by
exhaustive-depsrule.This code should now bail out (right)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested in a follow-up