diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index 10c45ccb3f5..1e1079d6867 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -416,6 +416,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ into: signatureArgument(4), signature: null, mutatesFunction: false, + loc: GeneratedSource, }, // captures the result of the callback into the return array { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 4dbb1107b1f..a8fa2765e01 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -1015,6 +1015,12 @@ export function printAliasingEffect(effect: AliasingEffect): string { case 'MutateGlobal': { return `MutateGlobal ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`; } + case 'Impure': { + return `Impure ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`; + } + case 'Render': { + return `Render ${printPlaceForAliasEffect(effect.place)}`; + } default: { assertExhaustive(effect, `Unexpected kind '${(effect as any).kind}'`); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 91f99395b03..a96e26af80c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -100,6 +100,8 @@ function lowerWithMutationAliasing(fn: HIRFunction): void { capturedOrMutated.add(effect.value.identifier.id); break; } + case 'Impure': + case 'Render': case 'MutateFrozen': case 'MutateGlobal': case 'CreateFunction': 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 f7bc2c33f15..ca71b4d164e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -10,6 +10,7 @@ import { CompilerErrorDetailOptions, Effect, ErrorSeverity, + SourceLocation, ValueKind, } from '..'; import { @@ -380,10 +381,7 @@ function applySignature( context: new Set(), }); effects.push({ - kind: - value.kind === ValueKind.Frozen - ? 'MutateFrozen' - : 'MutateGlobal', + kind: 'MutateFrozen', place: effect.value, error: { severity: ErrorSeverity.InvalidReact, @@ -546,7 +544,10 @@ function applyEffect( const hasTrackedSideEffects = effect.function.loweredFunc.func.aliasingEffects?.some( effect => - effect.kind === 'MutateFrozen' || effect.kind === 'MutateGlobal', + // TODO; include "render" here? + effect.kind === 'MutateFrozen' || + effect.kind === 'MutateGlobal' || + effect.kind === 'Impure', ); // For legacy compatibility const capturesRef = effect.function.loweredFunc.func.context.some( @@ -721,6 +722,7 @@ function applyEffect( effect.receiver, effect.args, functionValues[0].loweredFunc.func.context, + effect.loc, ); if (signatureEffects != null) { if (DEBUG) { @@ -747,6 +749,8 @@ function applyEffect( effect.into, effect.receiver, effect.args, + [], + effect.loc, ) : null; if (signatureEffects != null) { @@ -766,6 +770,7 @@ function applyEffect( effect.into, effect.receiver, effect.args, + effect.loc, ); for (const legacyEffect of legacyEffects) { applyEffect(context, state, legacyEffect, aliased, effects); @@ -899,6 +904,8 @@ function applyEffect( } break; } + case 'Impure': + case 'Render': case 'MutateFrozen': case 'MutateGlobal': { effects.push(effect); @@ -1452,6 +1459,7 @@ function computeSignatureForInstruction( args: value.args, into: lvalue, signature, + loc: value.loc, }); break; } @@ -1631,6 +1639,24 @@ function computeSignatureForInstruction( into: lvalue, }); } + if (value.kind === 'JsxExpression') { + if (value.tag.kind === 'Identifier') { + // Tags are render function, by definition they're called during render + effects.push({ + kind: 'Render', + place: value.tag, + }); + } + if (value.children != null) { + // Children are typically called during render, not used as an event/effect callback + for (const child of value.children) { + effects.push({ + kind: 'Render', + place: child, + }); + } + } + } break; } case 'DeclareLocal': { @@ -1861,6 +1887,7 @@ function computeEffectsForLegacySignature( lvalue: Place, receiver: Place, args: Array, + loc: SourceLocation, ): Array { const returnValueReason = signature.returnValueReason ?? ValueReason.Other; const effects: Array = []; @@ -1870,6 +1897,23 @@ function computeEffectsForLegacySignature( value: signature.returnValueKind, reason: returnValueReason, }); + if (signature.impure && state.env.config.validateNoImpureFunctionsInRender) { + effects.push({ + kind: 'Impure', + place: receiver, + error: { + reason: + 'Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', + description: + signature.canonicalName != null + ? `\`${signature.canonicalName}\` is an impure function whose results may change on every call` + : null, + severity: ErrorSeverity.InvalidReact, + loc, + suggestions: null, + }, + }); + } const stores: Array = []; const captures: Array = []; function visit(place: Place, effect: Effect): void { @@ -2083,6 +2127,7 @@ function computeEffectsForSignature( args: Array, // Used for signatures constructed dynamically which reference context variables context: Array = [], + loc: SourceLocation, ): Array | null { if ( // Not enough args @@ -2163,6 +2208,7 @@ function computeEffectsForSignature( } break; } + case 'Impure': case 'MutateFrozen': case 'MutateGlobal': { const values = substitutions.get(effect.place.identifier.id) ?? []; @@ -2171,6 +2217,13 @@ function computeEffectsForSignature( } break; } + case 'Render': { + const values = substitutions.get(effect.place.identifier.id) ?? []; + for (const value of values) { + effects.push({kind: effect.kind, place: value}); + } + break; + } case 'Mutate': case 'MutateTransitive': case 'MutateTransitiveConditionally': @@ -2254,6 +2307,7 @@ function computeEffectsForSignature( function: applyFunction[0], into: applyInto[0], signature: effect.signature, + loc, }); break; } @@ -2468,6 +2522,7 @@ export type AliasingEffect = args: Array; into: Place; signature: FunctionSignature | null; + loc: SourceLocation; } /** * Constructs a function value with the given captures. The mutability of the function @@ -2490,6 +2545,20 @@ export type AliasingEffect = kind: 'MutateGlobal'; place: Place; error: CompilerErrorDetailOptions; + } + /** + * Indicates a side-effect that is not safe during render + */ + | {kind: 'Impure'; place: Place; error: CompilerErrorDetailOptions} + /** + * Indicates that a given place is accessed during render. Used to distingush + * hook arguments that are known to be called immediately vs those used for + * event handlers/effects, and for JSX values known to be called during render + * (tags, children) vs those that may be events/effect (other props). + */ + | { + kind: 'Render'; + place: Place; }; function hashEffect(effect: AliasingEffect): string { @@ -2536,6 +2605,8 @@ function hashEffect(effect: AliasingEffect): string { case 'Freeze': { return [effect.kind, effect.value.identifier.id, effect.reason].join(':'); } + case 'Impure': + case 'Render': case 'MutateFrozen': case 'MutateGlobal': { return [effect.kind, effect.place.identifier.id].join(':'); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts index 1a0fbaa803b..c3e7f52cc1e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts @@ -108,7 +108,9 @@ export function inferMutationAliasingFunctionEffects( ]); } else if ( effect.kind === 'MutateFrozen' || - effect.kind === 'MutateGlobal' + effect.kind === 'MutateGlobal' || + effect.kind === 'Impure' || + effect.kind === 'Render' ) { effects.push(effect); } 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 3fd29297e5e..cd559baa92c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -66,6 +66,7 @@ export function inferMutationAliasingRanges( kind: MutationKind; place: Place; }> = []; + const renders: Array<{index: number; place: Place}> = []; let index = 0; @@ -160,9 +161,12 @@ export function inferMutationAliasingRanges( }); } else if ( effect.kind === 'MutateFrozen' || - effect.kind === 'MutateGlobal' + effect.kind === 'MutateGlobal' || + effect.kind === 'Impure' ) { errors.push(effect.error); + } else if (effect.kind === 'Render') { + renders.push({index: index++, place: effect.place}); } } } @@ -209,6 +213,9 @@ export function inferMutationAliasingRanges( errors, ); } + for (const render of renders) { + state.render(render.index, render.place.identifier, errors); + } if (DEBUG) { console.log(pretty([...state.nodes.keys()])); } @@ -357,6 +364,8 @@ export function inferMutationAliasingRanges( // no-op, Read is the default break; } + case 'Impure': + case 'Render': case 'MutateFrozen': case 'MutateGlobal': { // no-op @@ -440,6 +449,7 @@ export function inferMutationAliasingRanges( function appendFunctionErrors(errors: CompilerError, fn: HIRFunction): void { for (const effect of fn.aliasingEffects ?? []) { switch (effect.kind) { + case 'Impure': case 'MutateFrozen': case 'MutateGlobal': { errors.push(effect.error); @@ -530,6 +540,43 @@ class AliasingState { } } + render(index: number, start: Identifier, errors: CompilerError): void { + const seen = new Set(); + const queue: Array = [start]; + while (queue.length !== 0) { + const current = queue.pop()!; + if (seen.has(current)) { + continue; + } + seen.add(current); + const node = this.nodes.get(current); + if (node == null || node.transitive != null || node.local != null) { + continue; + } + if (node.value.kind === 'Function') { + appendFunctionErrors(errors, node.value.function); + } + for (const [alias, when] of node.createdFrom) { + if (when >= index) { + continue; + } + queue.push(alias); + } + for (const [alias, when] of node.aliases) { + if (when >= index) { + continue; + } + queue.push(alias); + } + for (const [capture, when] of node.captures) { + if (when >= index) { + continue; + } + queue.push(capture); + } + } + } + mutate( index: number, start: Identifier, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md index 3861b16e90d..3f0b5530ee2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enableNewMutationAliasingModel:false function Component() { const foo = () => { someGlobal = true; @@ -15,13 +16,13 @@ function Component() { ## Error ``` - 1 | function Component() { - 2 | const foo = () => { -> 3 | someGlobal = true; - | ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3) - 4 | }; - 5 | return
; - 6 | } + 2 | function Component() { + 3 | const foo = () => { +> 4 | someGlobal = true; + | ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4) + 5 | }; + 6 | return
; + 7 | } ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js index 1eea9267b50..e749f10f78f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js @@ -1,3 +1,4 @@ +// @enableNewMutationAliasingModel:false function Component() { const foo = () => { someGlobal = true; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md index a67d467df8c..0fb17a8f6ea 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render.expect.md @@ -20,7 +20,7 @@ function Component() { 2 | 3 | function Component() { > 4 | const date = Date.now(); - | ^^^^^^^^ InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Date.now` is an impure function whose results may change on every call (4:4) + | ^^^^^^^^^^ InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Date.now` is an impure function whose results may change on every call (4:4) InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `performance.now` is an impure function whose results may change on every call (5:5) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md index 1ab2a46afe8..65292c65e99 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.expect.md @@ -2,6 +2,7 @@ ## Input ```javascript +// @enableNewMutationAliasingModel:false function Foo() { const x = () => { window.href = 'foo'; @@ -21,13 +22,13 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` - 1 | function Foo() { - 2 | const x = () => { -> 3 | window.href = 'foo'; - | ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (3:3) - 4 | }; - 5 | const y = {x}; - 6 | return ; + 2 | function Foo() { + 3 | const x = () => { +> 4 | window.href = 'foo'; + | ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (4:4) + 5 | }; + 6 | const y = {x}; + 7 | return ; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js index b3c936a2a28..d95a0a6265c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.object-capture-global-mutation.js @@ -1,3 +1,4 @@ +// @enableNewMutationAliasingModel:false function Foo() { const x = () => { window.href = 'foo'; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md index e310d0d71e0..df9b5e58f8b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md @@ -8,7 +8,6 @@ import {arrayPush, Stringify} from 'shared-runtime'; function Component({prop1, prop2}) { 'use memo'; - // we'll ultimately extract the item from this array as z, and mutate later let x = [{value: prop1}]; let z; while (x.length < 2) { @@ -17,27 +16,25 @@ function Component({prop1, prop2}) { // this mutation occurs before the reassigned value arrayPush(x, {value: prop2}); - // this condition will never be true, so x doesn't get reassigned - if (x[0].value === null) { + if (x[0].value === prop1) { x = [{value: prop2}]; const y = x; z = y[0]; } } - // the code is set up so that z will always be the value from the original x z.other = true; return ; } export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{prop1: 0, prop2: 0}], + params: [{prop1: 0, prop2: 'a'}], sequentialRenders: [ - {prop1: 0, prop2: 0}, - {prop1: 1, prop2: 0}, - {prop1: 1, prop2: 1}, - {prop1: 0, prop2: 1}, - {prop1: 0, prop2: 0}, + {prop1: 0, prop2: 'a'}, + {prop1: 1, prop2: 'a'}, + {prop1: 1, prop2: 'b'}, + {prop1: 0, prop2: 'b'}, + {prop1: 0, prop2: 'a'}, ], }; @@ -58,7 +55,7 @@ function Component(t0) { let x = [{ value: prop1 }]; while (x.length < 2) { arrayPush(x, { value: prop2 }); - if (x[0].value === null) { + if (x[0].value === prop1) { x = [{ value: prop2 }]; const y = x; z = y[0]; @@ -85,21 +82,21 @@ function Component(t0) { export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{ prop1: 0, prop2: 0 }], + params: [{ prop1: 0, prop2: "a" }], sequentialRenders: [ - { prop1: 0, prop2: 0 }, - { prop1: 1, prop2: 0 }, - { prop1: 1, prop2: 1 }, - { prop1: 0, prop2: 1 }, - { prop1: 0, prop2: 0 }, + { prop1: 0, prop2: "a" }, + { prop1: 1, prop2: "a" }, + { prop1: 1, prop2: "b" }, + { prop1: 0, prop2: "b" }, + { prop1: 0, prop2: "a" }, ], }; ``` ### Eval output -(kind: ok) [[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]] -[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]] -[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]] -[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]] -[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]] \ No newline at end of file +(kind: ok)
{"z":{"value":"a","other":true}}
+
{"z":{"value":"a","other":true}}
+
{"z":{"value":"b","other":true}}
+
{"z":{"value":"b","other":true}}
+
{"z":{"value":"a","other":true}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js index bda11b50019..042cae823f7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js @@ -22,14 +22,14 @@ function Component({prop1, prop2}) { return ; } -// export const FIXTURE_ENTRYPOINT = { -// fn: Component, -// params: [{prop1: 0, prop2: 0}], -// sequentialRenders: [ -// {prop1: 0, prop2: 0}, -// {prop1: 1, prop2: 0}, -// {prop1: 1, prop2: 1}, -// {prop1: 0, prop2: 1}, -// {prop1: 0, prop2: 0}, -// ], -// }; +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{prop1: 0, prop2: 'a'}], + sequentialRenders: [ + {prop1: 0, prop2: 'a'}, + {prop1: 1, prop2: 'a'}, + {prop1: 1, prop2: 'b'}, + {prop1: 0, prop2: 'b'}, + {prop1: 0, prop2: 'a'}, + ], +};