[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-source-id: ace777fcbb98fa8f41b977d0aec8418f3f58fb7b Pull Request resolved: https://github.com/facebook/react/pull/30836
Joe Savona committed
Aug 28, 2024 at 10:52 UTC
99a4b26e18a71a2ed5af5ec11f4b9bace3882f7e
3 files changed
+136
-3
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts
+79
-3
@@ -764,7 +764,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
764
loc: sequence.loc,
765
});
766
/**
767
- * Base case: inner `<variable> "." or "?."" <property>`
767
+ * Base case: inner `<variable> "?." <property>`
768
*```
769
* <lvalue> = OptionalExpression optional=true (`optionalValue` is here)
770
* Sequence (`sequence` is here)
@@ -776,8 +776,8 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
776
*/
777
if (
778
sequence.instructions.length === 1 &&
779
- sequence.instructions[0].value.kind === 'LoadLocal' &&
779
sequence.instructions[0].lvalue !== null &&
780
+ sequence.instructions[0].value.kind === 'LoadLocal' &&
781
sequence.instructions[0].value.place.identifier.name !== null &&
782
!context.isUsedOutsideDeclaringScope(sequence.instructions[0].lvalue) &&
783
sequence.value.kind === 'SequenceExpression' &&
@@ -803,7 +803,83 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
803
};
804
}
805
/**
806
- * Composed case: `<base-case> "." or "?." <property>`
806
+ * Base case 2: inner `<variable> "." <property1> "?." <property2>
807
+ * ```
808
+ * <lvalue> = OptionalExpression optional=true (`optionalValue` is here)
809
+ * Sequence (`sequence` is here)
810
+ * t0 = Sequence
811
+ * t1 = LoadLocal <variable>
812
+ * ... // see note
813
+ * PropertyLoad t1 . <property1>
814
+ * [46] Sequence
815
+ * t2 = PropertyLoad t0 . <property2>
816
+ * [46] LoadLocal t2
817
+ * ```
818
+ *
819
+ * Note that it's possible to have additional inner chained non-optional
820
+ * property loads at "...", from an expression like `a?.b.c.d.e`. We could
821
+ * expand to support this case by relaxing the check on the inner sequence
822
+ * length, ensuring all instructions after the first LoadLocal are PropertyLoad
823
+ * and then iterating to ensure that the lvalue of the previous is always
824
+ * the object of the next PropertyLoad, w the final lvalue as the object
825
+ * of the sequence.value's object.
826
+ *
827
+ * But this case is likely rare in practice, usually once you're optional
828
+ * chaining all property accesses are optional (not `a?.b.c` but `a?.b?.c`).
829
+ * Also, HIR-based PropagateScopeDeps will handle this case so it doesn't
830
+ * seem worth it to optimize for that edge-case here.
831
+ */
832
+ if (
833
+ sequence.instructions.length === 1 &&
834
+ sequence.instructions[0].lvalue !== null &&
835
+ sequence.instructions[0].value.kind === 'SequenceExpression' &&
836
+ sequence.instructions[0].value.instructions.length === 1 &&
837
+ sequence.instructions[0].value.instructions[0].lvalue !== null &&
838
+ sequence.instructions[0].value.instructions[0].value.kind ===
839
+ 'LoadLocal' &&
840
+ sequence.instructions[0].value.instructions[0].value.place.identifier
841
+ .name !== null &&
842
+ !context.isUsedOutsideDeclaringScope(
843
+ sequence.instructions[0].value.instructions[0].lvalue,
844
+ ) &&
845
+ sequence.instructions[0].value.value.kind === 'PropertyLoad' &&
846
+ sequence.instructions[0].value.value.object.identifier.id ===
847
+ sequence.instructions[0].value.instructions[0].lvalue.identifier.id &&
848
+ sequence.value.kind === 'SequenceExpression' &&
849
+ sequence.value.instructions.length === 1 &&
850
+ sequence.value.instructions[0].lvalue !== null &&
851
+ sequence.value.instructions[0].value.kind === 'PropertyLoad' &&
852
+ sequence.value.instructions[0].value.object.identifier.id ===
853
+ sequence.instructions[0].lvalue.identifier.id &&
854
+ sequence.value.value.kind === 'LoadLocal' &&
855
+ sequence.value.value.place.identifier.id ===
856
+ sequence.value.instructions[0].lvalue.identifier.id
857
+ ) {
858
+ // LoadLocal <variable>
859
+ context.declareTemporary(
860
+ sequence.instructions[0].value.instructions[0].lvalue,
861
+ sequence.instructions[0].value.instructions[0].value.place,
862
+ );
863
+ // PropertyLoad <variable> . <property1> (the inner non-optional property)
864
+ context.declareProperty(
865
+ sequence.instructions[0].lvalue,
866
+ sequence.instructions[0].value.value.object,
867
+ sequence.instructions[0].value.value.property,
868
+ false,
869
+ );
870
+ const propertyLoad = sequence.value.instructions[0].value;
871
+ return {
872
+ lvalue,
873
+ object: propertyLoad.object,
874
+ property: propertyLoad.property,
875
+ optional: optionalValue.optional,
876
+ };
877
+ }
878
+
879
+ /**
880
+ * Composed case:
881
+ * - `<base-case> "." or "?." <property>`
882
+ * - `<composed-case> "." or "?>" <property>`
883
*
884
* This case is convoluted, note how `t0` appears as an lvalue *twice*
885
* and then is an operand of an intermediate LoadLocal and then the
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.expect.md
new
+46
@@ -0,0 +1,46 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies
6
+import {ValidateMemoization} from 'shared-runtime';
7
+function Component(props) {
8
+ const data = useMemo(() => {
9
+ const x = [];
10
+ x.push(props?.a.b?.c.d?.e);
11
+ x.push(props.a?.b.c?.d.e);
12
+ return x;
13
+ }, [props.a.b.c.d.e]);
14
+ return <ValidateMemoization inputs={[props.a.b.c.d.e]} output={x} />;
15
+}
16
+
17
+```
18
+
19
+## Code
20
+
21
+```javascript
22
+import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies
23
+import { ValidateMemoization } from "shared-runtime";
24
+function Component(props) {
25
+ const $ = _c(2);
26
+ let t0;
27
+
28
+ const x$0 = [];
29
+ x$0.push(props?.a.b?.c.d?.e);
30
+ x$0.push(props.a?.b.c?.d.e);
31
+ t0 = x$0;
32
+ let t1;
33
+ if ($[0] !== props.a.b.c.d.e) {
34
+ t1 = <ValidateMemoization inputs={[props.a.b.c.d.e]} output={x} />;
35
+ $[0] = props.a.b.c.d.e;
36
+ $[1] = t1;
37
+ } else {
38
+ t1 = $[1];
39
+ }
40
+ return t1;
41
+}
42
+
43
+```
44
+
45
+### Eval output
46
+(kind: exception) Fixture not implemented
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/optional-member-expression-inverted-optionals-parallel-paths.js
new
+11
@@ -0,0 +1,11 @@
1
+// @validatePreserveExistingMemoizationGuarantees @enableOptionalDependencies
2
+import {ValidateMemoization} from 'shared-runtime';
3
+function Component(props) {
4
+ const data = useMemo(() => {
5
+ const x = [];
6
+ x.push(props?.a.b?.c.d?.e);
7
+ x.push(props.a?.b.c?.d.e);
8
+ return x;
9
+ }, [props.a.b.c.d.e]);
10
+ return <ValidateMemoization inputs={[props.a.b.c.d.e]} output={x} />;
11
+}