-
Notifications
You must be signed in to change notification settings - Fork 49k
[compiler] Fix for uncalled functions that are known-mutable #33078
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
Merged
Conversation
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
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. If we do that we'd also want to similarly disallow cases like passing a mutable function to a hook. [ghstack-poisoned]
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. If we do that we'd also want to similarly disallow cases like passing a mutable function to a hook. [ghstack-poisoned]
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. However, even if we do that we likely won't be able to flip the validation on immediately, so I think it still makes sense to have the compiler do the more-correct thing in the invalid case. Especially considering that there are examples, like with pure caching, that technically violate the rules but are pure in practice. [ghstack-poisoned]
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. However, even if we do that we likely won't be able to flip the validation on immediately, so I think it still makes sense to have the compiler do the more-correct thing in the invalid case. Especially considering that there are examples, like with pure caching, that technically violate the rules but are pure in practice. [ghstack-poisoned]
mvitousek
approved these changes
May 2, 2025
josephsavona
added a commit
that referenced
this pull request
May 3, 2025
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures. This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example: ```js hook useFoo() { const x = makeObject(); return () => { return readObject(x); // could be a mutation! } } ``` If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation: ```js hook useFoo(createEntryForKey) { const cache = new WeakMap(); return (key) => { let entry = cache.get(key); if (entry == null) { entry = createEntryForKey(key); cache.set(key, entry); // known mutation! } return entry; } } ``` Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1). This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together *and* that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set. NOTE: The alternative is to reject these cases. If we do that we'd also want to similarly disallow cases like passing a mutable function to a hook. ghstack-source-id: 5d81582 Pull Request resolved: #33078
This was referenced May 5, 2025
This was referenced May 12, 2025
This was referenced May 19, 2025
This was referenced May 21, 2025
17 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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):
If a function captures a mutable value but never gets called, we don't infer a mutable range for that function. This means that we also don't alias the function with its mutable captures.
This case is tricky, because we don't generally know for sure what is a mutation and what may just be a normal function call. For example:
If we pessimistically assume that all such cases are mutations, we'd have to group lots of memo scopes together unnecessarily. However, if there is definitely a mutation:
Then we have to ensure that the function and its mutable captures alias together and end up in the same scope. However, aliasing together isn't enough if the function and operands all have empty mutable ranges (end = start + 1).
This pass finds function expressions and object methods that have an empty mutable range and known-mutable operands which also don't have a mutable range, and ensures that the function and those operands are aliased together and that their ranges are updated to end after the function expression. This is sufficient to ensure that a reactive scope is created for the alias set.
NOTE: The alternative is to reject these cases. However, even if we do that we likely won't be able to flip the validation on immediately, so I think it still makes sense to have the compiler do the more-correct thing in the invalid case. Especially considering that there are examples, like with pure caching, that technically violate the rules but are pure in practice.