Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context.
And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here.
[ghstack-poisoned]
Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context.
And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here.
[ghstack-poisoned]
Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context.
And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here.
[ghstack-poisoned]
Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context.
And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here.
[ghstack-poisoned]
Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context.
And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here.
[ghstack-poisoned]
The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.
[ghstack-poisoned]
The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.
[ghstack-poisoned]
The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.
[ghstack-poisoned]
The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.
[ghstack-poisoned]
The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.
[ghstack-poisoned]
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]
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]
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]
Fixes for a few cases:
* If you extract part of a value and mutate the part, that has to count as mutating the whole. See the new fixture, but this also came up with an existing test that has an array whose items are modified via array.map with a lambda that mutates its input. The key change here is preserving the CreateFrom effect (not downgrading to Alias) and then handling CreateFrom specially in range inference. Mutations of values derived from CreateFrom are transitive against the value they came from. We handle this by keeping a queue of not just the places to visit during mutation, but whether to visit them transitively or not.
* TODO: we may also want to track whether we've seen a value as transitive or not
* For that array.map case w the mutable lambda, we weren't bubbling up the effects correctly so that we knew the param was mutated. Basically we inferred this information and then didn't record it. There is a bit of plumbing here.
* Similarly, if a function expression returns a function expression and the inner one mutates context, we weren't propagating that up to the outer function expression.
[ghstack-poisoned]
Fixes for a few cases:
* If you extract part of a value and mutate the part, that has to count as mutating the whole. See the new fixture, but this also came up with an existing test that has an array whose items are modified via array.map with a lambda that mutates its input. The key change here is preserving the CreateFrom effect (not downgrading to Alias) and then handling CreateFrom specially in range inference. Mutations of values derived from CreateFrom are transitive against the value they came from. We handle this by keeping a queue of not just the places to visit during mutation, but whether to visit them transitively or not.
* TODO: we may also want to track whether we've seen a value as transitive or not
* For that array.map case w the mutable lambda, we weren't bubbling up the effects correctly so that we knew the param was mutated. Basically we inferred this information and then didn't record it. There is a bit of plumbing here.
* Similarly, if a function expression returns a function expression and the inner one mutates context, we weren't propagating that up to the outer function expression.
[ghstack-poisoned]
Fixes for a few cases:
* If you extract part of a value and mutate the part, that has to count as mutating the whole. See the new fixture, but this also came up with an existing test that has an array whose items are modified via array.map with a lambda that mutates its input. The key change here is preserving the CreateFrom effect (not downgrading to Alias) and then handling CreateFrom specially in range inference. Mutations of values derived from CreateFrom are transitive against the value they came from. We handle this by keeping a queue of not just the places to visit during mutation, but whether to visit them transitively or not.
* TODO: we may also want to track whether we've seen a value as transitive or not
* For that array.map case w the mutable lambda, we weren't bubbling up the effects correctly so that we knew the param was mutated. Basically we inferred this information and then didn't record it. There is a bit of plumbing here.
* Similarly, if a function expression returns a function expression and the inner one mutates context, we weren't propagating that up to the outer function expression.
[ghstack-poisoned]
Fixes for a few cases:
* If you extract part of a value and mutate the part, that has to count as mutating the whole. See the new fixture, but this also came up with an existing test that has an array whose items are modified via array.map with a lambda that mutates its input. The key change here is preserving the CreateFrom effect (not downgrading to Alias) and then handling CreateFrom specially in range inference. Mutations of values derived from CreateFrom are transitive against the value they came from. We handle this by keeping a queue of not just the places to visit during mutation, but whether to visit them transitively or not.
* TODO: we may also want to track whether we've seen a value as transitive or not
* For that array.map case w the mutable lambda, we weren't bubbling up the effects correctly so that we knew the param was mutated. Basically we inferred this information and then didn't record it. There is a bit of plumbing here.
* Similarly, if a function expression returns a function expression and the inner one mutates context, we weren't propagating that up to the outer function expression.
[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]
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]
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]
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]
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]
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]
Alias was akward becuase it had a specific data flow / mutability relationship — direct mutations affect both sides - but also populated the output. We need the ability to express the aliasing dataflwo/mutability relationship w/o expressing actual assignment.
An example of this is that for an unknown method call `const z = x.y()`, we want to express the Alias-style mutability relationship btw z and x (Muate(z) => Mutate(x)), but we want to assume that `z !== x`.
This ends up being really clean, the places that should use Assign are very obvious (LoadLocal, StoreLocal) and most places keep using Alias.
[ghstack-poisoned]
Alias was akward becuase it had a specific data flow / mutability relationship — direct mutations affect both sides - but also populated the output. We need the ability to express the aliasing dataflwo/mutability relationship w/o expressing actual assignment.
An example of this is that for an unknown method call `const z = x.y()`, we want to express the Alias-style mutability relationship btw z and x (Muate(z) => Mutate(x)), but we want to assume that `z !== x`.
This ends up being really clean, the places that should use Assign are very obvious (LoadLocal, StoreLocal) and most places keep using Alias.
[ghstack-poisoned]
Alias was akward becuase it had a specific data flow / mutability relationship — direct mutations affect both sides - but also populated the output. We need the ability to express the aliasing dataflwo/mutability relationship w/o expressing actual assignment.
An example of this is that for an unknown method call `const z = x.y()`, we want to express the Alias-style mutability relationship btw z and x (Muate(z) => Mutate(x)), but we want to assume that `z !== x`.
This ends up being really clean, the places that should use Assign are very obvious (LoadLocal, StoreLocal) and most places keep using Alias.
[ghstack-poisoned]
This ties all the ideas together, showing the promise of the new inference. When applying effects, when we encounter an Apply effect we now check to see if the function we're calling is a locally defined FunctionExpression. If so, we construct a signature for it on the fly (we already have created the effects in AnalyzeFunctions), substitute in the args to get a set of effects we can apply, and then recursively apply those effects.
This required adding an ability for signatures to declare additional temporary places that they can reference. For example, Array.prototype.map needs a temporary to represent the items it extracts from the receiver array, and another temporary for the result of calling the user-provided function.
This also meant adding a `CreateFunction` effect which a) allows us to preserve the FunctionExpression value in the inference state (the previous Create effect meant we just created a dummy ObjectExpression) and b) allows dynamically constructing the ValueKind of the function based on whether it actually captures any mutable values.
Lots of other little fixes as well, such as changing function related effects (and PropertyLoad) to use Alias instead of Capture so that subsequent mutations of the output count as mutations of the input.
[ghstack-poisoned]
This ties all the ideas together, showing the promise of the new inference. When applying effects, when we encounter an Apply effect we now check to see if the function we're calling is a locally defined FunctionExpression. If so, we construct a signature for it on the fly (we already have created the effects in AnalyzeFunctions), substitute in the args to get a set of effects we can apply, and then recursively apply those effects.
This required adding an ability for signatures to declare additional temporary places that they can reference. For example, Array.prototype.map needs a temporary to represent the items it extracts from the receiver array, and another temporary for the result of calling the user-provided function.
This also meant adding a `CreateFunction` effect which a) allows us to preserve the FunctionExpression value in the inference state (the previous Create effect meant we just created a dummy ObjectExpression) and b) allows dynamically constructing the ValueKind of the function based on whether it actually captures any mutable values.
Lots of other little fixes as well, such as changing function related effects (and PropertyLoad) to use Alias instead of Capture so that subsequent mutations of the output count as mutations of the input.
[ghstack-poisoned]
This ties all the ideas together, showing the promise of the new inference. When applying effects, when we encounter an Apply effect we now check to see if the function we're calling is a locally defined FunctionExpression. If so, we construct a signature for it on the fly (we already have created the effects in AnalyzeFunctions), substitute in the args to get a set of effects we can apply, and then recursively apply those effects.
This required adding an ability for signatures to declare additional temporary places that they can reference. For example, Array.prototype.map needs a temporary to represent the items it extracts from the receiver array, and another temporary for the result of calling the user-provided function.
This also meant adding a `CreateFunction` effect which a) allows us to preserve the FunctionExpression value in the inference state (the previous Create effect meant we just created a dummy ObjectExpression) and b) allows dynamically constructing the ValueKind of the function based on whether it actually captures any mutable values.
Lots of other little fixes as well, such as changing function related effects (and PropertyLoad) to use Alias instead of Capture so that subsequent mutations of the output count as mutations of the input.
[ghstack-poisoned]
To help bootstrap the new inference model, this PR adds a helper that takes a legacy FunctionSignature and converts into a list of (new) AliasingEffects. This conversion tries to make explicit all the implicit handling of InferReferenceEffects and previous FunctionSignature.
For example, the signature for Array.proto.pop has a calleeEffect of `Store`. Nowhere does it say that the receiver flows into the result! There's an implicit behavior that the receiver flows into the result. The new function makes this explicit by emitting a `Capture receiver -> lvalue` effect.
So far I confirmed that this works for Array.proto.push() if i hard code the inference to ignore new-style aliasing signatures. I'll continue to refine it going forward as I start running the new inference on more fixtures.
[ghstack-poisoned]
To help bootstrap the new inference model, this PR adds a helper that takes a legacy FunctionSignature and converts into a list of (new) AliasingEffects. This conversion tries to make explicit all the implicit handling of InferReferenceEffects and previous FunctionSignature.
For example, the signature for Array.proto.pop has a calleeEffect of `Store`. Nowhere does it say that the receiver flows into the result! There's an implicit behavior that the receiver flows into the result. The new function makes this explicit by emitting a `Capture receiver -> lvalue` effect.
So far I confirmed that this works for Array.proto.push() if i hard code the inference to ignore new-style aliasing signatures. I'll continue to refine it going forward as I start running the new inference on more fixtures.
[ghstack-poisoned]
To help bootstrap the new inference model, this PR adds a helper that takes a legacy FunctionSignature and converts into a list of (new) AliasingEffects. This conversion tries to make explicit all the implicit handling of InferReferenceEffects and previous FunctionSignature.
For example, the signature for Array.proto.pop has a calleeEffect of `Store`. Nowhere does it say that the receiver flows into the result! There's an implicit behavior that the receiver flows into the result. The new function makes this explicit by emitting a `Capture receiver -> lvalue` effect.
So far I confirmed that this works for Array.proto.push() if i hard code the inference to ignore new-style aliasing signatures. I'll continue to refine it going forward as I start running the new inference on more fixtures.
[ghstack-poisoned]
Adds an aliasing signature for Array.prototype.push and fixes up the logic for consuming these signatures during effect inference. As the test fixture shows, we correctly model the capturing. Mutable values that are captured into the array count as co-mutated if the array itself is transitively mutated later, while mutable values captured after such transitive mutation are not considered co-mutated.
The implementation is based on the fact that the final `push` call is only locally mutating the array. During the phase that looks at local mutations we are only tracking direct aliases, and the push doesn't directly alias the items to the array, it only captures them.
[ghstack-poisoned]
Joe Savona