Skip to content

Commit a34d0e0

Browse files
committed
[compiler] Fix false positive hook return mutation error (#34424)
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch(); <div onClick={(e) => { dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias. DiffTrain build for [acada30](acada30)
1 parent 2d3a7f8 commit a34d0e0

35 files changed

+130
-108
lines changed

compiled/eslint-plugin-react-hooks/index.js

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -40870,9 +40870,12 @@ function applyEffect(context, state, _effect, initialized, effects) {
4087040870
case 'MaybeAlias':
4087140871
case 'Alias':
4087240872
case 'Capture': {
40873-
CompilerError.invariant(effect.kind === 'Capture' || initialized.has(effect.into.identifier.id), {
40874-
reason: `Expected destination value to already be initialized within this instruction for Alias effect`,
40875-
description: `Destination ${printPlace(effect.into)} is not initialized in this instruction`,
40873+
CompilerError.invariant(effect.kind === 'Capture' ||
40874+
effect.kind === 'MaybeAlias' ||
40875+
initialized.has(effect.into.identifier.id), {
40876+
reason: `Expected destination to already be initialized within this instruction`,
40877+
description: `Destination ${printPlace(effect.into)} is not initialized in this ` +
40878+
`instruction for effect ${printAliasingEffect(effect)}`,
4087640879
details: [
4087740880
{
4087840881
kind: 'error',
@@ -40882,44 +40885,53 @@ function applyEffect(context, state, _effect, initialized, effects) {
4088240885
],
4088340886
});
4088440887
const intoKind = state.kind(effect.into).kind;
40885-
let isMutableDesination;
40888+
let destinationType = null;
4088640889
switch (intoKind) {
40887-
case ValueKind.Context:
40888-
case ValueKind.Mutable:
40889-
case ValueKind.MaybeFrozen: {
40890-
isMutableDesination = true;
40890+
case ValueKind.Context: {
40891+
destinationType = 'context';
4089140892
break;
4089240893
}
40893-
default: {
40894-
isMutableDesination = false;
40894+
case ValueKind.Mutable:
40895+
case ValueKind.MaybeFrozen: {
40896+
destinationType = 'mutable';
4089540897
break;
4089640898
}
4089740899
}
4089840900
const fromKind = state.kind(effect.from).kind;
40899-
let isMutableReferenceType;
40901+
let sourceType = null;
4090040902
switch (fromKind) {
40903+
case ValueKind.Context: {
40904+
sourceType = 'context';
40905+
break;
40906+
}
4090140907
case ValueKind.Global:
4090240908
case ValueKind.Primitive: {
40903-
isMutableReferenceType = false;
4090440909
break;
4090540910
}
4090640911
case ValueKind.Frozen: {
40907-
isMutableReferenceType = false;
40908-
applyEffect(context, state, {
40909-
kind: 'ImmutableCapture',
40910-
from: effect.from,
40911-
into: effect.into,
40912-
}, initialized, effects);
40912+
sourceType = 'frozen';
4091340913
break;
4091440914
}
4091540915
default: {
40916-
isMutableReferenceType = true;
40916+
sourceType = 'mutable';
4091740917
break;
4091840918
}
4091940919
}
40920-
if (isMutableDesination && isMutableReferenceType) {
40920+
if (sourceType === 'frozen') {
40921+
applyEffect(context, state, {
40922+
kind: 'ImmutableCapture',
40923+
from: effect.from,
40924+
into: effect.into,
40925+
}, initialized, effects);
40926+
}
40927+
else if ((sourceType === 'mutable' && destinationType === 'mutable') ||
40928+
effect.kind === 'MaybeAlias') {
4092140929
effects.push(effect);
4092240930
}
40931+
else if ((sourceType === 'context' && destinationType != null) ||
40932+
(sourceType === 'mutable' && destinationType === 'context')) {
40933+
applyEffect(context, state, { kind: 'MaybeAlias', from: effect.from, into: effect.into }, initialized, effects);
40934+
}
4092340935
break;
4092440936
}
4092540937
case 'Assign': {
@@ -44119,7 +44131,12 @@ class AliasingState {
4411944131
if (edge.index >= index) {
4412044132
break;
4412144133
}
44122-
queue.push({ place: edge.node, transitive, direction: 'forwards', kind });
44134+
queue.push({
44135+
place: edge.node,
44136+
transitive,
44137+
direction: 'forwards',
44138+
kind: edge.kind === 'maybeAlias' ? MutationKind.Conditional : kind,
44139+
});
4412344140
}
4412444141
for (const [alias, when] of node.createdFrom) {
4412544142
if (when >= index) {
@@ -44137,7 +44154,12 @@ class AliasingState {
4413744154
if (when >= index) {
4413844155
continue;
4413944156
}
44140-
queue.push({ place: alias, transitive, direction: 'backwards', kind });
44157+
queue.push({
44158+
place: alias,
44159+
transitive,
44160+
direction: 'backwards',
44161+
kind,
44162+
});
4414144163
}
4414244164
for (const [alias, when] of node.maybeAliases) {
4414344165
if (when >= index) {

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
665de2ed283205fccecda649ae2d66f62983f15f
1+
acada3035fe2a0dacfafc0ad78914e77d11fb823
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
665de2ed283205fccecda649ae2d66f62983f15f
1+
acada3035fe2a0dacfafc0ad78914e77d11fb823

compiled/facebook-www/React-dev.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1418,7 +1418,7 @@ __DEV__ &&
14181418
exports.useTransition = function () {
14191419
return resolveDispatcher().useTransition();
14201420
};
1421-
exports.version = "19.2.0-www-classic-665de2ed-20250909";
1421+
exports.version = "19.2.0-www-classic-acada303-20250909";
14221422
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
14231423
"function" ===
14241424
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-dev.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1418,7 +1418,7 @@ __DEV__ &&
14181418
exports.useTransition = function () {
14191419
return resolveDispatcher().useTransition();
14201420
};
1421-
exports.version = "19.2.0-www-modern-665de2ed-20250909";
1421+
exports.version = "19.2.0-www-modern-acada303-20250909";
14221422
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
14231423
"function" ===
14241424
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,4 +600,4 @@ exports.useSyncExternalStore = function (
600600
exports.useTransition = function () {
601601
return ReactSharedInternals.H.useTransition();
602602
};
603-
exports.version = "19.2.0-www-classic-665de2ed-20250909";
603+
exports.version = "19.2.0-www-classic-acada303-20250909";

compiled/facebook-www/React-prod.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,4 +600,4 @@ exports.useSyncExternalStore = function (
600600
exports.useTransition = function () {
601601
return ReactSharedInternals.H.useTransition();
602602
};
603-
exports.version = "19.2.0-www-modern-665de2ed-20250909";
603+
exports.version = "19.2.0-www-modern-acada303-20250909";

compiled/facebook-www/React-profiling.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ exports.useSyncExternalStore = function (
604604
exports.useTransition = function () {
605605
return ReactSharedInternals.H.useTransition();
606606
};
607-
exports.version = "19.2.0-www-classic-665de2ed-20250909";
607+
exports.version = "19.2.0-www-classic-acada303-20250909";
608608
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
609609
"function" ===
610610
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-profiling.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ exports.useSyncExternalStore = function (
604604
exports.useTransition = function () {
605605
return ReactSharedInternals.H.useTransition();
606606
};
607-
exports.version = "19.2.0-www-modern-665de2ed-20250909";
607+
exports.version = "19.2.0-www-modern-acada303-20250909";
608608
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
609609
"function" ===
610610
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19708,10 +19708,10 @@ __DEV__ &&
1970819708
(function () {
1970919709
var internals = {
1971019710
bundleType: 1,
19711-
version: "19.2.0-www-classic-665de2ed-20250909",
19711+
version: "19.2.0-www-classic-acada303-20250909",
1971219712
rendererPackageName: "react-art",
1971319713
currentDispatcherRef: ReactSharedInternals,
19714-
reconcilerVersion: "19.2.0-www-classic-665de2ed-20250909"
19714+
reconcilerVersion: "19.2.0-www-classic-acada303-20250909"
1971519715
};
1971619716
internals.overrideHookState = overrideHookState;
1971719717
internals.overrideHookStateDeletePath = overrideHookStateDeletePath;
@@ -19745,7 +19745,7 @@ __DEV__ &&
1974519745
exports.Shape = Shape;
1974619746
exports.Surface = Surface;
1974719747
exports.Text = Text;
19748-
exports.version = "19.2.0-www-classic-665de2ed-20250909";
19748+
exports.version = "19.2.0-www-classic-acada303-20250909";
1974919749
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
1975019750
"function" ===
1975119751
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

0 commit comments

Comments
 (0)