-
Notifications
You must be signed in to change notification settings - Fork 49k
[compiler] Fix for false positive mutation of destructured spread object #33786
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
Closes #33781 |
t0 = nonPrimaryButtons.map(_temp); | ||
$[2] = nonPrimaryButtons; | ||
$[3] = t0; | ||
$[0] = buttons; |
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.
this is the example where i realized we weren't inferring array pattern spread as an array type
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.
Ahh makes sense. MergeReactiveScopesThatInvalidateTogether
uses type info as a proxy for whether something was newly created, which currently works due to how we infer types
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.
Makes sense! Thanks for the local mutation test case to check that we correctly infer mutations on object spread! I guess we don't gain much from inferring BuiltInObjectId
for destructured object spreads. The only benefits seem... arguably incorrect (objects can define mutating toString
properties that override the prototype) or not as important.
- inference of object properties, which is currently only the (not very helpful)
toString
method from the Object prototype. We could also addhasOwnProperty
, but that's about it.
function Component({prop}) {
'use memo';
const {value, ...rest} = prop;
// we can infer that `toString` does not mutate its operand
return rest.toString();
}
MergeReactiveScopesThatInvalidateTogether
function Component({prop}) {
'use memo';
const {value, ...rest} = prop;
// we can merge the scope that creates {rest} with the destructuring scope
return {rest}
}
When destructuring, spread creates a new mutable object that _captures_ part of the original rvalue. This new value is safe to modify. When making this change I realized that we weren't inferring array pattern spread as creating an array (in type inference) so I also added that here.
Yeah, fair. Let's come back to the object case in a follow-up, as a potential optimization for scope merging. Scope merging in general could use some work to handle a few more common cases like an intermediate StoreLocal btw scopes. |
…ect (#33786) When destructuring, spread creates a new mutable object that _captures_ part of the original rvalue. This new value is safe to modify. When making this change I realized that we weren't inferring array pattern spread as creating an array (in type inference) so I also added that here. DiffTrain build for [448f781](448f781)
…ect (#33786) When destructuring, spread creates a new mutable object that _captures_ part of the original rvalue. This new value is safe to modify. When making this change I realized that we weren't inferring array pattern spread as creating an array (in type inference) so I also added that here. DiffTrain build for [448f781](448f781)
When destructuring, spread creates a new mutable object that captures part of the original rvalue. This new value is safe to modify.
When making this change I realized that we weren't inferring array pattern spread as creating an array (in type inference) so I also added that here.