-
Notifications
You must be signed in to change notification settings - Fork 48.7k
[compiler][newinference] Fixes for transitive function capturing, mutation via property loads #33430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ation via property loads [ghstack-poisoned]
if (!context.has(effect.value.identifier.id)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function expression effects now include effects for params, but these values won't exist in the outer context so we skip them
@@ -467,6 +475,7 @@ function applyEffect( | |||
break; | |||
} | |||
case 'CreateFunction': { | |||
effects.push(effect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a consistent theme is that effects should almost always be preserved rather than dropped, so that range analysis can understand them. here, we want to know about the creation
applyEffect( | ||
context, | ||
state, | ||
{kind: 'MutateTransitiveConditionally', value: effect.function}, | ||
aliased, | ||
effects, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if we're able to construct a signature for a local function expression, we still need to consider it as mutating to account for mutations of its captures.
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
…turing, mutation via property loads" 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]
Stack from ghstack (oldest at bottom):
Fixes for a few cases: