Comparing d878489431...c088ad09f1 - react - گیت مرکز وب ایران

bahman/react

Author	SHA1	Message	Date
Mofei Zhang	c088ad09f1	Update base for Update on "[compiler][rewrite] PropagateScopeDeps hir rewrite" \### Quick background: \#### Rvalues / temporaries: In the compiler, unnamed temporaries that represents the evaluation of an expression In the code snippet below, $1, $2, $3, and $4 are temporaries. ```js // input function Component({ bar} ) { const x = {a: foo(bar), b: {}}; } // gets lowered to [1] $2 = LoadGlobal(global) foo [2] $3 = LoadLocal bar$1 [3] $4 = Call $2(<unknown> $3) [4] $5 = Object { } [5] $6 = Object { a: $4, b: $5 } [6] $8 = StoreLocal Const x$7 = $6 ``` The compiler currently treats temporaries and named variables (e.g. `x`) differently in this pass. - named variables may be reassigned (in fact, since we're running after LeaveSSA, a single named identifier's IdentifierId may map to multiple `Identifier` instances -- each with its own scope and mutable range) - temporaries are replaced with their represented expressions during codegen. This is correct (mostly correct, see #29878) as we're careful to always lower the correct evaluation semantics. However, since we rewrite reactive scopes entirely (to if/else blocks), we need to track temporaries that a scope produces in `ReactiveScope.declarations` and later promote them to named variables. In the same example, $4, $5, and $6 need to be promoted: $2 ->`t0`, $5 ->`t1`, and $6 ->`t2`. ```js [1] $2 = LoadGlobal(global) foo [2] $3 = LoadLocal bar$1 scope 0: [3] $4 = Call $2(<unknown> $3) scope 1: [4] $5 = Object { } scope 2: [5] $6 = Object { a: $4, b: $5 } [6] $8 = StoreLocal Const x$7 = $6 ``` \#### Dependencies `ReactiveScope.dependencies` records the set of (read-only) values that a reactive scope is dependent on. This is currently limited to just variables (named variables from source and promoted temporaries) and property-loads. All dependencies we record need to be hoistable -- i.e. reordered to just before the ReactiveScope begins. Not all PropertyLoads are hoistable. In this example, we should not evaluate `obj.a.b` without before creating x and checking `objIsNull`. ```js // reduce-reactive-deps/no-uncond.js function useFoo({ obj, objIsNull }) { const x = []; if (isFalse(objIsNull)) { x.push(obj.a.b); } return x; } ``` While other memoization strategies with different constraints exist, the current compiler requires that `ReactiveScope.dependencies` be re-orderable to the beginning of the reactive scope. But.. `PropertyLoad`s from null values will throw `TypeError`. This means that evaluating hoisted dependencies should throw if and only if the source program throws. (It is also a bug if source throws and compiler output does not throw. See https://github.com/facebook/react-forget/pull/2709) --- \### Rough high level overview 1. Pass 1 Walk over instructions to gather every temporary used outside of its defining scope (same as ReactiveFunction version). These determine the sidemaps we produce, as temporaries used outside of their declaring scopes get promoted to named variables later (and are not considered hoistable rvals). 2. Pass 2 (collectHoistablePropertyLoads) Walk over instructions to generate sidemaps: a. temporary identifier -> named variable and property path (e.g. `$3 -> {obj: props, path: ["a", "b"]}`) b. block -> accessed variables and properties (e.g. `bb0 -> [ {obj: props, path: ["a", "b"]} ]`) c. Walk over control flow graph to understand the set of object and property paths that can be read by each basic block. This analysis: - relies on post-dominator trees - traverses the CFG from entry (producing the set of variables/paths unconditionally evaluated before a block). - traverses the CFG from exit (producing the set of variables/paths unconditionally evaluated after a block). 4. Pass 3: (collectDependencies) Walks over instructions again to record dependencies and declarations, using the previously produced sidemaps Will add more fixture tests (although most cases should be covered in `reduce-reactive-deps`). Tested by syncing internally and checking compilation output differences ([internal link](https://fburl.com/wiki_markdown/nazsiszd)) --- \### Followups: 1. Rewrite function expression deps This change produces much more optimal output as the compiler now uses the function CFG to understand which variables / paths are assumed to be non-null. However, it may exacerbate [this function-expr hoisting bug](https://github.com/facebook/react/blob/main/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.ts). A short term fix here is to simply call some form of `collectNonNullObjects` on every function expression to find hoistable variable / paths. In the longer term, we should refactor out `FunctionExpression.deps`. 2. Enable optional paths (a) don't count optional load temporaries as dependencies (e.g. `collectOptionalLoadRValues(...)`). (b) add optional paths back. This is a bit tricky as we'll want to implement some merging logic for `ConditionalAccess \| OptionalChain \| UnconditionalAccess`. In addition, our current optional chain lowering is slightly incorrect / imprecise [ghstack-poisoned]	2024-07-10 19:22:02 -04:00
Mofei Zhang	2c9fdc4838	[compiler][ez] PrintHIR prints optional flag for debugging [ghstack-poisoned]	2024-06-24 19:19:46 -04:00
Mofei Zhang	79198b0cd7	[compiler][patch] Patch O(n^2) traversal in validatePreserveMemo [ghstack-poisoned]	2024-06-24 19:19:44 -04:00
Mofei Zhang	f8026d06f5	[compiler][hir] Correctly remove non-existent terminal preds when pruning labels [ghstack-poisoned]	2024-06-24 19:19:42 -04:00
Mofei Zhang	f399ed896b	[compiler][ez] Add more Array.prototype methods [ghstack-poisoned]	2024-06-24 19:19:40 -04:00
Mofei Zhang	1c55d8810f	[compiler][ez] Patch Array.concat object shape to capture callee [ghstack-poisoned]	2024-06-24 19:19:38 -04:00
Mofei Zhang	a2f6bbe6a4	Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-24 19:19:37 -04:00
Mofei Zhang	f8855cfd1e	Update base for Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-24 19:19:37 -04:00
Mofei Zhang	b718a77516	Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-13 20:33:40 -04:00
Mofei Zhang	3fa1cceed8	Update base for Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-13 20:33:39 -04:00
Mofei Zhang	9194615949	Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-13 20:32:38 -04:00
Mofei Zhang	d720486b12	Update base for Update on "[compiler][rewrite] Patch logic for aligning scopes to non-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) \|- bb1 \|- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range. ``` ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]	2024-06-13 20:32:38 -04:00
Mofei Zhang	1ba72191a5	Update on "[compiler][draft] Patch logic for aligning scopes to non-value blocks" [ghstack-poisoned]	2024-06-13 20:31:02 -04:00
Mofei Zhang	2d6a434df5	[compiler][draft] Patch logic for aligning scopes to non-value blocks [ghstack-poisoned]	2024-06-13 16:10:23 -04:00
Mofei Zhang	bec373555a	Update on "[compiler][fixtures] test repros: codegen, alignScope, phis" The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`. I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though! The other two are likely bigger issues, as they're not caught by static invariants. Update: - removed bug-phi-reference-effect as it's been patched by josephsavona - added bug-array-concat-should-capture [ghstack-poisoned]	2024-06-13 12:47:42 -04:00
Mofei Zhang	0bceafbe97	Update base for Update on "[compiler][fixtures] test repros: codegen, alignScope, phis" The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`. I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though! The other two are likely bigger issues, as they're not caught by static invariants. Update: - removed bug-phi-reference-effect as it's been patched by josephsavona - added bug-array-concat-should-capture [ghstack-poisoned]	2024-06-13 12:47:42 -04:00
Mofei Zhang	2827cbc594	[compiler][fixtures] Bug repros: codegen, alignScope, phis [ghstack-poisoned]	2024-06-12 15:51:01 -04:00

Diff Content Not Available