[compiler][newinference] Fix for phi types, extracting primitives from objects #33440
+310
−32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…m objects First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
This was referenced Jun 5, 2025
This was referenced Jun 5, 2025
github-actions
bot
added
the
React Core Team
josephsavona
added a commit
that referenced
this pull request
Jun 5, 2025
…m objects First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). ghstack-source-id: 7d6374c Pull Request resolved: #33440
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
This was referenced Jun 6, 2025
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
…mitives from objects" First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types. The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain. But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together. There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable: * Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase. * LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together. * InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there). [ghstack-poisoned]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types.
The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider
alias a -> b, mutate(b). We have to walk back the alias chain from b to a, and mark a as mutated too. But foralias a -> b, mutate(a), we also have to mark b as mutated — walking forwards along the alias chain.But phis are a bit different. You can have a mutation of one of the phi operands, such that you have
a, b -> phi c, mutate(a). Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together.There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable:
applyEffect()creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase.