[compiler] First cut at dep inference #31386

jbrown215 · 2024-10-30T03:18:43Z

This is for researching/prototyping, not a feature we are releasing imminently.

Putting up an early version of inferring effect dependencies to get feedback on the approach. We do not plan to ship this as-is, and may not start by going after direct useEffect calls. Until we make that decision, the heuristic I use to detect when to insert effect deps will suffice for testing.

The approach is simple: when we see a useEffect call with no dep array we insert the deps inferred for the lambda passed in. If the first argument is not a lambda then we do not do anything.

This diff is the easy part. I think the harder part will be ensuring that we can infer the deps even when we have to bail out of memoization. We have no other features that must run regardless of rules of react violations. Does anyone foresee any issues using the compiler passes to infer reactive deps when there may be violations?

I have a few questions:

Will there ever be more than one instruction in a block containing a useEffect? if no, I can get rid of theaddedInstrs variable that I use to make sure I insert the effect deps array temp creation at the right spot.
Are there any cases for resolving the first argument beyond just looking at the lvalue's identifier id that I'll need to take into account? e.g., do I need to recursively resolve certain bindings?

vercel · 2024-10-30T03:18:48Z

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	Nov 21, 2024 9:39pm

josephsavona · 2024-10-30T04:36:45Z

Will there ever be more than one instruction in a block containing a useEffect

Ahh I think what you might be observing is that we create scopes for hook calls, but then flatten them away. And so either you’re looking at the pre-flattened state or you’re looking at a later pass after we flatten to a labeled block. We want to get rid of the labeled block and truly flatten it as if there had never been a scope, though. So I would definitely code as if there could be other instructions in the block w the effect.

josephsavona · 2024-10-30T06:02:24Z

Are there any cases for resolving the first argument beyond just looking at the lvalue's identifier id

No, that should only happen if there wasn’t an inline function expression.

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

josephsavona

Exciting! A bunch of smaller feedback, we should discuss the strategy (given the need for guaranteed compilation) in our compiler design sync

compiler/packages/babel-plugin-react-compiler/src/Inference/InferEffectDependencies.ts

...ges/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.js

jbrown215 · 2024-11-06T21:25:59Z

@mofeiZ mentioned that it would be good to promote effect lambdas to their own scope before running PropagateScopeDependencies to get the most precise deps I can

josephsavona · 2024-11-06T21:42:28Z

Hmmm given that effect lambdas are immediately frozen and also transitively freeze their dependencies, they should already be in their own scope anyway. Do you have a specific example where that isn't the case now?

jbrown215 · 2024-11-06T22:02:00Z

I don't have a specific example, but we were talking about this in the context of a reduced pipeline that may not be running all of the phases. For example, we may not freeze functions in the re-run because it relies on certain Rules of React not to be violated. In that context it seems possible? In any case, it doesn't need to block this PR and I can wait until I produce that issue!

josephsavona · 2024-11-06T22:24:34Z

Ahhh i see. Yeah, in the reduced pipeline you could just force-creates scopes around the lambdas passed to useEffect and run propagateScopeDeps to get the deps.

poteto · 2024-11-07T20:22:59Z

...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md

+    console.log(foo);
+    console.log(bar);
+    console.log(moduleNonReactive);
+    console.log(localNonreactive);
+    console.log(globalValue);


could you add a ref here and a non-primitive local too?

jbrown215 · 2024-11-19T17:00:06Z

...el-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies.expect.md

+  } else {
+    t4 = $[9];
+  }
+  useEffect(t4, [bar, bar.qux]);


This looks a little weird to me-- the reactive deps of the function on 93 are more precise than the deps we insert here, but we filter the effect deps to just its reactive deps...

@mofeiZ: I think you were working on a related problem here about capturing member expressions- does anything look strange to you here? Should I be filtering the deps differently?

Chatted about this offline, the correct fix here was to change effect dependencies to be derived from ReactiveScope.dependencies instead of LoweredFunction.dependencies (also see #31386 (comment))

mofeiZ · 2024-11-21T21:49:08Z

...el-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-memberexpr-merge.expect.md

+  } else {
+    t2 = $[3];
+  }
+  useEffect(t2, [obj.a]);


Note that obj.a (the merged dependency) is taken here as we're now using memo scope dependencies, which are merged in PropagateScopeDeps. Another benefit is that this is more "correct" as LoweredFunction.dependencies do not take into account hoistability (e.g. nullability) of accessed values.

function Component({value, cond}) { // here, we should take [cond, value] as a dependency, not // [cond, value.a.b] as evaluating value.a.b may throw useEffect(() => { if (cond) { read(value.a.b); } });

mofeiZ · 2024-11-21T21:51:00Z

...babel-plugin-react-compiler/src/__tests__/fixtures/compiler/pruned-nonreactive-obj.expect.md

+import { useIdentity, mutate, makeObject } from "shared-runtime";
+import { useEffect } from "react";
+
+/**


This is an interesting case. @jbrown215 and I discussed offline and came up with an example for why these should be dependencies (see code snippet in below comment block)

mofeiZ · 2024-11-21T21:52:39Z

...ckages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-dep.expect.md

+  } else {
+    t1 = $[1];
+  }
+  useEffect(t1, [obj]);


See comment at top of fixture for why [obj] is included in the deps array even though it's a stable (and memoized) value

mofeiZ · 2024-11-21T21:56:28Z

Hmmm given that effect lambdas are immediately frozen and also transitively freeze their dependencies, they should already be in their own scope anyway. Do you have a specific example where that isn't the case now?

In this case, should we bail out on @enableAssumeHooksFollowRulesOfReact:false for this feature?

mofeiZ

Super exciting!

jbrown215 · 2024-11-22T17:16:27Z

Thanks for all the help merging this @mofeiZ !

yongsk0066 · 2024-11-23T04:29:30Z

I noticed this PR is about dependency inference at the compiler level, but this wouldn't be reflected in the actual source code. This seems a bit ambiguous - is this groundwork for the React IDE?

facebook-github-bot added the CLA Signed label Oct 30, 2024

vercel bot deployed to Preview October 30, 2024 03:21 View deployment

jbrown215 changed the title ~~[compiler] First cut at inferring effect dependencies~~ [compiler] First cut at dep inference Oct 30, 2024

josephsavona reviewed Oct 30, 2024

View reviewed changes

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts Outdated Show resolved Hide resolved

josephsavona reviewed Oct 30, 2024

View reviewed changes

vercel bot deployed to Preview November 1, 2024 19:08 View deployment

vercel bot deployed to Preview November 1, 2024 19:26 View deployment

poteto reviewed Nov 7, 2024

View reviewed changes

jbrown215 added 7 commits November 19, 2024 11:50

Add env config to infer effect deps

469ae32

First cut of inferring effect deps

3bd6f87

Address PR feedback + run prettier

5d7cc78

No need to spread a filtered array

72b6f1b

prettier again lol

0481ddd

update tests

cedc4b6

update tests

d99e393

jbrown215 force-pushed the main branch from e585fc5 to d99e393 Compare November 19, 2024 16:58

jbrown215 commented Nov 19, 2024

View reviewed changes

vercel bot deployed to Preview November 19, 2024 17:00 View deployment

prettier

d77ad3d

vercel bot deployed to Preview November 19, 2024 17:04 View deployment

refactor to use scope deps instead of FunctionExpression.dependencies

9b6d39f

vercel bot deployed to Preview November 21, 2024 21:33 View deployment

lints

bd14dde

vercel bot deployed to Preview November 21, 2024 21:39 View deployment

mofeiZ reviewed Nov 21, 2024

View reviewed changes

mofeiZ approved these changes Nov 22, 2024

View reviewed changes

mofeiZ merged commit e697386 into facebook:main Nov 22, 2024
18 checks passed

devluismeta mentioned this pull request Nov 22, 2024

Bug: Can't built chrome extension - yarn build-for-devtools fails #31618

Open

This was referenced Dec 4, 2024

Upgrade React from b01722d5-20241114 to 1b1283ad-20241203 vercel/next.js#73506

Merged

Upgrade React from b01722d5-20241114 to de68d2f4-20241204 vercel/next.js#73522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] First cut at dep inference #31386

[compiler] First cut at dep inference #31386

jbrown215 commented Oct 30, 2024 •

edited

Loading

vercel bot commented Oct 30, 2024 •

edited

Loading

josephsavona commented Oct 30, 2024

josephsavona commented Oct 30, 2024

josephsavona left a comment

jbrown215 commented Nov 6, 2024

josephsavona commented Nov 6, 2024

jbrown215 commented Nov 6, 2024

josephsavona commented Nov 6, 2024

poteto Nov 7, 2024

jbrown215 Nov 19, 2024 •

edited

Loading

jbrown215 Nov 19, 2024

mofeiZ Nov 21, 2024

mofeiZ Nov 21, 2024

mofeiZ Nov 21, 2024

mofeiZ Nov 21, 2024

mofeiZ commented Nov 21, 2024

mofeiZ left a comment

jbrown215 commented Nov 22, 2024

yongsk0066 commented Nov 23, 2024

[compiler] First cut at dep inference #31386

[compiler] First cut at dep inference #31386

Conversation

jbrown215 commented Oct 30, 2024 • edited Loading

vercel bot commented Oct 30, 2024 • edited Loading

josephsavona commented Oct 30, 2024

josephsavona commented Oct 30, 2024

josephsavona left a comment

Choose a reason for hiding this comment

jbrown215 commented Nov 6, 2024

josephsavona commented Nov 6, 2024

jbrown215 commented Nov 6, 2024

josephsavona commented Nov 6, 2024

poteto Nov 7, 2024

Choose a reason for hiding this comment

jbrown215 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

jbrown215 Nov 19, 2024

Choose a reason for hiding this comment

mofeiZ Nov 21, 2024

Choose a reason for hiding this comment

mofeiZ Nov 21, 2024

Choose a reason for hiding this comment

mofeiZ Nov 21, 2024

Choose a reason for hiding this comment

mofeiZ Nov 21, 2024

Choose a reason for hiding this comment

mofeiZ commented Nov 21, 2024

mofeiZ left a comment

Choose a reason for hiding this comment

jbrown215 commented Nov 22, 2024

yongsk0066 commented Nov 23, 2024

jbrown215 commented Oct 30, 2024 •

edited

Loading

vercel bot commented Oct 30, 2024 •

edited

Loading

jbrown215 Nov 19, 2024 •

edited

Loading