From a53d824e29346577b6ab0cd121507bb409ce5cd0 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 6 Jun 2025 12:42:58 -0700 Subject: [PATCH] [compiler][newinference] Improve hoisted functions, validation of mutate-after-render Updates ValidateNoFreezingMutableFunctions to account for indirections, where the frozen function calls another function that actually does the mutation. We just need to propagate on the mutation. Also some fixes for hoisted functions, DeclareContext can actually appear twice, so we need to check that subsequent (re)declarations are treated as a mutation, not a creation. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 1 + .../Inference/InferMutationAliasingEffects.ts | 74 +++++++++--- .../Inference/InferMutationAliasingRanges.ts | 8 +- ...ValidateNoFreezingKnownMutableFunctions.ts | 26 ++++- ...table-range-shared-inner-outer-function.js | 2 +- ...n-local-variable-in-jsx-callback.expect.md | 108 ++++++++++++++++++ ...reassign-local-variable-in-jsx-callback.js | 32 ++++++ 7 files changed, 225 insertions(+), 26 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 89527dee08a7f..b73988c604af2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -247,6 +247,7 @@ function runWithEnvironment( } if (!env.config.enableNewMutationAliasingModel) { + // NOTE: in the new model this is part of validateNoFreezingKnownMutableFunctions validateLocalsNotReassignedAfterRender(hir); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 66839a89e8583..2ed4498193ee4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -527,7 +527,11 @@ function applyEffect( } case 'CreateFunction': { effects.push(effect); - const isMutable = effect.captures.some(capture => { + /** + * We consider the function mutable if it has any mutable context variables or + * any side-effects that need to be tracked if the function is called. + */ + const hasCaptures = effect.captures.some(capture => { switch (state.kind(capture).kind) { case ValueKind.Context: case ValueKind.Mutable: { @@ -538,6 +542,12 @@ function applyEffect( } } }); + const hasTrackedSideEffects = + effect.function.loweredFunc.func.aliasingEffects?.some( + effect => + effect.kind === 'MutateFrozen' || effect.kind === 'MutateGlobal', + ); + const isMutable = hasCaptures || hasTrackedSideEffects; for (const operand of effect.function.loweredFunc.func.context) { if (operand.effect !== Effect.Capture) { continue; @@ -1593,23 +1603,6 @@ function computeSignatureForInstruction( } break; } - case 'DeclareContext': { - // Context variables are conceptually like mutable boxes - effects.push({ - kind: 'Create', - into: value.lvalue.place, - value: ValueKind.Mutable, - reason: ValueReason.Other, - }); - effects.push({ - kind: 'Create', - into: lvalue, - // The result can't be referenced so this value doesn't matter - value: ValueKind.Primitive, - reason: ValueReason.Other, - }); - break; - } case 'DeclareLocal': { // TODO check this effects.push({ @@ -1648,6 +1641,43 @@ function computeSignatureForInstruction( effects.push({kind: 'CreateFrom', from: value.place, into: lvalue}); break; } + case 'DeclareContext': { + // Context variables are conceptually like mutable boxes + const kind = value.lvalue.kind; + if ( + !context.hoistedContextDeclarations.has( + value.lvalue.place.identifier.declarationId, + ) || + kind === InstructionKind.HoistedConst || + kind === InstructionKind.HoistedFunction || + kind === InstructionKind.HoistedLet + ) { + /** + * If this context variable is not hoisted, or this is the declaration doing the hoisting, + * then we create the box. + */ + effects.push({ + kind: 'Create', + into: value.lvalue.place, + value: ValueKind.Mutable, + reason: ValueReason.Other, + }); + } else { + /** + * Otherwise this may be a "declare", but there was a previous DeclareContext that + * hoisted this variable, and we're mutating it here. + */ + effects.push({kind: 'Mutate', value: value.lvalue.place}); + } + effects.push({ + kind: 'Create', + into: lvalue, + // The result can't be referenced so this value doesn't matter + value: ValueKind.Primitive, + reason: ValueReason.Other, + }); + break; + } case 'StoreContext': { /* * Context variables are like mutable boxes, so semantically @@ -1921,6 +1951,14 @@ function computeEffectsForLegacySignature( arg.kind === 'Identifier' && i < signature.positionalParams.length ? signature.positionalParams[i]! : (signature.restParam ?? Effect.ConditionallyMutate); + + if (arg.kind === 'Spread' && effect === Effect.Freeze) { + CompilerError.throwTodo({ + reason: 'Support spread syntax for hook arguments', + loc: arg.place.loc, + }); + } + visit(place, effect); } if (captures.length !== 0) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts index 7294125724217..da6f7036e6dc0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -321,10 +321,6 @@ export function inferMutationAliasingRanges( } break; } - case 'ImmutableCapture': { - operandEffects.set(effect.from.identifier.id, Effect.Read); - break; - } case 'CreateFunction': case 'Create': { break; @@ -352,6 +348,10 @@ export function inferMutationAliasingRanges( operandEffects.set(effect.value.identifier.id, Effect.Freeze); break; } + case 'ImmutableCapture': { + // no-op, Read is the default + break; + } case 'MutateFrozen': case 'MutateGlobal': { // no-op diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts index 2de67ebc5fa7f..eb61d81a8b618 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts @@ -58,8 +58,7 @@ export function validateNoFreezingKnownMutableFunctions( const effect = contextMutationEffects.get(operand.identifier.id); if (effect != null) { errors.push({ - reason: `This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update`, - description: `Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables`, + reason: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`, loc: operand.loc, severity: ErrorSeverity.InvalidReact, }); @@ -124,7 +123,15 @@ export function validateNoFreezingKnownMutableFunctions( switch (effect.kind) { case 'Mutate': case 'MutateTransitive': { - if (context.has(effect.value.identifier.id)) { + const knownMutation = contextMutationEffects.get( + effect.value.identifier.id, + ); + if (knownMutation != null) { + contextMutationEffects.set( + lvalue.identifier.id, + knownMutation, + ); + } else if (context.has(effect.value.identifier.id)) { contextMutationEffects.set(lvalue.identifier.id, { kind: 'ContextMutation', effect: Effect.Mutate, @@ -135,6 +142,19 @@ export function validateNoFreezingKnownMutableFunctions( } break; } + case 'MutateConditionally': + case 'MutateTransitiveConditionally': { + const knownMutation = contextMutationEffects.get( + effect.value.identifier.id, + ); + if (knownMutation != null) { + contextMutationEffects.set( + lvalue.identifier.id, + knownMutation, + ); + } + break; + } } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js index b975527138f3b..ac7299181ed5f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js @@ -14,7 +14,7 @@ function Component(props) { a.property = true; b.push(false); }; - return
; + return
; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md new file mode 100644 index 0000000000000..d25fd2bfb3aa5 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md @@ -0,0 +1,108 @@ + +## Input + +```javascript +function Component() { + let local; + + const reassignLocal = newValue => { + local = newValue; + }; + + const onClick = newValue => { + reassignLocal('hello'); + + if (local === newValue) { + // Without React Compiler, `reassignLocal` is freshly created + // on each render, capturing a binding to the latest `local`, + // such that invoking reassignLocal will reassign the same + // binding that we are observing in the if condition, and + // we reach this branch + console.log('`local` was updated!'); + } else { + // With React Compiler enabled, `reassignLocal` is only created + // once, capturing a binding to `local` in that render pass. + // Therefore, calling `reassignLocal` will reassign the wrong + // version of `local`, and not update the binding we are checking + // in the if condition. + // + // To protect against this, we disallow reassigning locals from + // functions that escape + throw new Error('`local` not updated!'); + } + }; + + return ; +} + +``` + + +## Error + +``` + 29 | }; + 30 | +> 31 | return ; + | ^^^^^^^ InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31) + +InvalidReact: The function modifies a local variable here (5:5) + 32 | } + 33 | + +ReactCompilerError: InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31) + +InvalidReact: The function modifies a local variable here (5:5) + at validateNoFreezingKnownMutableFunctions (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113829:18) + at runWithEnvironment (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114076:7) + at run (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113959:10) + at compileFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114301:10) + at tryCompileFunction (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114793:19) + at processFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114730:25) + at compileProgram (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114638:22) + at PluginPass.enter (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:115773:26) + at newFn (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/visitors.js:172:14) + at NodePath._call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:49:20) + at NodePath.call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:39:18) + at NodePath.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:88:31) + at TraversalContext.visitQueue (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:90:16) + at TraversalContext.visitSingle (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:66:19) + at TraversalContext.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:113:19) + at traverseNode (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/traverse-node.js:22:17) + at traverse (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/index.js:53:34) + at transformFile (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:80:31) + at transformFile.next () + at run (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:25:12) + at run.next () + at /Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:23:33 + at Generator.next () + at evaluateSync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:251:28) + at sync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:89:14) + at stopHiding - secret - don't use this - v1 (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/errors/rewrite-stack-trace.js:47:12) + at transformFromAstSync (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:43:83) + at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:204:62 + at Generator.next () + at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:37:71 + at new Promise () + at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:33:12) + at transformFixtureInput (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:186:12) + at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:97:71 + at Generator.next () + at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71 + at new Promise () + at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12) + at compile (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:44:12) + at Object. (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:163:48) + at Generator.next () + at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71 + at new Promise () + at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12) + at Object.transformFixture (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:148:12) + at execFunction (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:148:17) + at execHelper (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:127:5) + at execMethod (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:131:5) + at MessagePort.messageListener (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:49:7) + at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js new file mode 100644 index 0000000000000..121495ac1e05c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js @@ -0,0 +1,32 @@ +function Component() { + let local; + + const reassignLocal = newValue => { + local = newValue; + }; + + const onClick = newValue => { + reassignLocal('hello'); + + if (local === newValue) { + // Without React Compiler, `reassignLocal` is freshly created + // on each render, capturing a binding to the latest `local`, + // such that invoking reassignLocal will reassign the same + // binding that we are observing in the if condition, and + // we reach this branch + console.log('`local` was updated!'); + } else { + // With React Compiler enabled, `reassignLocal` is only created + // once, capturing a binding to `local` in that render pass. + // Therefore, calling `reassignLocal` will reassign the wrong + // version of `local`, and not update the binding we are checking + // in the if condition. + // + // To protect against this, we disallow reassigning locals from + // functions that escape + throw new Error('`local` not updated!'); + } + }; + + return ; +}