[compiler][newinference] Fixes for transitive function capturing, mutation via property loads#33430
Closed
josephsavona wants to merge 11 commits intogh/josephsavona/113/basefrom
Closed
[compiler][newinference] Fixes for transitive function capturing, mutation via property loads#33430josephsavona wants to merge 11 commits intogh/josephsavona/113/basefrom
josephsavona wants to merge 11 commits intogh/josephsavona/113/basefrom
Conversation
…ation via property loads [ghstack-poisoned]
This was referenced Jun 3, 2025
This was referenced Jun 4, 2025
github-actions
bot
added
the
React Core Team
josephsavona
commented
Jun 4, 2025
Comment on lines
+319
to
+321
| if (!context.has(effect.value.identifier.id)) { | ||
| continue; | ||
| } |
Member
Author
There was a problem hiding this comment.
function expression effects now include effects for params, but these values won't exist in the outer context so we skip them
| break; | ||
| } | ||
| case 'CreateFunction': { | ||
| effects.push(effect); |
Member
Author
There was a problem hiding this comment.
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
Comment on lines
+656
to
+662
| applyEffect( | ||
| context, | ||
| state, | ||
| {kind: 'MutateTransitiveConditionally', value: effect.function}, | ||
| aliased, | ||
| effects, | ||
| ); |
Member
Author
There was a problem hiding this comment.
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]
This was referenced Jun 6, 2025
…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]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Fixes for a few cases: