[compiler][hir] Only hoist always-accessed PropertyLoads from function decls

[mofeiZ]

Stack from ghstack (oldest at bottom):

• -> [compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066
• [compiler][hir-rewrite] Check mutability of base identifier when hoisting #31032

Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their entry block.

Note that this only replaces the hoisting part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see todo-infer-function-uncond-optionals-hoisted.tsx)

See newly added test fixtures for details

Update: Note that toggling enableTreatFunctionDepsAsConditional makes a non-trivial impact on granularity of inferred deps (i.e. we find that function declarations uniquely identify some paths as hoistable). Snapshot comparison of internal code shows ~2.5% of files get worse dependencies (internal link)

[vercel]

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	Oct 3, 2024 6:33pm

[mofeiZ]

Note that we could technically reuse knownImmutableIdentifiers for non-mutable captured identifiers. This is a bit more explicit

[mofeiZ]

This is just code movement. All mutability check logic moved to isImmutableAtInstr because it got more complex ("if we're within a nested function, check if the base identifier is in nestedFnImmutableContext, otherwise do the scope / mutable range check")

[mofeiZ]

This is the core logic for collecting hoistable paths in inner functions

[mofeiZ]

Note that the propagate-hir-fork version of this fixture (correctly) only adds a as a dependency

[mofeiZ]

conditional dependency within a function is truncated down to non-null load (e.g. local)

[mofeiZ]

conditional dependency within a function is truncated down to non-null load (e.g. a)

[mofeiZ]

Unconditional dependency within a nested function is retained (a.b.c)

[mofeiZ]

This is a todo because we're still adding dependencies using LoweredFunction.dependencies.

[josephsavona]

Hmm, mostly makes sense but i'm not sure about saying that because a function accesses eg a.b.c, that we can promote this to a non-nullable / hoistable expression. There's no guarantee the function will ever be called. OTOH if we knew the function was called, then it would be safe to make this assumption.

[mofeiZ]

Hmm, mostly makes sense but i'm not sure about saying that because a function accesses eg a.b.c, that we can promote this to a non-nullable / hoistable expression. There's no guarantee the function will ever be called. OTOH if we knew the function was called, then it would be safe to make this assumption.

Yeah that's fair. One thing we could do is take the hoistable deps / paths of a function to be Intersect(hoistables in function definition site, ...hoistables in every known callsite). For functions that don't end up being called though (i.e. only passed around or captured by other non-called functions), this would mean that we don't infer anything to be hoistable.

My assumptions were that a function should be type-safe to call for all post-dominators of the function definition site. I think this lines up with flow and typescript's modeling as well.

// e.g. I believe this case should be rare because it produces a type error
const arr = getArrayOrNull();
const fn = () => arr.length;
if (arr != null) {
  return fn();
}
// ...

[josephsavona]

One thing we could do is take the hoistable deps / paths of a function to be Intersect(hoistables in function definition site, ...hoistables in every known callsite). For functions that don't end up being called though (i.e. only passed around or captured by other non-called functions), this would mean that we don't infer anything to be hoistable.

I'm not sure about the conclusion here for functions that aren't known to be called. Generally functions are accessing variables in local scope that are also referenced in other places during render, which would provde that those values are hoistable. So intersect(hoistable-in-function, hoistable-globally) seems like it would still let us get pretty good function deps and be fully safe. Can we try this?

[josephsavona]

Chatted on VC, I see what you're getting at. Basically, type systems check functions based on where they're defined, not where they're potentially called, so developers generally have to add null-checks in functions to satisfy the type-checker. If they don't have a null-check, it's a good indication the value is not just non-nullable in practice, but a non-nullable type.

Let's proceed as planned!

[mofeiZ]

Thanks for the quick review! Updated PR summary to include internal snapshot comparison + 2.5% statistic