From 34f5c6eb70db05b2411f48848a3956d6ec7ad68f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 Aug 2024 09:41:58 -0700 Subject: [PATCH] [compiler] Handle optional where innermost property access is non-optional Handles an additional case as part of testing combinations of the same path being accessed in different places with different segments as optional/unconditional. [ghstack-poisoned] --- .../PropagateScopeDependencies.ts | 82 ++++++++++++++++++- ...nverted-optionals-parallel-paths.expect.md | 46 +++++++++++ ...ssion-inverted-optionals-parallel-paths.js | 11 +++ 3 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.js 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 4a054ab84c0bf..dc1142b271e77 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -764,7 +764,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor { loc: sequence.loc, }); /** - * Base case: inner ` "." or "?."" ` + * Base case: inner ` "?." ` *``` * = OptionalExpression optional=true (`optionalValue` is here) * Sequence (`sequence` is here) @@ -776,8 +776,8 @@ class PropagationVisitor extends ReactiveFunctionVisitor { */ if ( sequence.instructions.length === 1 && - sequence.instructions[0].value.kind === 'LoadLocal' && sequence.instructions[0].lvalue !== null && + sequence.instructions[0].value.kind === 'LoadLocal' && sequence.instructions[0].value.place.identifier.name !== null && !context.isUsedOutsideDeclaringScope(sequence.instructions[0].lvalue) && sequence.value.kind === 'SequenceExpression' && @@ -803,7 +803,83 @@ class PropagationVisitor extends ReactiveFunctionVisitor { }; } /** - * Composed case: ` "." or "?." ` + * Base case 2: inner ` "." "?." + * ``` + * = OptionalExpression optional=true (`optionalValue` is here) + * Sequence (`sequence` is here) + * t0 = Sequence + * t1 = LoadLocal + * ... // see note + * PropertyLoad t1 . + * [46] Sequence + * t2 = PropertyLoad t0 . + * [46] LoadLocal t2 + * ``` + * + * Note that it's possible to have additional inner chained non-optional + * property loads at "...", from an expression like `a?.b.c.d.e`. We could + * expand to support this case by relaxing the check on the inner sequence + * length, ensuring all instructions after the first LoadLocal are PropertyLoad + * and then iterating to ensure that the lvalue of the previous is always + * the object of the next PropertyLoad, w the final lvalue as the object + * of the sequence.value's object. + * + * But this case is likely rare in practice, usually once you're optional + * chaining all property accesses are optional (not `a?.b.c` but `a?.b?.c`). + * Also, HIR-based PropagateScopeDeps will handle this case so it doesn't + * seem worth it to optimize for that edge-case here. + */ + if ( + sequence.instructions.length === 1 && + sequence.instructions[0].lvalue !== null && + 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 === + 'LoadLocal' && + sequence.instructions[0].value.instructions[0].value.place.identifier + .name !== null && + !context.isUsedOutsideDeclaringScope( + sequence.instructions[0].value.instructions[0].lvalue, + ) && + sequence.instructions[0].value.value.kind === 'PropertyLoad' && + sequence.instructions[0].value.value.object.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].lvalue.identifier.id && + sequence.value.value.kind === 'LoadLocal' && + sequence.value.value.place.identifier.id === + sequence.value.instructions[0].lvalue.identifier.id + ) { + // LoadLocal + context.declareTemporary( + sequence.instructions[0].value.instructions[0].lvalue, + sequence.instructions[0].value.instructions[0].value.place, + ); + // PropertyLoad . (the inner non-optional property) + context.declareProperty( + sequence.instructions[0].lvalue, + sequence.instructions[0].value.value.object, + sequence.instructions[0].value.value.property, + false, + ); + const propertyLoad = sequence.value.instructions[0].value; + return { + lvalue, + object: propertyLoad.object, + property: propertyLoad.property, + optional: optionalValue.optional, + }; + } + + /** + * Composed case: + * - ` "." or "?." ` + * - ` "." 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 diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.expect.md new file mode 100644 index 0000000000000..98fcfbe7f0f6f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.a.b?.c.d?.e); + x.push(props.a?.b.c?.d.e); + return x; + }, [props.a.b.c.d.e]); + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import { ValidateMemoization } from "shared-runtime"; +function Component(props) { + const $ = _c(2); + let t0; + + const x$0 = []; + x$0.push(props?.a.b?.c.d?.e); + x$0.push(props.a?.b.c?.d.e); + t0 = x$0; + let t1; + if ($[0] !== props.a.b.c.d.e) { + t1 = ; + $[0] = props.a.b.c.d.e; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +``` + +### 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-inverted-optionals-parallel-paths.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.js new file mode 100644 index 0000000000000..563b0bbf0f418 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.js @@ -0,0 +1,11 @@ +// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies +import {ValidateMemoization} from 'shared-runtime'; +function Component(props) { + const data = useMemo(() => { + const x = []; + x.push(props?.a.b?.c.d?.e); + x.push(props.a?.b.c?.d.e); + return x; + }, [props.a.b.c.d.e]); + return ; +}