Mirror of upstream PR #33429 #126

kushxg · 2025-06-04T00:06:48Z

Mirrored from facebook/react PR facebook#33429

…ctions Found when testing the new validation from facebook#33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned]

…nMutableFunctions" Found when testing the new validation from facebook#33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned]

… places The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned]

…n places as outer (context) places" The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned]

…r (context) places" The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value. With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable. [ghstack-poisoned]

@mofeiZ

This is a stab at addressing a pattern that @mofeiZ and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example @mofeiZ added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

…nction expressions" This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

…nction expressions" This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

…nction expressions" This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships. From an example mofeiz added: ```js const x = {}; const y = {}; const f = () => { const a = [y]; const b = x; // this sets y.x = x a[0].x = b; } f(); mutate(y.x); // which means this mutates x! ``` Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too. The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to: * Return the alias sets from InferMutableRanges * Augment them with capturing of the form above, handling cases such as the `a[0].x = b` * For each alias group, record a CaptureEffect for any group that contains 2+ context operands * Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions. This isn't quite right yet, just sharing early hacking. [ghstack-poisoned]

@mofeiZ

We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently @mofeiZ found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't. The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root. [ghstack-poisoned]

…n to InferMutableRanges" We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't. The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root. [ghstack-poisoned]

…eRanges" We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't. The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root. [ghstack-poisoned]

…n to InferMutableRanges" We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't. The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root. [ghstack-poisoned]

…eRanges" We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't. The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root. [ghstack-poisoned]

Adds fixture tests to demonstrate an issue in changing PropertyStore to always have a Store effect on its object operand, regardless of the operand type. The issue is that if we're doing a PropertyStore on a nested value, that has be considered a transitive mutation of the parent object: ``` const x = {y: {z: {}}}; x.y.z.key = 'value'; // this has to be a mutation of `x` ``` Fix in the next PR. [ghstack-poisoned]

Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

…fect" Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

…fect" Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

…fect" Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

This is a fix for a problem where React retains shadow nodes longer than it needs to. The behaviour is shown in React Native test: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169 # Problem When React commits a new shadow tree, old shadow nodes are stored inside `fiber.alternate.stateNode`. This is not cleared up until React clones the node again. This may be problematic if mutation deletes a subtree, in that case `fiber.alternate.stateNode` will retain entire subtree until next update. In case of image nodes, this means retaining entire images. So when React goes from revision A: `<View><View /></View>` to revision B: `<View />`, `fiber.alternate.stateNode` will be pointing to Shadow Node that represents revision A.. ![image](https://github.com/user-attachments/assets/076b677e-d152-4763-8c9d-4f923212b424) # Fix To fix this, this PR adds a new feature flag `enableEagerAlternateStateNodeCleanup`. When enabled, `alternate.stateNode` is proactively pointed towards finishedWork's stateNode, releasing resources sooner. I have verified this fixes the issue [demonstrated by React Native tests](https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169). All existing React tests pass when the flag is enabled.

…fect" Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

Fix for the issue in the previous PR. Long-term the ideal thing would be to make InferMutableRanges smarter about Store effects, and recognize that they are also transitive mutations of whatever was captured into the object. So in the following: ``` const x = {y: {z: {}}}; x.y.z.key = value; ``` That the `PropertyStore z . 'key' = value` is a transitive mutation of x and all three object expressions (x, x.y, x.y.z). But for now it's simpler to stick to the original idea of Store only counting if we know that the type is an object. [ghstack-poisoned]

…prefix [ghstack-poisoned]

[ghstack-poisoned]

…mutate when It turns out that InferMutationAliasingRanges does need a fixpoint loop, but the approach is arguably simpler overall and more precise than the previous implementation. Like InferMutationAliasingEffects (which is the new InferReferenceEffects), we build an abstract model of the heap. But here we know what the effects are, so we can do abstract interpretation of the effects. Each abstract value stores a set of values that it has captured (for transitive mutation), while each variable keeps a set of values it may directly mutate (for assign/alias/capturefrom). This means that at each mutation, we can mark _exactly_ the set of variables/values that are affected by that specific instruction. This means we can correctly infer that `mutate(b)` can't impact `a` here: ``` a = make(); b = make(); mutate(b); // when we interpret the mutation here, a isn't captured yet b.a = a; ``` We will need to make this a fixpoint, but only if there are backedges in the CFG. [ghstack-poisoned]

…cisely models which values mutate when" It turns out that InferMutationAliasingRanges does need a fixpoint loop, but the approach is arguably simpler overall and more precise than the previous implementation. Like InferMutationAliasingEffects (which is the new InferReferenceEffects), we build an abstract model of the heap. But here we know what the effects are, so we can do abstract interpretation of the effects. Each abstract value stores a set of values that it has captured (for transitive mutation), while each variable keeps a set of values it may directly mutate (for assign/alias/capturefrom). This means that at each mutation, we can mark _exactly_ the set of variables/values that are affected by that specific instruction. This means we can correctly infer that `mutate(b)` can't impact `a` here: ``` a = make(); b = make(); mutate(b); // when we interpret the mutation here, a isn't captured yet b.a = a; ``` We will need to make this a fixpoint, but only if there are backedges in the CFG. [ghstack-poisoned]

…ich values mutate when" It turns out that InferMutationAliasingRanges does need a fixpoint loop, but the approach is arguably simpler overall and more precise than the previous implementation. Like InferMutationAliasingEffects (which is the new InferReferenceEffects), we build an abstract model of the heap. But here we know what the effects are, so we can do abstract interpretation of the effects. Each abstract value stores a set of values that it has captured (for transitive mutation), while each variable keeps a set of values it may directly mutate (for assign/alias/capturefrom). This means that at each mutation, we can mark _exactly_ the set of variables/values that are affected by that specific instruction. This means we can correctly infer that `mutate(b)` can't impact `a` here: ``` a = make(); b = make(); mutate(b); // when we interpret the mutation here, a isn't captured yet b.a = a; ``` We will need to make this a fixpoint, but only if there are backedges in the CFG. [ghstack-poisoned]

Further refine the abstract interpretation for InferMutationAliasingRanges to account for forward data flow: Alias/Capture a -> b, mutate(a) => mutate(b) ie mutation affects aliases of the place being mutated, not just things that place aliased. Fixes inference of which context vars of a function mutate, using the precise inference from the Range pass. Adds MutateFrozen and MutateGlobal effects but they're not fully hooked up yet. [ghstack-poisoned]

…ence" Further refine the abstract interpretation for InferMutationAliasingRanges to account for forward data flow: Alias/Capture a -> b, mutate(a) => mutate(b) ie mutation affects aliases of the place being mutated, not just things that place aliased. Fixes inference of which context vars of a function mutate, using the precise inference from the Range pass. Adds MutateFrozen and MutateGlobal effects but they're not fully hooked up yet. [ghstack-poisoned]

Further refine the abstract interpretation for InferMutationAliasingRanges to account for forward data flow: Alias/Capture a -> b, mutate(a) => mutate(b) ie mutation affects aliases of the place being mutated, not just things that place aliased. Fixes inference of which context vars of a function mutate, using the precise inference from the Range pass. Adds MutateFrozen and MutateGlobal effects but they're not fully hooked up yet. [ghstack-poisoned]

…ence" Further refine the abstract interpretation for InferMutationAliasingRanges to account for forward data flow: Alias/Capture a -> b, mutate(a) => mutate(b) ie mutation affects aliases of the place being mutated, not just things that place aliased. Fixes inference of which context vars of a function mutate, using the precise inference from the Range pass. Adds MutateFrozen and MutateGlobal effects but they're not fully hooked up yet. [ghstack-poisoned]

Further refine the abstract interpretation for InferMutationAliasingRanges to account for forward data flow: Alias/Capture a -> b, mutate(a) => mutate(b) ie mutation affects aliases of the place being mutated, not just things that place aliased. Fixes inference of which context vars of a function mutate, using the precise inference from the Range pass. Adds MutateFrozen and MutateGlobal effects but they're not fully hooked up yet. [ghstack-poisoned]

… support [ghstack-poisoned]

…ConditionallyMutateIterator support" [ghstack-poisoned]

…ateIterator support" [ghstack-poisoned]

[ghstack-poisoned]

…erminals" Adds explicit freeze effects for Return terminals and, more importantly, adds effects for MaybeThrow terminals. MaybeThrow is super interesting. The idea of the terminal is to represent that control-flow can break from nearly any instruction to the error handler (catch). InferMutableRanges has a pass that explicitly handles the corresponding data flow, saying that for any instruction in a block ending in MaybeThrow, to alias the instruction's lvalue to the catch handler. This is to handle cases like this: ```js const x = []; try { throwInput(x); } catch (x_alias) { mutate(x_alias); // mutates x! } ``` One realization is that this logic was overly pessimistic: most instruction types cannot actually throw their lvalue. In fact, the only things that can throw their lvalue are call expressions! `c = a.b` can throw, for example, but only with an error generated by the runtime, not with a user value. Doing this allows us to encode the data flow once and then not have to handle wiring up this data again later. [ghstack-poisoned]

Adds explicit freeze effects for Return terminals and, more importantly, adds effects for MaybeThrow terminals. MaybeThrow is super interesting. The idea of the terminal is to represent that control-flow can break from nearly any instruction to the error handler (catch). InferMutableRanges has a pass that explicitly handles the corresponding data flow, saying that for any instruction in a block ending in MaybeThrow, to alias the instruction's lvalue to the catch handler. This is to handle cases like this: ```js const x = []; try { throwInput(x); } catch (x_alias) { mutate(x_alias); // mutates x! } ``` One realization is that this logic was overly pessimistic: most instruction types cannot actually throw their lvalue. In fact, the only things that can throw their lvalue are call expressions! `c = a.b` can throw, for example, but only with an error generated by the runtime, not with a user value. Doing this allows us to encode the data flow once and then not have to handle wiring up this data again later. [ghstack-poisoned]