@samitouri / QOS-React / commits / a43fc2bcf3

Consistently use new lowering for OptionalMemberExpression

Earlier PRs in the stack change the way we lower OptionalMemberExpression, but only when they ultimately appear inside some OptionalCallExpression. This PR ensures that _all_ OptionalMemberExpressions get the new lowering. Note that one test case has what is arguably a regression, but the new behavior is also reasonable: if we see both `a.b?.c` and `a.b.c.` as dependencies of a scope, we previously inferred `a.b.c` as the dependency, but we now infer `a.b` as the dependency. This isn't as optimal as what we had before, but it also seems good enough for now. Also note that some cases are improved: `foo(a.b?.c)` would previously have taken `a.b` as a dependency, we now take the full value of `a.b?.c` as a dependency - more precise. So overall i'm inclined to land and follow-up on the one regression, since the overall model is more cohesive.

Joe Savona committed May 3, 2023 at 17:10 UTC a43fc2bcf3f4e854796c99e2e9414609ad1a36c7
4 files changed +28 -11
compiler/forget/src/HIR/BuildHIR.ts
+5 -1
@@ -1425,7 +1425,11 @@ function lowerExpression(
1425 }
1426 }
1427 }
1428 - case "OptionalMemberExpression":
1428 + case "OptionalMemberExpression": {
1429 + const expr = exprPath as NodePath<t.OptionalMemberExpression>;
1430 + const { value } = lowerOptionalMemberExpression(builder, expr, null);
1431 + return { kind: "LoadLocal", place: value, loc: value.loc };
1432 + }
1433 case "MemberExpression": {
1434 const expr = exprPath as NodePath<
1435 t.MemberExpression | t.OptionalMemberExpression
compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+13 -1
@@ -525,7 +525,19 @@ function computeMemoizationInputs(
525 rvalues: [value.value],
526 };
527 }
528 - case "OptionalExpression":
528 + case "OptionalExpression": {
529 + // Indirection for the inner value, memoized if the value is
530 + const lvalues = [];
531 + if (lvalue !== null) {
532 + lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional });
533 + }
534 + return {
535 + lvalues: lvalues,
536 + rvalues: [
537 + ...computeMemoizationInputs(value.value, null, options).rvalues,
538 + ],
539 + };
540 + }
541 case "RegExpLiteral":
542 case "FunctionExpression":
543 case "TaggedTemplateExpression":
compiler/forget/src/__tests__/fixtures/compiler/nested-optional-member-expr.expect.md
+8 -7
@@ -18,16 +18,17 @@ import { unstable_useMemoCache as useMemoCache } from "react"; // We should code
18 // (i.e. placing `?` in the correct PropertyLoad)
19 function Component(props) {
20 const $ = useMemoCache(2);
21 - const c_0 = $[0] !== props.a;
22 - let t0;
21 + const t0 = props.a?.b.c.d;
22 + const c_0 = $[0] !== t0;
23 + let t1;
24 if (c_0) {
24 - t0 = foo(props.a?.b.c.d);
25 - $[0] = props.a;
26 - $[1] = t0;
25 + t1 = foo(t0);
26 + $[0] = t0;
27 + $[1] = t1;
28 } else {
28 - t0 = $[1];
29 + t1 = $[1];
30 }
30 - const x = t0;
31 + const x = t1;
32 return x;
33 }
34
compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-cond-memberexpr-join.expect.md
+2 -2
@@ -38,13 +38,13 @@ import { unstable_useMemoCache as useMemoCache } from "react"; // To preserve th
38
39 function Component(props) {
40 const $ = useMemoCache(2);
41 - const c_0 = $[0] !== props.a.b;
41 + const c_0 = $[0] !== props.a;
42 let x;
43 if (c_0) {
44 x = [];
45 x.push(props.a?.b);
46 x.push(props.a.b.c);
47 - $[0] = props.a.b;
47 + $[0] = props.a;
48 $[1] = x;
49 } else {
50 x = $[1];