[compiler] Hoist dependencies from functions more conservatively

[mofeiZ]

Alternative to #31584 which sets enableTreatFunctionDepsAsConditional:true` by default.

This PR changes dependency hoisting to be more conservative while trying to preserve an optimal "happy path". We assume that a function "is likely called" if we observe the following in the react function body.

• a direct callsite
• passed directly as a jsx attribute or child
• passed directly to a hook
• a direct return

A function is also "likely called" if it is directly called, passed to jsx / hooks, or returned from another function that "is likely called".

Note that this approach marks the function definition site with its hoistable properties (not its use site). I tried implementing use-site hoisting semantics, but it felt both unpredictable (i.e. as a developer, I can't trust that callbacks are well memoized) and not helpful (type + null checks of a value are usually colocated with their use site)

In this fixture (copied here for easy reference), it should be safe to use a.value and b.value as dependencies, even though these functions are conditionally called.

// inner-function/nullable-objects/assume-invoked/conditional-call-chain.tsx
function Component({a, b}) {
  const logA = () => {
    console.log(a.value);
  };
  const logB = () => {
    console.log(b.value);
  };
  const hasLogged = useRef(false);
  const log = () => {
    if (!hasLogged.current) {
      logA();
      logB();
      hasLogged.current = true;
    }
  };
  return <Stringify log={log} shouldInvokeFns={true} />;
}

On the other hand, this means that we produce invalid output for code like manually implementing Array.map

// inner-function/nullable-objects/bug-invalid-array-map-manual.js
function useFoo({arr1, arr2}) {
  const cb = e => arr2[0].value + e.value;
  const y = [];
  for (let i = 0; i < arr1.length; i++) {
    y.push(cb(arr1[i]));
  }
  return y;
}

[josephsavona]

Super exciting! Overall the logic makes a lot of sense. I have a few small nits, also i noticed a couple of the outputs aren't what i would have expected given what we discussed walking through this (eg why is it arr1[0] when arr1 could be nullable, i'd expect the dep to be arr1 in these places.

[josephsavona]

we remove ObjectMethod here because any function captured in an object is assumed to be maybe not called, makes sense

[josephsavona]

we could teach the compiler about Function.prototype.call/apply but this is likely super rare, this makes sense

[jbrown215]

Does JSX allow you to pass in null for props? I don't know what the syntax for that would be. I think you'd need to invoke createElement/jsx directly to get a nullable props argument.

Do we actually need to be more conservative on props?

[josephsavona]

good catch, i thought we already tracked non-null objects. maybe the changes here are overriding that?

[mofeiZ]

I think this is because we're typing this function as Other, not Component (playground shows that infer mode doesn't pick up on this being a component)

[josephsavona]

when we do the inference of assumed-called functions, let's compute dominators and only look at calls that are in unconditionally executed blocks

[mofeiZ]

I previously implemented some version of mapFnExpressionsToEscapingBlocks, which computes a Map<LoweredFunction, Set<BlockId>>. This approach felt incomplete because it didn't optimize the common conditional-call-chain case but did reduce the number of internal deopts significantly, so I'm happy to put up a PR with that approach.

// conditional-call-chain
function Component({a, b}) {
  const logA = () => {
    console.log(a.value);
  };
  const logB = () => {
    console.log(b.value);
  };
  const hasLogged = useRef(false);
  const log = () => {
    if (!hasLogged.current) {
      logA();
      logB();
      hasLogged.current = true;
    }
  };
  return <Stringify log={log} shouldInvokeFns={true} />;
}

// conditionally-return-fn.ts
function useMakeCallback({
  obj,
  shouldMakeCb,
  setState,
}) {
  const cb = () => setState(obj.value);
  if (shouldMakeCb) return cb;
  else return null;
}

Another option is that we could consider "conditionally used within a lambda" differently from "conditionally used within the main component / hook body". This feels difficult to understand though

[mofeiZ]

Either way, I'm not sure that dominators are the right approach as we might have exhaustive data-flow

function Component() {
  const cb = () => {...};
  if (cond) return <Child1 cb={cb} />;
  else return <Child2 cb={cb} />;
}

[josephsavona]

Gotcha, discussed offline and this makes sense. Thanks for clarifying?

[josephsavona]

see other comment - this should likely use computeUnconditionalBlocks() and only consider calls in such blocks

[josephsavona]

this presumably change auto deps inference too for effects. this one is interesting, cc @jbrown215

[jbrown215]

item should be deeply-immutable because it is returned from a hook. Shouldn't it be safe to take item.value as a dep?

[josephsavona]

it's not guaranteed to be non-null

[jbrown215]

A lot of the test cases I looked at had values that we've typically assumed to be frozen/immutable become less precise in deps. Why do we need to be more conservative in those cases? Shouldn't those only be incorrect in the presence of Rules of React violations?

[jbrown215]

Does JSX allow you to pass in null for props? I don't know what the syntax for that would be. I think you'd need to invoke createElement/jsx directly to get a nullable props argument.

Do we actually need to be more conservative on props?

[jbrown215]

Since arr1 and arr2 are props, shouldn't they be deeply immutable? It should be safe to hoist them out?

If this is ever unsafe it's because of a rules-of-react violation

[mofeiZ]

Yep arr1 and arr2 are both known to be deeply immutable, but that doesn't mean it's safe to hoist property loads from them as they might be null (or have no elements)

[jbrown215]

item should be deeply-immutable because it is returned from a hook. Shouldn't it be safe to take item.value as a dep?

[josephsavona]

Super exciting!