[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

josephsavona · 2024-07-19T01:18:23Z

Stack from ghstack (oldest at bottom):

Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this.

This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where markPredecessors() handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids.

…g scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned]

vercel · 2024-07-19T01:18:25Z

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	Jul 23, 2024 1:14am

…g scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 5a3e3dc25961ec39f3ff8788d9c59ad84ab8c6d0 Pull Request resolved: #30399

…constructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned]

…g scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 7fc6ee2492a24a1d6fa99795c4faec06bc81bd8a Pull Request resolved: #30399

…constructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned]

…g scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2127da749ff7d2f9f27144f6b6e484250389b0de Pull Request resolved: #30399

…nstructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned]

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2127da749ff7d2f9f27144f6b6e484250389b0de Pull Request resolved: #30399

mofeiZ · 2024-07-19T17:28:50Z

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

+  /**
+   * Step 5:
+   * Fix scope and identifier ranges to account for renumbered instructions
+   */


Hmm I don't think the range fixing logic covers all cases here. I wrote some validation in #30409, and we see quite a few cases in which identifiers have an attached mutable range that is neither (1) invalid e.g. end === start + 1 nor (2) a scope range.

There's roughly 3-4 classes:

(fixed by [compiler][donotcommit] show differences between identifier + scope ranges #30409) when we merge scopes, we're currently not setting identifier.mutableRange to that of the scope.

There are lvalues for which we end up pruning the scope, but keep their previous mutable range (see method call example).

Handful of other times we end up assigning a mutable range but not a scope: see props aliasing as a context variable and mutable ref examples

I think currently, mutable ranges are referenced after this pass (see ValidateMemoizedEffectDeps and ValidatePreserveExistingManualMemo for examples).

Overall, I admit I'm a bit confused whether we need to ensure that identifier and scope ranges are always equivalent (i.e. pointing to the same object). My understanding is that this is unnecessary and we should instead deep copy when creating scopes in InferReactiveScopeVariables. Currently, our aligning and merging logic extends the mutableRange of some but not all identifiers

(cc @mvitousek as we were discussing a related topic yesterday)

Yeah I was realizing after writing this that there were some other cases to cover. I think we need to just update all the passes that look at identifier or scope ranges (after this pass) to stop doing that and look at the scope’s effective range.

Alternatively I can do a more thorough tracking of how every scope needs to update its range before/after.

update all the passes that look at identifier or scope ranges (after this pass) to stop doing that

Is this possible to do even before this pass? I.e., is it semantically necessary that identifiers report their own mutable ranges at all? It feels like it would be more consistent to always refer to an identifier's scope's mutable range. As it stands I am uncertain what it even means when they don't align to be honest. (Happy to split this into a separate conversation!)

Well, it's necessary for identifiers to report their ranges so that we can construct scopes. But yeah, after that i don't see why we'd ever need to use identifier ranges.

Addressed in #30428 - happy to squash that into this PR though

…nstructing scope terminals" Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. [ghstack-poisoned]

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: #30399

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. [ghstack-poisoned]

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: 9ad6f5dc1773ab0ef2b4cfd01be106c6d929be51 Pull Request resolved: #30428

… constructing scopes" Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. [ghstack-poisoned]

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: 9d92032389d3cbca779823775baad441f06a7226 Pull Request resolved: #30428

…ble ranges after constructing scopes" Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. We already have fixtures that test for these edge cases: ## preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts This case passes the validation because the way Forget constructs the scopes matches what the user had: there is an unmemoized dependency (x, whose scope is pruned bc it has a hook call) whose range ends before the useMemo, and the useMemo is preserved: ```js const x = []; useHook(); x.push(props); return useMemo(() => [x], [x]); ``` Concretely: there is a `StartMemoize deps=x0`, but scope `0` is pruned so it's allowed. ## preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.ts This case fails the validation because Forget constructs the scopes differently than the user code: the construction of `val` is part of the scope for `identity(val)` since Forget infers that `val` may be mutated there. So rather than `val` appearing as an unmemoized dependency of the memo block, it becomes part of the memo block. So we correctly reject it. ```js const val = [1, 2, 3]; return useMemo(() => { return identity(val); }, [val]); ``` Concretely, there is a `StartMemoize deps=val0`, but the StartMemoize instruction is within scope `0`, which hasn't completed yet, so it can't be a valid dependency and we reject it. # Sync Results I synced this stack: * Minimal changes to compilation output: P1491221074. The pattern is consistent: we previously had false positives where we reported that values that came from pruned scopes (due to hooks) were "unmemoized", whereas the new logic accounts for this with the pruned scope check. * No changes to lint output [ghstack-poisoned]

… constructing scopes" Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. We already have fixtures that test for these edge cases: ## preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts This case passes the validation because the way Forget constructs the scopes matches what the user had: there is an unmemoized dependency (x, whose scope is pruned bc it has a hook call) whose range ends before the useMemo, and the useMemo is preserved: ```js const x = []; useHook(); x.push(props); return useMemo(() => [x], [x]); ``` Concretely: there is a `StartMemoize deps=x0`, but scope `0` is pruned so it's allowed. ## preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.ts This case fails the validation because Forget constructs the scopes differently than the user code: the construction of `val` is part of the scope for `identity(val)` since Forget infers that `val` may be mutated there. So rather than `val` appearing as an unmemoized dependency of the memo block, it becomes part of the memo block. So we correctly reject it. ```js const val = [1, 2, 3]; return useMemo(() => { return identity(val); }, [val]); ``` Concretely, there is a `StartMemoize deps=val0`, but the StartMemoize instruction is within scope `0`, which hasn't completed yet, so it can't be a valid dependency and we reject it. # Sync Results I synced this stack: * Minimal changes to compilation output: P1491221074. The pattern is consistent: we previously had false positives where we reported that values that came from pruned scopes (due to hooks) were "unmemoized", whereas the new logic accounts for this with the pruned scope check. * No changes to lint output [ghstack-poisoned]

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: d7126aff6b733201d0f368180655cfea40017fa7 Pull Request resolved: #30428

…ble ranges after constructing scopes" Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. We already have fixtures that test for these edge cases: ## preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts This case passes the validation because the way Forget constructs the scopes matches what the user had: there is an unmemoized dependency (x, whose scope is pruned bc it has a hook call) whose range ends before the useMemo, and the useMemo is preserved: ```js const x = []; useHook(); x.push(props); return useMemo(() => [x], [x]); ``` Concretely: there is a `StartMemoize deps=x0`, but scope `0` is pruned so it's allowed. ## preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.ts This case fails the validation because Forget constructs the scopes differently than the user code: the construction of `val` is part of the scope for `identity(val)` since Forget infers that `val` may be mutated there. So rather than `val` appearing as an unmemoized dependency of the memo block, it becomes part of the memo block. So we correctly reject it. ```js const val = [1, 2, 3]; return useMemo(() => { return identity(val); }, [val]); ``` Concretely, there is a `StartMemoize deps=val0`, but the StartMemoize instruction is within scope `0`, which hasn't completed yet, so it can't be a valid dependency and we reject it. # Sync Results I synced this stack: * Minimal changes to compilation output: P1491221074. The pattern is consistent: we previously had false positives where we reported that values that came from pruned scopes (due to hooks) were "unmemoized", whereas the new logic accounts for this with the pruned scope check. See the fixture added in the previous PR in the stack, and how it becomes compiled here. * No changes to lint output [ghstack-poisoned]

… constructing scopes" Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. We already have fixtures that test for these edge cases: ## preserve-memo-validation/useMemo-dropped-infer-always-invalidating.ts This case passes the validation because the way Forget constructs the scopes matches what the user had: there is an unmemoized dependency (x, whose scope is pruned bc it has a hook call) whose range ends before the useMemo, and the useMemo is preserved: ```js const x = []; useHook(); x.push(props); return useMemo(() => [x], [x]); ``` Concretely: there is a `StartMemoize deps=x0`, but scope `0` is pruned so it's allowed. ## preserve-memo-validation/error.false-positive-useMemo-infer-mutate-deps.ts This case fails the validation because Forget constructs the scopes differently than the user code: the construction of `val` is part of the scope for `identity(val)` since Forget infers that `val` may be mutated there. So rather than `val` appearing as an unmemoized dependency of the memo block, it becomes part of the memo block. So we correctly reject it. ```js const val = [1, 2, 3]; return useMemo(() => { return identity(val); }, [val]); ``` Concretely, there is a `StartMemoize deps=val0`, but the StartMemoize instruction is within scope `0`, which hasn't completed yet, so it can't be a valid dependency and we reject it. # Sync Results I synced this stack: * Minimal changes to compilation output: P1491221074. The pattern is consistent: we previously had false positives where we reported that values that came from pruned scopes (due to hooks) were "unmemoized", whereas the new logic accounts for this with the pruned scope check. See the fixture added in the previous PR in the stack, and how it becomes compiled here. * No changes to lint output [ghstack-poisoned]

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: #30428

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: #30399

…ng scopes Addresses discussion at #30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: #30428

…ng scopes Addresses discussion at facebook#30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: facebook#30428

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: facebook#30399

…ng scopes Addresses discussion at facebook#30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: facebook#30428

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: facebook#30399

…ng scopes Addresses discussion at facebook#30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: facebook#30428

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: facebook#30399

…ng scopes Addresses discussion at facebook#30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: facebook#30428

…scope terminals Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this. This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids. ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625 Pull Request resolved: facebook#30399

…ng scopes Addresses discussion at facebook#30399 (comment). Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: facebook#30428

This was referenced Jul 19, 2024

[compiler] printTerminal always prints instruction id #30397

Merged

[compiler] Flatten scopes based on fallthrough, not scope range #30398

Merged

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 19, 2024

josephsavona changed the title ~~[compiler] Maintain RPO and uniquely instruction ids when constructing scope terminals~~ [compiler] Maintain RPO and unique instruction ids when constructing scope terminals Jul 19, 2024

vercel bot deployed to Preview July 19, 2024 01:31 View deployment

josephsavona requested a review from mofeiZ July 19, 2024 01:32

mofeiZ reviewed Jul 19, 2024

View reviewed changes

vercel bot deployed to Preview July 23, 2024 01:14 View deployment

josephsavona mentioned this pull request Jul 23, 2024

[compiler] Stop relying on identifier mutable ranges after constructing scopes #30428

Merged

josephsavona mentioned this pull request Jul 23, 2024

[compiler] repro of false positive for ValidatePreserveManualMemo #30431

Merged

josephsavona merged commit 9983ccf into gh/josephsavona/34/base Jul 24, 2024
21 checks passed

josephsavona deleted the gh/josephsavona/34/head branch July 24, 2024 00:23

This was referenced Jul 25, 2024

Update React from 6230622a1a-20240610 to 76002254-20240724 vercel/next.js#68149

Merged

Update React from 6230622a1a-20240610 to 3208e73e-20240730 vercel/next.js#68330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

josephsavona commented Jul 19, 2024 •

edited

Loading

vercel bot commented Jul 19, 2024 •

edited

Loading

mofeiZ Jul 19, 2024

mofeiZ Jul 19, 2024 •

edited

Loading

josephsavona Jul 19, 2024

mvitousek Jul 22, 2024

josephsavona Jul 23, 2024

josephsavona Jul 23, 2024

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

[compiler] Maintain RPO and unique instruction ids when constructing scope terminals #30399

Conversation

josephsavona commented Jul 19, 2024 • edited Loading

vercel bot commented Jul 19, 2024 • edited Loading

mofeiZ Jul 19, 2024

Choose a reason for hiding this comment

mofeiZ Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

josephsavona Jul 19, 2024

Choose a reason for hiding this comment

mvitousek Jul 22, 2024

Choose a reason for hiding this comment

josephsavona Jul 23, 2024

Choose a reason for hiding this comment

josephsavona Jul 23, 2024

Choose a reason for hiding this comment

josephsavona commented Jul 19, 2024 •

edited

Loading

vercel bot commented Jul 19, 2024 •

edited

Loading

mofeiZ Jul 19, 2024 •

edited

Loading