Skip to content

Conversation

josephsavona
Copy link
Member

@josephsavona josephsavona commented Jun 5, 2025

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 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).

…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
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jun 5, 2025
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]
…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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants