Skip to content

Commit 9d795d3

Browse files
authored
[compiler][bugfix] expand StoreContext to const / let / function variants (facebook#32747)
```js function Component() { useEffect(() => { let hasCleanedUp = false; document.addEventListener(..., () => hasCleanedUp ? foo() : bar()); // effect return values shouldn't be typed as frozen return () => { hasCleanedUp = true; } }; } ``` ### Problem `PruneHoistedContexts` currently strips hoisted declarations and rewrites the first `StoreContext` reassignment to a declaration. For example, in the following example, instruction 0 is removed while a synthetic `DeclareContext let` is inserted before instruction 1. ```js // source const cb = () => x; // reference that causes x to be hoisted let x = 4; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] StoreContext reassign 'x' = 4 [2] StoreContext reassign 'x' = 5 ``` Currently, we don't account for `DeclareContext let`. As a result, we're rewriting to insert duplicate declarations. ```js // source const cb = () => x; // reference that causes x to be hoisted let x; x = 5; // React Compiler IR [0] DeclareContext HoistedLet 'x' ... [1] DeclareContext Let 'x' [2] StoreContext reassign 'x' = 5 ``` ### Solution Instead of always lowering context variables to a DeclareContext followed by a StoreContext reassign, we can keep `kind: 'Const' | 'Let' | 'Reassign' | etc` on StoreContext. Pros: - retain more information in HIR, so we can codegen easily `const` and `let` context variable declarations back - pruning hoisted `DeclareContext` instructions is simple. Cons: - passes are more verbose as we need to check for both `DeclareContext` and `StoreContext` declarations ~(note: also see alternative implementation in facebook#32745 ### Testing Context variables are tricky. I synced and diffed changes in a large meta codebase and feel pretty confident about landing this. About 0.01% of compiled files changed. Among these changes, ~25% were [direct bugfixes](https://www.internalfb.com/phabricator/paste/view/P1800029094). The [other changes](https://www.internalfb.com/phabricator/paste/view/P1800028575) were primarily due to changed (corrected) mutable ranges from facebook#33047. I tried to represent most interesting changes in new test fixtures `
1 parent 12f4cb8 commit 9d795d3

File tree

36 files changed

+916
-232
lines changed

36 files changed

+916
-232
lines changed

compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3609,31 +3609,40 @@ function lowerAssignment(
36093609

36103610
let temporary;
36113611
if (builder.isContextIdentifier(lvalue)) {
3612-
if (kind !== InstructionKind.Reassign && !isHoistedIdentifier) {
3613-
if (kind === InstructionKind.Const) {
3614-
builder.errors.push({
3615-
reason: `Expected \`const\` declaration not to be reassigned`,
3616-
severity: ErrorSeverity.InvalidJS,
3617-
loc: lvalue.node.loc ?? null,
3618-
suggestions: null,
3619-
});
3620-
}
3621-
lowerValueToTemporary(builder, {
3622-
kind: 'DeclareContext',
3623-
lvalue: {
3624-
kind: InstructionKind.Let,
3625-
place: {...place},
3626-
},
3627-
loc: place.loc,
3612+
if (kind === InstructionKind.Const && !isHoistedIdentifier) {
3613+
builder.errors.push({
3614+
reason: `Expected \`const\` declaration not to be reassigned`,
3615+
severity: ErrorSeverity.InvalidJS,
3616+
loc: lvalue.node.loc ?? null,
3617+
suggestions: null,
36283618
});
36293619
}
36303620

3631-
temporary = lowerValueToTemporary(builder, {
3632-
kind: 'StoreContext',
3633-
lvalue: {place: {...place}, kind: InstructionKind.Reassign},
3634-
value,
3635-
loc,
3636-
});
3621+
if (
3622+
kind !== InstructionKind.Const &&
3623+
kind !== InstructionKind.Reassign &&
3624+
kind !== InstructionKind.Let &&
3625+
kind !== InstructionKind.Function
3626+
) {
3627+
builder.errors.push({
3628+
reason: `Unexpected context variable kind`,
3629+
severity: ErrorSeverity.InvalidJS,
3630+
loc: lvalue.node.loc ?? null,
3631+
suggestions: null,
3632+
});
3633+
temporary = lowerValueToTemporary(builder, {
3634+
kind: 'UnsupportedNode',
3635+
node: lvalueNode,
3636+
loc: lvalueNode.loc ?? GeneratedSource,
3637+
});
3638+
} else {
3639+
temporary = lowerValueToTemporary(builder, {
3640+
kind: 'StoreContext',
3641+
lvalue: {place: {...place}, kind},
3642+
value,
3643+
loc,
3644+
});
3645+
}
36373646
} else {
36383647
const typeAnnotation = lvalue.get('typeAnnotation');
36393648
let type: t.FlowType | t.TSType | null;

compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,27 @@ export enum InstructionKind {
746746
Function = 'Function',
747747
}
748748

749+
export function convertHoistedLValueKind(
750+
kind: InstructionKind,
751+
): InstructionKind | null {
752+
switch (kind) {
753+
case InstructionKind.HoistedLet:
754+
return InstructionKind.Let;
755+
case InstructionKind.HoistedConst:
756+
return InstructionKind.Const;
757+
case InstructionKind.HoistedFunction:
758+
return InstructionKind.Function;
759+
case InstructionKind.Let:
760+
case InstructionKind.Const:
761+
case InstructionKind.Function:
762+
case InstructionKind.Reassign:
763+
case InstructionKind.Catch:
764+
return null;
765+
default:
766+
assertExhaustive(kind, 'Unexpected lvalue kind');
767+
}
768+
}
769+
749770
function _staticInvariantInstructionValueHasLocation(
750771
value: InstructionValue,
751772
): SourceLocation {
@@ -880,8 +901,20 @@ export type InstructionValue =
880901
| StoreLocal
881902
| {
882903
kind: 'StoreContext';
904+
/**
905+
* StoreContext kinds:
906+
* Reassign: context variable reassignment in source
907+
* Const: const declaration + assignment in source
908+
* ('const' context vars are ones whose declarations are hoisted)
909+
* Let: let declaration + assignment in source
910+
* Function: function declaration in source (similar to `const`)
911+
*/
883912
lvalue: {
884-
kind: InstructionKind.Reassign;
913+
kind:
914+
| InstructionKind.Reassign
915+
| InstructionKind.Const
916+
| InstructionKind.Let
917+
| InstructionKind.Function;
885918
place: Place;
886919
};
887920
value: Place;

compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts

Lines changed: 50 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
FunctionExpression,
3131
ObjectMethod,
3232
PropertyLiteral,
33+
convertHoistedLValueKind,
3334
} from './HIR';
3435
import {
3536
collectHoistablePropertyLoads,
@@ -246,12 +247,18 @@ function isLoadContextMutable(
246247
id: InstructionId,
247248
): instrValue is LoadContext {
248249
if (instrValue.kind === 'LoadContext') {
249-
CompilerError.invariant(instrValue.place.identifier.scope != null, {
250-
reason:
251-
'[PropagateScopeDependencies] Expected all context variables to be assigned a scope',
252-
loc: instrValue.loc,
253-
});
254-
return id >= instrValue.place.identifier.scope.range.end;
250+
/**
251+
* Not all context variables currently have scopes due to limitations of
252+
* mutability analysis for function expressions.
253+
*
254+
* Currently, many function expressions references are inferred to be
255+
* 'Read' | 'Freeze' effects which don't replay mutable effects of captured
256+
* context.
257+
*/
258+
return (
259+
instrValue.place.identifier.scope != null &&
260+
id >= instrValue.place.identifier.scope.range.end
261+
);
255262
}
256263
return false;
257264
}
@@ -471,6 +478,9 @@ export class DependencyCollectionContext {
471478
}
472479
this.#reassignments.set(identifier, decl);
473480
}
481+
hasDeclared(identifier: Identifier): boolean {
482+
return this.#declarations.has(identifier.declarationId);
483+
}
474484

475485
// Checks if identifier is a valid dependency in the current scope
476486
#checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean {
@@ -672,21 +682,21 @@ export function handleInstruction(
672682
});
673683
} else if (value.kind === 'DeclareLocal' || value.kind === 'DeclareContext') {
674684
/*
675-
* Some variables may be declared and never initialized. We need
676-
* to retain (and hoist) these declarations if they are included
677-
* in a reactive scope. One approach is to simply add all `DeclareLocal`s
678-
* as scope declarations.
685+
* Some variables may be declared and never initialized. We need to retain
686+
* (and hoist) these declarations if they are included in a reactive scope.
687+
* One approach is to simply add all `DeclareLocal`s as scope declarations.
688+
*
689+
* Context variables with hoisted declarations only become live after their
690+
* first assignment. We only declare real DeclareLocal / DeclareContext
691+
* instructions (not hoisted ones) to avoid generating dependencies on
692+
* hoisted declarations.
679693
*/
680-
681-
/*
682-
* We add context variable declarations here, not at `StoreContext`, since
683-
* context Store / Loads are modeled as reads and mutates to the underlying
684-
* variable reference (instead of through intermediate / inlined temporaries)
685-
*/
686-
context.declare(value.lvalue.place.identifier, {
687-
id,
688-
scope: context.currentScope,
689-
});
694+
if (convertHoistedLValueKind(value.lvalue.kind) === null) {
695+
context.declare(value.lvalue.place.identifier, {
696+
id,
697+
scope: context.currentScope,
698+
});
699+
}
690700
} else if (value.kind === 'Destructure') {
691701
context.visitOperand(value.value);
692702
for (const place of eachPatternOperand(value.lvalue.pattern)) {
@@ -698,6 +708,26 @@ export function handleInstruction(
698708
scope: context.currentScope,
699709
});
700710
}
711+
} else if (value.kind === 'StoreContext') {
712+
/**
713+
* Some StoreContext variables have hoisted declarations. If we're storing
714+
* to a context variable that hasn't yet been declared, the StoreContext is
715+
* the declaration.
716+
* (see corresponding logic in PruneHoistedContext)
717+
*/
718+
if (
719+
!context.hasDeclared(value.lvalue.place.identifier) ||
720+
value.lvalue.kind !== InstructionKind.Reassign
721+
) {
722+
context.declare(value.lvalue.place.identifier, {
723+
id,
724+
scope: context.currentScope,
725+
});
726+
}
727+
728+
for (const operand of eachInstructionValueOperand(value)) {
729+
context.visitOperand(operand);
730+
}
701731
} else {
702732
for (const operand of eachInstructionValueOperand(value)) {
703733
context.visitOperand(operand);

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,15 @@ export function inferMutableLifetimes(
176176
if (
177177
instr.value.kind === 'DeclareContext' ||
178178
(instr.value.kind === 'StoreContext' &&
179-
instr.value.lvalue.kind !== InstructionKind.Reassign)
179+
instr.value.lvalue.kind !== InstructionKind.Reassign &&
180+
!contextVariableDeclarationInstructions.has(
181+
instr.value.lvalue.place.identifier,
182+
))
180183
) {
181-
// Save declarations of context variables
184+
/**
185+
* Save declarations of context variables if they hasn't already been
186+
* declared (due to hoisted declarations).
187+
*/
182188
contextVariableDeclarationInstructions.set(
183189
instr.value.lvalue.place.identifier,
184190
instr.id,

compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,9 +407,14 @@ class InferenceState {
407407

408408
freezeValues(values: Set<InstructionValue>, reason: Set<ValueReason>): void {
409409
for (const value of values) {
410-
if (value.kind === 'DeclareContext') {
410+
if (
411+
value.kind === 'DeclareContext' ||
412+
(value.kind === 'StoreContext' &&
413+
(value.lvalue.kind === InstructionKind.Let ||
414+
value.lvalue.kind === InstructionKind.Const))
415+
) {
411416
/**
412-
* Avoid freezing hoisted context declarations
417+
* Avoid freezing context variable declarations, hoisted or otherwise
413418
* function Component() {
414419
* const cb = useBar(() => foo(2)); // produces a hoisted context declaration
415420
* const foo = useFoo(); // reassigns to the context variable
@@ -1606,6 +1611,14 @@ function inferBlock(
16061611
);
16071612

16081613
const lvalue = instr.lvalue;
1614+
if (instrValue.lvalue.kind !== InstructionKind.Reassign) {
1615+
state.initialize(instrValue, {
1616+
kind: ValueKind.Mutable,
1617+
reason: new Set([ValueReason.Other]),
1618+
context: new Set(),
1619+
});
1620+
state.define(instrValue.lvalue.place, instrValue);
1621+
}
16091622
state.alias(lvalue, instrValue.value);
16101623
lvalue.effect = Effect.Store;
16111624
continuation = {kind: 'funeffects'};

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,6 +1000,14 @@ function codegenTerminal(
10001000
lval = codegenLValue(cx, iterableItem.value.lvalue.pattern);
10011001
break;
10021002
}
1003+
case 'StoreContext': {
1004+
CompilerError.throwTodo({
1005+
reason: 'Support non-trivial for..in inits',
1006+
description: null,
1007+
loc: terminal.init.loc,
1008+
suggestions: null,
1009+
});
1010+
}
10031011
default:
10041012
CompilerError.invariant(false, {
10051013
reason: `Expected a StoreLocal or Destructure to be assigned to the collection`,
@@ -1092,6 +1100,14 @@ function codegenTerminal(
10921100
lval = codegenLValue(cx, iterableItem.value.lvalue.pattern);
10931101
break;
10941102
}
1103+
case 'StoreContext': {
1104+
CompilerError.throwTodo({
1105+
reason: 'Support non-trivial for..of inits',
1106+
description: null,
1107+
loc: terminal.init.loc,
1108+
suggestions: null,
1109+
});
1110+
}
10951111
default:
10961112
CompilerError.invariant(false, {
10971113
reason: `Expected a StoreLocal or Destructure to be assigned to the collection`,

0 commit comments

Comments
 (0)