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 ; +}