From 8b8087faef0769c4f76abd145a02d88187a96820 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 6 Jun 2025 12:39:13 -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-source-id: 8b05494224bbc647221fa798a66b097fcb3c442b Pull Request resolved: https://github.com/facebook/react/pull/33470 --- .../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 89527dee08..b73988c604 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 66839a89e8..2ed4498193 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 7294125724..da6f7036e6 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 2de67ebc5f..eb61d81a8b 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 b975527138..ac7299181e 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 0000000000..d25fd2bfb3 --- /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 0000000000..121495ac1e --- /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 ; +}