Skip to content

Commit c919590

Browse files
committed
[compiler][bugfix] expand StoreContext to const / let / function variants
(TODO: handle edge cases with some context variables no longer having mutable ranges ```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 #32745)
1 parent 8039f1b commit c919590

31 files changed

+603
-224
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
@@ -3595,31 +3595,40 @@ function lowerAssignment(
35953595

35963596
let temporary;
35973597
if (builder.isContextIdentifier(lvalue)) {
3598-
if (kind !== InstructionKind.Reassign && !isHoistedIdentifier) {
3599-
if (kind === InstructionKind.Const) {
3600-
builder.errors.push({
3601-
reason: `Expected \`const\` declaration not to be reassigned`,
3602-
severity: ErrorSeverity.InvalidJS,
3603-
loc: lvalue.node.loc ?? null,
3604-
suggestions: null,
3605-
});
3606-
}
3607-
lowerValueToTemporary(builder, {
3608-
kind: 'DeclareContext',
3609-
lvalue: {
3610-
kind: InstructionKind.Let,
3611-
place: {...place},
3612-
},
3613-
loc: place.loc,
3598+
if (kind === InstructionKind.Const && !isHoistedIdentifier) {
3599+
builder.errors.push({
3600+
reason: `Expected \`const\` declaration not to be reassigned`,
3601+
severity: ErrorSeverity.InvalidJS,
3602+
loc: lvalue.node.loc ?? null,
3603+
suggestions: null,
36143604
});
36153605
}
36163606

3617-
temporary = lowerValueToTemporary(builder, {
3618-
kind: 'StoreContext',
3619-
lvalue: {place: {...place}, kind: InstructionKind.Reassign},
3620-
value,
3621-
loc,
3622-
});
3607+
if (
3608+
kind !== InstructionKind.Const &&
3609+
kind !== InstructionKind.Reassign &&
3610+
kind !== InstructionKind.Let &&
3611+
kind !== InstructionKind.Function
3612+
) {
3613+
builder.errors.push({
3614+
reason: `Unexpected context variable kind`,
3615+
severity: ErrorSeverity.InvalidJS,
3616+
loc: lvalue.node.loc ?? null,
3617+
suggestions: null,
3618+
});
3619+
temporary = lowerValueToTemporary(builder, {
3620+
kind: 'UnsupportedNode',
3621+
node: lvalueNode,
3622+
loc: lvalueNode.loc ?? GeneratedSource,
3623+
});
3624+
} else {
3625+
temporary = lowerValueToTemporary(builder, {
3626+
kind: 'StoreContext',
3627+
lvalue: {place: {...place}, kind},
3628+
value,
3629+
loc,
3630+
});
3631+
}
36233632
} else {
36243633
const typeAnnotation = lvalue.get('typeAnnotation');
36253634
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: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
FunctionExpression,
2424
ObjectMethod,
2525
PropertyLiteral,
26+
convertHoistedLValueKind,
2627
} from './HIR';
2728
import {
2829
collectHoistablePropertyLoads,
@@ -464,6 +465,9 @@ class Context {
464465
}
465466
this.#reassignments.set(identifier, decl);
466467
}
468+
hasDeclared(identifier: Identifier): boolean {
469+
return this.#declarations.has(identifier.declarationId);
470+
}
467471

468472
// Checks if identifier is a valid dependency in the current scope
469473
#checkValidDependency(maybeDependency: ReactiveScopeDependency): boolean {
@@ -662,21 +666,21 @@ function handleInstruction(instr: Instruction, context: Context): void {
662666
});
663667
} else if (value.kind === 'DeclareLocal' || value.kind === 'DeclareContext') {
664668
/*
665-
* Some variables may be declared and never initialized. We need
666-
* to retain (and hoist) these declarations if they are included
667-
* in a reactive scope. One approach is to simply add all `DeclareLocal`s
668-
* as scope declarations.
669-
*/
670-
671-
/*
672-
* We add context variable declarations here, not at `StoreContext`, since
673-
* context Store / Loads are modeled as reads and mutates to the underlying
674-
* variable reference (instead of through intermediate / inlined temporaries)
669+
* Some variables may be declared and never initialized. We need to retain
670+
* (and hoist) these declarations if they are included in a reactive scope.
671+
* One approach is to simply add all `DeclareLocal`s as scope declarations.
672+
*
673+
* Context variables with hoisted declarations only become live after their
674+
* first assignment. We only declare real DeclareLocal / DeclareContext
675+
* instructions (not hoisted ones) to avoid generating dependencies on
676+
* hoisted declarations.
675677
*/
676-
context.declare(value.lvalue.place.identifier, {
677-
id,
678-
scope: context.currentScope,
679-
});
678+
if (convertHoistedLValueKind(value.lvalue.kind) === null) {
679+
context.declare(value.lvalue.place.identifier, {
680+
id,
681+
scope: context.currentScope,
682+
});
683+
}
680684
} else if (value.kind === 'Destructure') {
681685
context.visitOperand(value.value);
682686
for (const place of eachPatternOperand(value.lvalue.pattern)) {
@@ -688,6 +692,26 @@ function handleInstruction(instr: Instruction, context: Context): void {
688692
scope: context.currentScope,
689693
});
690694
}
695+
} else if (value.kind === 'StoreContext') {
696+
/**
697+
* Some StoreContext variables have hoisted declarations. If we're storing
698+
* to a context variable that hasn't yet been declared, the StoreContext is
699+
* the declaration.
700+
* (see corresponding logic in PruneHoistedContext)
701+
*/
702+
if (
703+
!context.hasDeclared(value.lvalue.place.identifier) ||
704+
value.lvalue.kind !== InstructionKind.Reassign
705+
) {
706+
context.declare(value.lvalue.place.identifier, {
707+
id,
708+
scope: context.currentScope,
709+
});
710+
}
711+
712+
for (const operand of eachInstructionValueOperand(value)) {
713+
context.visitOperand(operand);
714+
}
691715
} else {
692716
for (const operand of eachInstructionValueOperand(value)) {
693717
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: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,13 @@ class InferenceState {
394394

395395
freezeValues(values: Set<InstructionValue>, reason: Set<ValueReason>): void {
396396
for (const value of values) {
397-
if (value.kind === 'DeclareContext') {
397+
if (
398+
value.kind === 'DeclareContext' ||
399+
(value.kind === 'StoreContext' &&
400+
value.lvalue.kind === InstructionKind.Let)
401+
) {
398402
/**
399-
* Avoid freezing hoisted context declarations
403+
* Avoid freezing context variable declarations, hoisted or otherwise
400404
* function Component() {
401405
* const cb = useBar(() => foo(2)); // produces a hoisted context declaration
402406
* const foo = useFoo(); // reassigns to the context variable
@@ -1591,6 +1595,14 @@ function inferBlock(
15911595
);
15921596

15931597
const lvalue = instr.lvalue;
1598+
if (instrValue.lvalue.kind !== InstructionKind.Reassign) {
1599+
state.initialize(instrValue, {
1600+
kind: ValueKind.Mutable,
1601+
reason: new Set([ValueReason.Other]),
1602+
context: new Set(),
1603+
});
1604+
state.define(instrValue.lvalue.place, instrValue);
1605+
}
15941606
state.alias(lvalue, instrValue.value);
15951607
lvalue.effect = Effect.Store;
15961608
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)