From 7475d568da137b661ce23edc24446871d58c67ef Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 Aug 2024 10:52:36 -0700 Subject: [PATCH] [wip][compiler] Infer optional dependencies Updates PropagateScopeDeps and DeriveMinimalDeps to understand optional dependency paths (`a?.b`). There a few key pieces to this: In PropagateScopeDeps we jump through some hoops to work around the awkward structure of nested OptionalExpressions. This is much easier in HIR form, but I managed to get this pretty close and i think it will be landable with further cleanup. A good chunk of this is avoiding prematurely registering a value as a dependency - there are a bunch of indirections in the ReactiveFunction structure: ``` t0 = OptionalExpression SequenceExpression t0 = Sequence ... LoadLocal t0 ``` Where if at any point we call `visitOperand()` we'll prematurely register a dependency instead of declareProperty(). The other bit is that optionals can be optional=false for nested member expressions where not all the parts are actually optional (`foo.bar?.bar.call()`). And of course, parts of an optional chain can still be conditional even when optional=true (for example the `x` in `foo.bar?.[x]?.baz`). Not all of this is tested yet so there are likely bugs still. The other bit is DeriveMinimalDeps, which is thankfully easier. We add OptionalAccess and OptionalDep and update the merge and reducing logic for these cases. There is probably still more to update though, for things like merging subtrees. There are a lot of ternaries that assume a result can be exactly one of two states (conditional/unconditional, dependency/access) and these assumptions don't hold anymore. I'd like to refactor to dependency/access separate from conditional/optional/unconditional. Also, the reducing logic isn't quite right: once a child is optional we keep inferring all the parents as optional too, losing some precision. I need to adjust the reducing logic to let children decide whether their path token is optional or not. ghstack-source-id: 207842ac64560cf0f93ec96eb9ae1f17c62493ac Pull Request resolved: https://github.com/facebook/react/pull/30819 --- .../src/HIR/Environment.ts | 8 + .../src/HIR/PrintHIR.ts | 2 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 14 +- .../DeriveMinimalDependencies.ts | 165 ++++++++--- .../ReactiveScopes/PrintReactiveFunction.ts | 2 +- .../PropagateScopeDependencies.ts | 271 ++++++++++++++---- ...al-member-expression-as-memo-dep.expect.md | 32 --- ...-optional-member-expression-as-memo-dep.js | 7 - ...al-member-expression-as-memo-dep.expect.md | 48 ++++ .../optional-member-expression-as-memo-dep.js | 7 + ...ession-single-with-unconditional.expect.md | 65 +++++ ...er-expression-single-with-unconditional.js | 11 + ...ptional-member-expression-single.expect.md | 63 ++++ .../optional-member-expression-single.js | 10 + ...ession-with-conditional-optional.expect.md | 74 +++++ ...er-expression-with-conditional-optional.js | 15 + ...mber-expression-with-conditional.expect.md | 74 +++++ ...onal-member-expression-with-conditional.js | 15 + 18 files changed, 755 insertions(+), 128 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 12c741641c7e0..a13ebb6aac878 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -224,6 +224,14 @@ const EnvironmentConfigSchema = z.object({ enableReactiveScopesInHIR: z.boolean().default(true), + /** + * Enables inference of optional dependency chains. Without this flag + * a property chain such as `props?.items?.foo` will infer as a dep on + * just `props`. With this flag enabled, we'll infer that full path as + * the dependency. + */ + enableOptionalDependencies: z.boolean().default(false), + /* * Enable validation of hooks to partially check that the component honors the rules of hooks. * When disabled, the component is assumed to follow the rules (though the Babel plugin looks 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 ecf0b5f0c6041..c2db20c5099a1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -191,7 +191,7 @@ export function printTerminal(terminal: Terminal): Array | string { case 'branch': { value = `[${terminal.id}] Branch (${printPlace(terminal.test)}) then:bb${ terminal.consequent - } else:bb${terminal.alternate}`; + } else:bb${terminal.alternate} fallthrough:bb${terminal.fallthrough}`; break; } case 'logical': { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 73330b959e018..2df7b5ed1c7fd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -1446,9 +1446,19 @@ function codegenDependency( dependency: ReactiveScopeDependency, ): t.Expression { let object: t.Expression = convertIdentifier(dependency.identifier); - if (dependency.path !== null) { + if (dependency.path.length !== 0) { + const hasOptional = dependency.path.some(path => path.optional); for (const path of dependency.path) { - object = t.memberExpression(object, t.identifier(path.property)); + if (hasOptional) { + object = t.optionalMemberExpression( + object, + t.identifier(path.property), + false, + path.optional, + ); + } else { + object = t.memberExpression(object, t.identifier(path.property)); + } } } return object; diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts index dcf67a36e5c6b..c7e16cce7ac86 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -60,13 +60,14 @@ export class ReactiveScopeDependencyTree { const {path} = dep; let currNode = this.#getOrCreateRoot(dep.identifier); - const accessType = inConditional - ? PropertyAccessType.ConditionalAccess - : PropertyAccessType.UnconditionalAccess; - for (const item of path) { // all properties read 'on the way' to a dependency are marked as 'access' let currChild = getOrMakeProperty(currNode, item.property); + const accessType = inConditional + ? PropertyAccessType.ConditionalAccess + : item.optional + ? PropertyAccessType.OptionalAccess + : PropertyAccessType.UnconditionalAccess; currChild.accessType = merge(currChild.accessType, accessType); currNode = currChild; } @@ -77,7 +78,9 @@ export class ReactiveScopeDependencyTree { */ const depType = inConditional ? PropertyAccessType.ConditionalDependency - : PropertyAccessType.UnconditionalDependency; + : isOptional(currNode.accessType) + ? PropertyAccessType.OptionalDependency + : PropertyAccessType.UnconditionalDependency; currNode.accessType = merge(currNode.accessType, depType); } @@ -85,10 +88,12 @@ export class ReactiveScopeDependencyTree { deriveMinimalDependencies(): Set { const results = new Set(); for (const [rootId, rootNode] of this.#roots.entries()) { - const deps = deriveMinimalDependenciesInSubtree(rootNode); + const deps = deriveMinimalDependenciesInSubtree(rootNode, null); CompilerError.invariant( deps.every( - dep => dep.accessType === PropertyAccessType.UnconditionalDependency, + dep => + dep.accessType === PropertyAccessType.UnconditionalDependency || + dep.accessType == PropertyAccessType.OptionalDependency, ), { reason: @@ -173,6 +178,27 @@ export class ReactiveScopeDependencyTree { } return res.flat().join('\n'); } + + debug(): string { + const buf: Array = [`tree() [`]; + for (const [rootId, rootNode] of this.#roots) { + buf.push(`${printIdentifier(rootId)} (${rootNode.accessType}):`); + this.#debugImpl(buf, rootNode, 1); + } + buf.push(']'); + return buf.length > 2 ? buf.join('\n') : buf.join(''); + } + + #debugImpl( + buf: Array, + node: DependencyNode, + depth: number = 0, + ): void { + for (const [property, childNode] of node.properties) { + buf.push(`${' '.repeat(depth)}.${property} (${childNode.accessType}):`); + this.#debugImpl(buf, childNode, depth + 1); + } + } } /* @@ -196,8 +222,10 @@ export class ReactiveScopeDependencyTree { */ enum PropertyAccessType { ConditionalAccess = 'ConditionalAccess', + OptionalAccess = 'OptionalAccess', UnconditionalAccess = 'UnconditionalAccess', ConditionalDependency = 'ConditionalDependency', + OptionalDependency = 'OptionalDependency', UnconditionalDependency = 'UnconditionalDependency', } @@ -211,9 +239,16 @@ function isUnconditional(access: PropertyAccessType): boolean { function isDependency(access: PropertyAccessType): boolean { return ( access === PropertyAccessType.ConditionalDependency || + access === PropertyAccessType.OptionalDependency || access === PropertyAccessType.UnconditionalDependency ); } +function isOptional(access: PropertyAccessType): boolean { + return ( + access === PropertyAccessType.OptionalAccess || + access === PropertyAccessType.OptionalDependency + ); +} function merge( access1: PropertyAccessType, @@ -222,6 +257,7 @@ function merge( const resultIsUnconditional = isUnconditional(access1) || isUnconditional(access2); const resultIsDependency = isDependency(access1) || isDependency(access2); + const resultIsOptional = isOptional(access1) || isOptional(access2); /* * Straightforward merge. @@ -237,6 +273,12 @@ function merge( } else { return PropertyAccessType.UnconditionalAccess; } + } else if (resultIsOptional) { + if (resultIsDependency) { + return PropertyAccessType.OptionalDependency; + } else { + return PropertyAccessType.OptionalAccess; + } } else { if (resultIsDependency) { return PropertyAccessType.ConditionalDependency; @@ -256,19 +298,34 @@ type ReduceResultNode = { accessType: PropertyAccessType; }; -const promoteUncondResult = [ - { +function promoteResult( + accessType: PropertyAccessType, + path: {property: string; optional: boolean} | null, +): Array { + const result: ReduceResultNode = { relativePath: [], - accessType: PropertyAccessType.UnconditionalDependency, - }, -]; + accessType, + }; + if (path !== null) { + result.relativePath.push(path); + } + return [result]; +} -const promoteCondResult = [ - { - relativePath: [], - accessType: PropertyAccessType.ConditionalDependency, - }, -]; +function prependPath( + results: Array, + path: {property: string; optional: boolean} | null, +): Array { + if (path === null) { + return results; + } + return results.map(result => { + return { + accessType: result.accessType, + relativePath: [path, ...result.relativePath], + }; + }); +} /* * Recursively calculates minimal dependencies in a subtree. @@ -277,42 +334,76 @@ const promoteCondResult = [ */ function deriveMinimalDependenciesInSubtree( dep: DependencyNode, + property: string | null, ): Array { const results: Array = []; for (const [childName, childNode] of dep.properties) { - const childResult = deriveMinimalDependenciesInSubtree(childNode).map( - ({relativePath, accessType}) => { - return { - relativePath: [ - {property: childName, optional: false}, - ...relativePath, - ], - accessType, - }; - }, + const childResult = deriveMinimalDependenciesInSubtree( + childNode, + childName, ); results.push(...childResult); } switch (dep.accessType) { case PropertyAccessType.UnconditionalDependency: { - return promoteUncondResult; + return promoteResult( + PropertyAccessType.UnconditionalDependency, + property !== null ? {property, optional: false} : null, + ); } case PropertyAccessType.UnconditionalAccess: { if ( results.every( ({accessType}) => - accessType === PropertyAccessType.UnconditionalDependency, + accessType === PropertyAccessType.UnconditionalDependency || + accessType === PropertyAccessType.OptionalDependency, ) ) { // all children are unconditional dependencies, return them to preserve granularity - return results; + return prependPath( + results, + property !== null ? {property, optional: false} : null, + ); } else { /* * at least one child is accessed conditionally, so this node needs to be promoted to * unconditional dependency */ - return promoteUncondResult; + return promoteResult( + PropertyAccessType.UnconditionalDependency, + property !== null ? {property, optional: false} : null, + ); + } + } + case PropertyAccessType.OptionalDependency: { + return promoteResult( + PropertyAccessType.OptionalDependency, + property !== null ? {property, optional: true} : null, + ); + } + case PropertyAccessType.OptionalAccess: { + if ( + results.every( + ({accessType}) => + accessType === PropertyAccessType.UnconditionalDependency || + accessType === PropertyAccessType.OptionalDependency, + ) + ) { + // all children are unconditional dependencies, return them to preserve granularity + return prependPath( + results, + property !== null ? {property, optional: true} : null, + ); + } else { + /* + * at least one child is accessed conditionally, so this node needs to be promoted to + * unconditional dependency + */ + return promoteResult( + PropertyAccessType.OptionalDependency, + property !== null ? {property, optional: true} : null, + ); } } case PropertyAccessType.ConditionalAccess: @@ -328,13 +419,19 @@ function deriveMinimalDependenciesInSubtree( * unconditional access. * Truncate results of child nodes here, since we shouldn't access them anyways */ - return promoteCondResult; + return promoteResult( + PropertyAccessType.ConditionalDependency, + property !== null ? {property, optional: true} : null, + ); } else { /* * at least one child is accessed unconditionally, so this node can be promoted to * unconditional dependency */ - return promoteUncondResult; + return promoteResult( + PropertyAccessType.UnconditionalDependency, + property !== null ? {property, optional: true} : null, + ); } } default: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts index 80395a2e0ea41..b5aa44ead095d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts @@ -113,7 +113,7 @@ export function printDependency(dependency: ReactiveScopeDependency): string { const identifier = printIdentifier(dependency.identifier) + printType(dependency.identifier.type); - return `${identifier}${dependency.path.map(token => `.${token.property}`).join('')}`; + return `${identifier}${dependency.path.map(token => `${token.optional ? '?.' : '.'}${token.property}`).join('')}`; } export function printReactiveInstructions( diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts index 8fd324ae2c9aa..4a054ab84c0bf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -6,6 +6,7 @@ */ import {CompilerError} from '../CompilerError'; +import {Environment} from '../HIR'; import { areEqualPaths, BlockId, @@ -22,6 +23,7 @@ import { PrunedReactiveScopeBlock, ReactiveFunction, ReactiveInstruction, + ReactiveOptionalCallValue, ReactiveScope, ReactiveScopeBlock, ReactiveScopeDependency, @@ -65,11 +67,7 @@ export function propagateScopeDependencies(fn: ReactiveFunction): void { }); } } - visitReactiveFunction( - fn, - new PropagationVisitor(fn.env.config.enableTreatFunctionDepsAsConditional), - context, - ); + visitReactiveFunction(fn, new PropagationVisitor(fn.env), context); } type TemporariesUsedOutsideDefiningScope = { @@ -465,6 +463,7 @@ class Context { #getProperty( object: Place, property: string, + optional: boolean, ): ReactiveScopePropertyDependency { const resolvedObject = this.resolveTemporary(object); const resolvedDependency = this.#properties.get(resolvedObject.identifier); @@ -485,13 +484,18 @@ class Context { }; } - objectDependency.path.push({property, optional: false}); + objectDependency.path.push({property, optional}); return objectDependency; } - declareProperty(lvalue: Place, object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property); + declareProperty( + lvalue: Place, + object: Place, + property: string, + optional: boolean, + ): void { + const nextDependency = this.#getProperty(object, property, optional); this.#properties.set(lvalue.identifier, nextDependency); } @@ -571,8 +575,8 @@ class Context { this.visitDependency(dependency); } - visitProperty(object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property); + visitProperty(object: Place, property: string, optional: boolean): void { + const nextDependency = this.#getProperty(object, property, optional); this.visitDependency(nextDependency); } @@ -671,12 +675,11 @@ class Context { } class PropagationVisitor extends ReactiveFunctionVisitor { - enableTreatFunctionDepsAsConditional = false; + env: Environment; - constructor(enableTreatFunctionDepsAsConditional: boolean) { + constructor(env: Environment) { super(); - this.enableTreatFunctionDepsAsConditional = - enableTreatFunctionDepsAsConditional; + this.env = env; } override visitScope(scope: ReactiveScopeBlock, context: Context): void { @@ -744,51 +747,212 @@ class PropagationVisitor extends ReactiveFunctionVisitor { }); } + extractOptionalProperty( + context: Context, + optionalValue: ReactiveOptionalCallValue, + lvalue: Place, + ): { + lvalue: Place; + object: Place; + property: string; + optional: boolean; + } | null { + const sequence = optionalValue.value; + CompilerError.invariant(sequence.kind === 'SequenceExpression', { + reason: 'Expected OptionalExpression value to be a SequenceExpression', + description: `Found a \`${sequence.kind}\``, + loc: sequence.loc, + }); + /** + * Base case: inner ` "." or "?."" ` + *``` + * = OptionalExpression optional=true (`optionalValue` is here) + * Sequence (`sequence` is here) + * t0 = LoadLocal + * Sequence + * t1 = PropertyLoad t0 . + * LoadLocal t1 + * ``` + */ + if ( + sequence.instructions.length === 1 && + sequence.instructions[0].value.kind === 'LoadLocal' && + sequence.instructions[0].lvalue !== null && + sequence.instructions[0].value.place.identifier.name !== null && + !context.isUsedOutsideDeclaringScope(sequence.instructions[0].lvalue) && + sequence.value.kind === 'SequenceExpression' && + sequence.value.instructions.length === 1 && + sequence.value.instructions[0].value.kind === 'PropertyLoad' && + sequence.value.instructions[0].value.object.identifier.id === + sequence.instructions[0].lvalue.identifier.id && + sequence.value.instructions[0].lvalue !== null && + sequence.value.value.kind === 'LoadLocal' && + sequence.value.value.place.identifier.id === + sequence.value.instructions[0].lvalue.identifier.id + ) { + context.declareTemporary( + sequence.instructions[0].lvalue, + sequence.instructions[0].value.place, + ); + const propertyLoad = sequence.value.instructions[0].value; + return { + lvalue, + object: propertyLoad.object, + property: propertyLoad.property, + optional: optionalValue.optional, + }; + } + /** + * Composed case: ` "." or "?." ` + * + * This case is convoluted, note how `t0` appears as an lvalue *twice* + * and then is an operand of an intermediate LoadLocal and then the + * object of the final PropertyLoad: + * + * ``` + * = OptionalExpression optional=false (`optionalValue` is here) + * Sequence (`sequence` is here) + * t0 = Sequence + * t0 = + * + * LoadLocal t0 + * Sequence + * t1 = PropertyLoad t0. + * LoadLocal t1 + * ``` + */ + if ( + sequence.instructions.length === 1 && + sequence.instructions[0].value.kind === 'SequenceExpression' && + sequence.instructions[0].value.instructions.length === 1 && + sequence.instructions[0].value.instructions[0].lvalue !== null && + sequence.instructions[0].value.instructions[0].value.kind === + 'OptionalExpression' && + sequence.instructions[0].value.value.kind === 'LoadLocal' && + sequence.instructions[0].value.value.place.identifier.id === + sequence.instructions[0].value.instructions[0].lvalue.identifier.id && + sequence.value.kind === 'SequenceExpression' && + sequence.value.instructions.length === 1 && + sequence.value.instructions[0].lvalue !== null && + sequence.value.instructions[0].value.kind === 'PropertyLoad' && + sequence.value.instructions[0].value.object.identifier.id === + sequence.instructions[0].value.value.place.identifier.id && + sequence.value.value.kind === 'LoadLocal' && + sequence.value.value.place.identifier.id === + sequence.value.instructions[0].lvalue.identifier.id + ) { + const {lvalue: innerLvalue, value: innerOptional} = + sequence.instructions[0].value.instructions[0]; + const innerProperty = this.extractOptionalProperty( + context, + innerOptional, + innerLvalue, + ); + if (innerProperty === null) { + return null; + } + context.declareProperty( + innerProperty.lvalue, + innerProperty.object, + innerProperty.property, + innerProperty.optional, + ); + const propertyLoad = sequence.value.instructions[0].value; + return { + lvalue, + object: propertyLoad.object, + property: propertyLoad.property, + optional: optionalValue.optional, + }; + } + return null; + } + + visitOptionalExpression( + context: Context, + id: InstructionId, + value: ReactiveOptionalCallValue, + lvalue: Place | null, + ): void { + /** + * If this is the first optional=true optional in a recursive OptionalExpression + * subtree, we check to see if the subtree is of the form: + * ``` + * NestedOptional = + * ` . / ?. ` + * ` . / ?. ` + * ``` + * + * Ie strictly a chain like `foo?.bar?.baz` or `a?.b.c`. If the subtree contains + * any other types of expressions - for example `foo?.[makeKey(a)]` - then this + * will return null and we'll go to the default handling below. + * + * If the tree does match the NestedOptional shape, then we'll have recorded + * a sequence of declareProperty calls, and the final visitProperty call here + * will record that optional chain as a dependency (since we know it's about + * to be referenced via its lvalue which is non-null). + */ + if ( + lvalue !== null && + value.optional && + this.env.config.enableOptionalDependencies + ) { + const inner = this.extractOptionalProperty(context, value, lvalue); + if (inner !== null) { + context.visitProperty(inner.object, inner.property, inner.optional); + return; + } + } + + // Otherwise we treat everything after the optional as conditional + const inner = value.value; + /* + * OptionalExpression value is a SequenceExpression where the instructions + * represent the code prior to the `?` and the final value represents the + * conditional code that follows. + */ + CompilerError.invariant(inner.kind === 'SequenceExpression', { + reason: 'Expected OptionalExpression value to be a SequenceExpression', + description: `Found a \`${value.kind}\``, + loc: value.loc, + suggestions: null, + }); + // Instructions are the unconditionally executed portion before the `?` + for (const instr of inner.instructions) { + this.visitInstruction(instr, context); + } + // The final value is the conditional portion following the `?` + context.enterConditional(() => { + this.visitReactiveValue(context, id, inner.value, null); + }); + } + visitReactiveValue( context: Context, id: InstructionId, value: ReactiveValue, + lvalue: Place | null, ): void { switch (value.kind) { case 'OptionalExpression': { - const inner = value.value; - /* - * OptionalExpression value is a SequenceExpression where the instructions - * represent the code prior to the `?` and the final value represents the - * conditional code that follows. - */ - CompilerError.invariant(inner.kind === 'SequenceExpression', { - reason: - 'Expected OptionalExpression value to be a SequenceExpression', - description: `Found a \`${value.kind}\``, - loc: value.loc, - suggestions: null, - }); - // Instructions are the unconditionally executed portion before the `?` - for (const instr of inner.instructions) { - this.visitInstruction(instr, context); - } - // The final value is the conditional portion following the `?` - context.enterConditional(() => { - this.visitReactiveValue(context, id, inner.value); - }); + this.visitOptionalExpression(context, id, value, lvalue); break; } case 'LogicalExpression': { - this.visitReactiveValue(context, id, value.left); + this.visitReactiveValue(context, id, value.left, null); context.enterConditional(() => { - this.visitReactiveValue(context, id, value.right); + this.visitReactiveValue(context, id, value.right, null); }); break; } case 'ConditionalExpression': { - this.visitReactiveValue(context, id, value.test); + this.visitReactiveValue(context, id, value.test, null); const consequentDeps = context.enterConditional(() => { - this.visitReactiveValue(context, id, value.consequent); + this.visitReactiveValue(context, id, value.consequent, null); }); const alternateDeps = context.enterConditional(() => { - this.visitReactiveValue(context, id, value.alternate); + this.visitReactiveValue(context, id, value.alternate, null); }); context.promoteDepsFromExhaustiveConditionals([ consequentDeps, @@ -804,7 +968,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor { break; } case 'FunctionExpression': { - if (this.enableTreatFunctionDepsAsConditional) { + if (this.env.config.enableTreatFunctionDepsAsConditional) { context.enterConditional(() => { for (const operand of eachInstructionValueOperand(value)) { context.visitOperand(operand); @@ -851,9 +1015,9 @@ class PropagationVisitor extends ReactiveFunctionVisitor { } } else if (value.kind === 'PropertyLoad') { if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) { - context.declareProperty(lvalue, value.object, value.property); + context.declareProperty(lvalue, value.object, value.property, false); } else { - context.visitProperty(value.object, value.property); + context.visitProperty(value.object, value.property, false); } } else if (value.kind === 'StoreLocal') { context.visitOperand(value.value); @@ -896,7 +1060,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor { }); } } else { - this.visitReactiveValue(context, id, value); + this.visitReactiveValue(context, id, value, lvalue); } } @@ -947,25 +1111,30 @@ class PropagationVisitor extends ReactiveFunctionVisitor { break; } case 'for': { - this.visitReactiveValue(context, terminal.id, terminal.init); - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.init, null); + this.visitReactiveValue(context, terminal.id, terminal.test, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); if (terminal.update !== null) { - this.visitReactiveValue(context, terminal.id, terminal.update); + this.visitReactiveValue( + context, + terminal.id, + terminal.update, + null, + ); } }); break; } case 'for-of': { - this.visitReactiveValue(context, terminal.id, terminal.init); + this.visitReactiveValue(context, terminal.id, terminal.init, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); break; } case 'for-in': { - this.visitReactiveValue(context, terminal.id, terminal.init); + this.visitReactiveValue(context, terminal.id, terminal.init, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); @@ -974,12 +1143,12 @@ class PropagationVisitor extends ReactiveFunctionVisitor { case 'do-while': { this.visitBlock(terminal.loop, context); context.enterConditional(() => { - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.test, null); }); break; } case 'while': { - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.test, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md deleted file mode 100644 index 7e4145a27c16b..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.expect.md +++ /dev/null @@ -1,32 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -function Component(props) { - const data = useMemo(() => { - return props.items?.edges?.nodes ?? []; - }, [props.items?.edges?.nodes]); - return ; -} - -``` - - -## Error - -``` - 1 | // @validatePreserveExistingMemoizationGuarantees - 2 | function Component(props) { -> 3 | const data = useMemo(() => { - | ^^^^^^^ -> 4 | return props.items?.edges?.nodes ?? []; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 5 | }, [props.items?.edges?.nodes]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (3:5) - 6 | return ; - 7 | } - 8 | -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js deleted file mode 100644 index fd8cf0214c87b..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js +++ /dev/null @@ -1,7 +0,0 @@ -// @validatePreserveExistingMemoizationGuarantees -function Component(props) { - const data = useMemo(() => { - return props.items?.edges?.nodes ?? []; - }, [props.items?.edges?.nodes]); - return ; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md new file mode 100644 index 0000000000000..c34b79a848ba8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md @@ -0,0 +1,48 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +function Component(props) { + const data = useMemo(() => { + return props?.items.edges?.nodes.map(); + }, [props?.items.edges?.nodes]); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +function Component(props) { + const $ = _c(4); + + props?.items.edges?.nodes; + let t0; + let t1; + if ($[0] !== props?.items.edges?.nodes) { + t1 = props?.items.edges?.nodes.map(); + $[0] = props?.items.edges?.nodes; + $[1] = t1; + } else { + t1 = $[1]; + } + t0 = t1; + const data = t0; + let t2; + if ($[2] !== data) { + t2 = ; + $[2] = data; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js new file mode 100644 index 0000000000000..d82d36b547970 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.js @@ -0,0 +1,7 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +function Component(props) { + const data = useMemo(() => { + return props?.items.edges?.nodes.map(); + }, [props?.items.edges?.nodes]); + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md new file mode 100644 index 0000000000000..d0c4afe459da2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + x.push(props.items); + return x; + }, [props?.items]); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(7); + + props?.items; + let t0; + let x; + if ($[0] !== props.items) { + x = []; + x.push(props?.items); + x.push(props.items); + $[0] = props.items; + $[1] = x; + } else { + x = $[1]; + } + t0 = x; + const data = t0; + const t1 = props?.items; + let t2; + if ($[2] !== t1) { + t2 = [t1]; + $[2] = t1; + $[3] = t2; + } else { + t2 = $[3]; + } + let t3; + if ($[4] !== t2 || $[5] !== data) { + t3 = ; + $[4] = t2; + $[5] = data; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js new file mode 100644 index 0000000000000..c630dd6bb4b9d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single-with-unconditional.js @@ -0,0 +1,11 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + x.push(props.items); + return x; + }, [props?.items]); + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md new file mode 100644 index 0000000000000..a4cf6d767d1c3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + return x; + }, [props?.items]); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(7); + + props?.items; + let t0; + let x; + if ($[0] !== props?.items) { + x = []; + x.push(props?.items); + $[0] = props?.items; + $[1] = x; + } else { + x = $[1]; + } + t0 = x; + const data = t0; + const t1 = props?.items; + let t2; + if ($[2] !== t1) { + t2 = [t1]; + $[2] = t1; + $[3] = t2; + } else { + t2 = $[3]; + } + let t3; + if ($[4] !== t2 || $[5] !== data) { + t3 = ; + $[4] = t2; + $[5] = data; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js new file mode 100644 index 0000000000000..5750d7af3a0e0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-single.js @@ -0,0 +1,10 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + return x; + }, [props?.items]); + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.expect.md new file mode 100644 index 0000000000000..77ded20d939bd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + if (props.cond) { + x.push(props?.items); + } + return x; + }, [props?.items, props.cond]); + return ( + + ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(9); + + props?.items; + let t0; + let x; + if ($[0] !== props?.items || $[1] !== props.cond) { + x = []; + x.push(props?.items); + if (props.cond) { + x.push(props?.items); + } + $[0] = props?.items; + $[1] = props.cond; + $[2] = x; + } else { + x = $[2]; + } + t0 = x; + const data = t0; + + const t1 = props?.items; + let t2; + if ($[3] !== t1 || $[4] !== props.cond) { + t2 = [t1, props.cond]; + $[3] = t1; + $[4] = props.cond; + $[5] = t2; + } else { + t2 = $[5]; + } + let t3; + if ($[6] !== t2 || $[7] !== data) { + t3 = ; + $[6] = t2; + $[7] = data; + $[8] = t3; + } else { + t3 = $[8]; + } + return t3; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.js new file mode 100644 index 0000000000000..760f345e90210 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional-optional.js @@ -0,0 +1,15 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + if (props.cond) { + x.push(props?.items); + } + return x; + }, [props?.items, props.cond]); + return ( + + ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.expect.md new file mode 100644 index 0000000000000..10c23085d8e6b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.expect.md @@ -0,0 +1,74 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + if (props.cond) { + x.push(props.items); + } + return x; + }, [props?.items, props.cond]); + return ( + + ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(9); + + props?.items; + let t0; + let x; + if ($[0] !== props?.items || $[1] !== props.cond) { + x = []; + x.push(props?.items); + if (props.cond) { + x.push(props.items); + } + $[0] = props?.items; + $[1] = props.cond; + $[2] = x; + } else { + x = $[2]; + } + t0 = x; + const data = t0; + + const t1 = props?.items; + let t2; + if ($[3] !== t1 || $[4] !== props.cond) { + t2 = [t1, props.cond]; + $[3] = t1; + $[4] = props.cond; + $[5] = t2; + } else { + t2 = $[5]; + } + let t3; + if ($[6] !== t2 || $[7] !== data) { + t3 = ; + $[6] = t2; + $[7] = data; + $[8] = t3; + } else { + t3 = $[8]; + } + return t3; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.js new file mode 100644 index 0000000000000..3f773f4fe4e4b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-with-conditional.js @@ -0,0 +1,15 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.items); + if (props.cond) { + x.push(props.items); + } + return x; + }, [props?.items, props.cond]); + return ( + + ); +}