mirror of
https://github.com/facebook/react.git
synced 2025-11-01 09:12:30 +00:00
Mofei Zhang
5166869204
A dependency D from either an instruction or scope is poisoned if there may be a
(non-linear) jump instruction between it and the start of its immediate parent
scope. Poisoned dependencies are added as conditional dependencies to their
parent scope.
(done: reduce false positives in scopes that begin after return/throw) (done:
fix bugs in recording and joining exhaustive conditional deps) (done: flesh out
commit message, clean up PR, add more fixtures)
--- \## Bug details:
Take a simple example: ```js target: { instrA; if (...) { instrB;
break target; } else { instrC; } instrD; // ... } instrE; // ... ```
This diagram shows how we represent this program in the reactive IR. - Blocks
are represented as a list of nodes. - Green nodes show instructions and value
blocks (simplified as a single instruction). - Pink nodes show terminals, which
transfer control to a subtree of nodes. <img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/930789f2-39cd-4ea8-b12a-530042807b46">
Prior to this PR, PropagateReactiveScopeDeps was incorrect because it assumed
that a block's instructions are evaluated unconditionally (which is how HIR
basic blocks work). E.g. if a reactive scope enclosed `block 1`, we assume that
`instrA` and `instrD` both will evaluate unconditionally.
This failed to account for `jump` instructions like break, continue, return, and
throw. This may result in invalid hoisting of PropertyLoads (i.e. Forget output
may throw when source does not throw). Note that other terminals (e.g. if and
loops) are not affected as they are self contained subtrees that evaluate
sequentially.
With the changes in this PR, we mark `block 1` as poisoned upon encountering the
`break` instruction. While `block 1` is active and poisoned, it will determine
how visited dependencies are added.
Here, added solid lines show unconditional dependencies, dashed lines show
conditionally accessed dependencies: - dependencies from `instrB, instrC` are
conditional because they are within conditional subtrees - dependencies from
`instrD` are conditional because it is within a poisoned block within its parent
scope.
<img width="450" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/81980f68-7e65-4bd7-ba94-3f0c26550e5c">
--- Recapping an offline discussion with @josephsavona: this pass would really
benefit from operating on HIR. The minimal work needed for this pass to run on
HIR is to rewrite and reorder `AlignReactiveScopesToBlockScopes` to operate on
HIR.
The following diagram shows what HIR blocks look like for the same code.
Evaluating hoistable PropertyLoad dependencies for a scope enclosing
`instr{A-D}` is much simpler: just evaluate whether the PropertyLoad evaluates
for every path between `bb0` and `bb4`. <img width="250" alt="image"
src="https://github.com/facebook/react-forget/assets/34200447/44b38939-defb-4b29-878d-4445ec6ccc06">
---