[compiler] Infer optional dependencies (behind feature flag) #30819

josephsavona · 2024-08-26T23:18:52Z

Stack from ghstack (oldest at bottom):

Adds a new feature flag, @enableOptionalDependencies which when enabled allows PropagateScopeDeps and DeriveMinimalDeps to infer optional dependency paths (a?.b).

In PropagateScopeDeps we look for specific safe patterns of nested optional member expressions:

<variable> "." / "?." <property>
or <nested> "." / "?." <property>

When we find this pattern we record a dependency on the overall chain, so for example in a?.b.c?.d.map() we would record a dependency on a?.b.c?.d (because the outer .map() portion doesn't match the above structure). If the structure doesn't match - for example with a?.b?.[foo(bar)]?.z - then we fall back to the existing behavior which treats everything after the initial portion as conditional (for that last example we'd continue to record a as the dep).

The other portion is DeriveMinimalDeps, which now represents optional access/dependency states and updates the merge logic to handle them. The order of precedence is unconditional > optional > conditional.

Note: the flag is off by default, but i tried enabling it for all fixtures and the results were all either unchanged or had correct optional deps inferred. I'll also try running it internally with the flag enabled, but i think we can proceed with review and fix-forward any issues identified from that testing.

[ghstack-poisoned]

vercel · 2024-08-26T23:18:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 28, 2024 6:09pm

ghstack-source-id: 50d3523dbdb483cbd994a19a23a6ee262517bf04 Pull Request resolved: #30819

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-poisoned]

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: 4da2bdabd95ea214cf86a335fa521a6bc50a78e2 Pull Request resolved: #30819

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-poisoned]

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: a04946e75a80be6105232bbab5a90babf38a4bd7 Pull Request resolved: #30819

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-poisoned]

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: c17bf59680fa002903c03ac2fe5a7adfddaaec5c Pull Request resolved: #30819

josephsavona · 2024-08-27T19:57:04Z

...ct-compiler/src/__tests__/fixtures/compiler/optional-member-expression-as-memo-dep.expect.md

+  if ($[0] !== props?.items.edges?.nodes) {
+    t1 = props?.items.edges?.nodes.map();
+    $[0] = props?.items.edges?.nodes;


...c/__tests__/fixtures/compiler/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md

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-poisoned]

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: 400055a94f0e041f378f5a37823e8c54dc22205c Pull Request resolved: #30819

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-poisoned]

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: 2a8817daeffff9f0c1115aa303892d2e08d18fd0 Pull Request resolved: #30819

josephsavona · 2024-08-28T00:09:56Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts

+      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


This is bonkers but also temporary, we won’t need this once PropagateScopeDeps is HIR-based. So seems fine for now - worst case it doesn’t match and we fall back to the previous behavior.

josephsavona · 2024-08-28T00:18:50Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts

+    // 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, lvalue);
+    });


same code as before if the pattern doesn't match

Adds a new feature flag, `enableOptionalDependencies` which when enabled allows PropagateScopeDeps and DeriveMinimalDeps to infer optional dependency paths (`a?.b`). In PropagateScopeDeps we look for specific safe patterns of nested optional member expressions: * `<variable> "." / "?." <property>` * or `<nested> "." / "?." <property>` When we find this pattern we record a dependency on the overall chain, so for example in `a?.b.c?.d.map()` we would record a dependency on `a?.b.c?.d` (because the outer `.map()` portion doesn't match the above structure). If the structure doesn't match - for example with `a?.b?.[foo(bar)]?.z` - then we fall back to the existing behavior which treats everything after the initial portion as conditional (for that last example we'd continue to record `a` as the dep). The other portion is DeriveMinimalDeps, which now represents optional access/dependency states and updates the merge logic to handle them. The order of precedence is unconditional > optional > conditional. Note that this isn't complete: if i enable the feature by default some fixtures produce suboptiomal results that I need to debug. But this is pretty close and should be landable as a starting point. [ghstack-poisoned]

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: ae9e125a785c40f0ce6f78d008cda55f11a16ad7 Pull Request resolved: #30819

Adds a new feature flag, `enableOptionalDependencies` which when enabled allows PropagateScopeDeps and DeriveMinimalDeps to infer optional dependency paths (`a?.b`). In PropagateScopeDeps we look for specific safe patterns of nested optional member expressions: * `<variable> "." / "?." <property>` * or `<nested> "." / "?." <property>` When we find this pattern we record a dependency on the overall chain, so for example in `a?.b.c?.d.map()` we would record a dependency on `a?.b.c?.d` (because the outer `.map()` portion doesn't match the above structure). If the structure doesn't match - for example with `a?.b?.[foo(bar)]?.z` - then we fall back to the existing behavior which treats everything after the initial portion as conditional (for that last example we'd continue to record `a` as the dep). The other portion is DeriveMinimalDeps, which now represents optional access/dependency states and updates the merge logic to handle them. The order of precedence is unconditional > optional > conditional. Note that this isn't complete: if i enable the feature by default some fixtures produce suboptiomal results that I need to debug. But this is pretty close and should be landable as a starting point. [ghstack-poisoned]

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: 733d2bf36606b4eaa7b5e408ef923ebbf31e458c Pull Request resolved: #30819

Adds a new feature flag, `enableOptionalDependencies` which when enabled allows PropagateScopeDeps and DeriveMinimalDeps to infer optional dependency paths (`a?.b`). In PropagateScopeDeps we look for specific safe patterns of nested optional member expressions: * `<variable> "." / "?." <property>` * or `<nested> "." / "?." <property>` When we find this pattern we record a dependency on the overall chain, so for example in `a?.b.c?.d.map()` we would record a dependency on `a?.b.c?.d` (because the outer `.map()` portion doesn't match the above structure). If the structure doesn't match - for example with `a?.b?.[foo(bar)]?.z` - then we fall back to the existing behavior which treats everything after the initial portion as conditional (for that last example we'd continue to record `a` as the dep). The other portion is DeriveMinimalDeps, which now represents optional access/dependency states and updates the merge logic to handle them. The order of precedence is unconditional > optional > conditional. Note: the flag is off by default, but i tried enabling it for all fixtures and the results were all either unchanged or had correct optional deps inferred. I'll also try running it internally with the flag enabled, but i think we can proceed with review and fix-forward any issues identified from that testing. [ghstack-poisoned]

mofeiZ

Makes sense, looking forward to understanding / cleaning up the sequence expression logic

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: #30819

[wip] PropagateScopeDeps understands optionals

cee83fe

[ghstack-poisoned]

josephsavona mentioned this pull request Aug 26, 2024

[compiler] Stop reusing ScopeDep type in AnalyzeFunctions #30811

Merged

This was referenced Aug 26, 2024

[compiler] Wrap ReactiveScopeDep path tokens in object #30812

Merged

[compiler] Add DependencyPath optional property #30813

Merged

[compiler] Add fallthrough to branch terminal #30814

Merged

[compiler] Infer optional manual memo deps #30815

Open

facebook-github-bot added the CLA Signed label Aug 26, 2024

josephsavona mentioned this pull request Aug 26, 2024

[compiler] Allow inferred non-optional paths when manual deps were optional #30816

Merged

josephsavona added a commit that referenced this pull request Aug 26, 2024

[wip] PropagateScopeDeps understands optionals

e0a4e1e

ghstack-source-id: 50d3523dbdb483cbd994a19a23a6ee262517bf04 Pull Request resolved: #30819

facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Aug 26, 2024

vercel bot deployed to Preview August 26, 2024 23:24 View deployment

josephsavona changed the title ~~[wip] PropagateScopeDeps understands optionals~~ [wip][compiler] Infer optional dependencies Aug 27, 2024

vercel bot deployed to Preview August 27, 2024 05:46 View deployment

vercel bot deployed to Preview August 27, 2024 05:53 View deployment

josephsavona commented Aug 27, 2024

View reviewed changes

...c/__tests__/fixtures/compiler/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md Outdated Show resolved Hide resolved

vercel bot deployed to Preview August 27, 2024 20:00 View deployment

vercel bot deployed to Preview August 27, 2024 22:29 View deployment

vercel bot deployed to Preview August 27, 2024 22:44 View deployment

josephsavona commented Aug 28, 2024

View reviewed changes

vercel bot deployed to Preview August 28, 2024 01:03 View deployment

vercel bot deployed to Preview August 28, 2024 01:09 View deployment

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Handle optional where innermost property access is non-optional #30836

Merged

josephsavona added 2 commits August 28, 2024 10:44

vercel bot deployed to Preview August 28, 2024 18:09 View deployment

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Enable optional dependencies by default #30838

Merged

mofeiZ approved these changes Aug 28, 2024

View reviewed changes

josephsavona mentioned this pull request Aug 28, 2024

[compiler] Inferred deps must match exact optionality of manual deps #30840

Merged

josephsavona merged commit 597c7e7 into gh/josephsavona/53/base Aug 28, 2024
19 checks passed

josephsavona deleted the gh/josephsavona/53/head branch August 28, 2024 22:59

This was referenced Sep 12, 2024

Upgrade React from 7771d3a7-20240827 to 47352209-20240912 vercel/next.js#70035

Closed

Upgrade React from 7771d3a7-20240827 to 94e652d5-20240912 vercel/next.js#70037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Infer optional dependencies (behind feature flag) #30819

[compiler] Infer optional dependencies (behind feature flag) #30819

josephsavona commented Aug 26, 2024 •

edited

Loading

vercel bot commented Aug 26, 2024 •

edited

Loading

josephsavona Aug 27, 2024

josephsavona Aug 28, 2024

josephsavona Aug 28, 2024

josephsavona Aug 28, 2024

mofeiZ left a comment •

edited

Loading

[compiler] Infer optional dependencies (behind feature flag) #30819

[compiler] Infer optional dependencies (behind feature flag) #30819

Conversation

josephsavona commented Aug 26, 2024 • edited Loading

vercel bot commented Aug 26, 2024 • edited Loading

josephsavona Aug 27, 2024

Choose a reason for hiding this comment

josephsavona Aug 28, 2024

Choose a reason for hiding this comment

josephsavona Aug 28, 2024

Choose a reason for hiding this comment

josephsavona Aug 28, 2024

Choose a reason for hiding this comment

mofeiZ left a comment • edited Loading

Choose a reason for hiding this comment

josephsavona commented Aug 26, 2024 •

edited

Loading

vercel bot commented Aug 26, 2024 •

edited

Loading

mofeiZ left a comment •

edited

Loading