-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
[compiler] Propogate effects stored in object props #30457
Conversation
Previously, we propogated only effects of functions/methods to the outer function context. In this PR, we have extended the propagation of function/method effects to include those captured in object expressions. This change is generally a no-op, as the functions defined within the same context as the object expression already had their effects captured. This new infra allows us to delete certain effects, such as global mutation, for object expressions that are passed down as props. [ghstack-poisoned]
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Previously, we propogated only effects of functions/methods to the outer function context. In this PR, we have extended the propagation of function/method effects to include those captured in object expressions. This change is generally a no-op, as the functions defined within the same context as the object expression already had their effects captured. This new infra allows us to delete certain effects, such as global mutation, for object expressions that are passed down as props. [ghstack-poisoned]
const functionEffect = this.reference(place, effectKind, reason); | ||
if (functionEffect !== null) { | ||
functionEffects.push(functionEffect); | ||
} | ||
} | ||
|
||
propogateEffect( |
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.
propogateEffect( | |
propagateEffect( |
very minor spelling nit
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 couple questions:
- should we do the same for array expressions?
- just double-checking, is this safe in the case of the object being mutated? I guess it should be since we are eagerly evaluating the effects of the properties and propagating them to the parent function, so it doesn’t matter what happens to the object. If anything the main risk is of false positives where we eagerly propagate a function effect bc of a function property that is assigned, even though it’s subsequently deleted or reassigned from the object before it could be called.
It also seems like this wouldn’t handle the case where a mutating function is attached to the object after creation.
ie const obj = {mutatingFunction}
will be allowed but const obj ={}; obj.mutatingFunction = mutatingFunction
won’t, even if the objects are otherwise used the same way.
I think it still makes sense to land this as-is (maybe add support for array expressions) but just some things to consider.
Stack from ghstack (oldest at bottom):
Previously, we propogated only effects of functions/methods to the outer
function context.
In this PR, we have extended the propagation of function/method effects
to include those captured in object expressions. This change is
generally a no-op, as the functions defined within the same context as
the object expression already had their effects captured.
This new infra allows us to delete certain effects, such as global
mutation, for object expressions that are passed down as props.