Skip to content
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][repro] False positives for ValidatePreserveMemo #30629

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2024 11:17pm

* of its scope (but after its mutable range ends).
*/

function useFoo(a, b) {
Copy link
Contributor Author

@mofeiZ mofeiZ Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only false positive not fixed by #30630 and unrelated to #30603. Included since it shows a use-case for an identifier's original mutable range (instead of the currently used scope range).

Currently, forget (incorrectly) infers that x is potentially mutated after StartMemoize deps=[x, ...]

useIdentity(2);
mutate(x);

return useCallback(() => [x, []], [x]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a false positive because the input wasn't memoized in source, right? Just making sure i'm interpreting this the same way you are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly! I added this fixture to show changes from #30630

Comment on lines +13 to +14
* We currently bail out because `a` has a scope and is not transitively memoized
* (as its scope is pruned due to a hook call)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - looks like we are grouping the argument destructuring into a scope? that seems wrong, a and b aren't mutable so they shouldn't be getting added into any scopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is because x is a context variable, and since our understanding of mutable context variables is imprecise, x and all potential aliases get marked as mutable. I might be incorrect though -- can look into this more

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are great fixtures, fine to land. See comments, i wonder if the fix might be different for some of these than is suggested by the code comments

@mofeiZ mofeiZ merged commit 2857460 into gh/mofeiZ/17/base Aug 8, 2024
19 checks passed
mofeiZ added a commit that referenced this pull request Aug 8, 2024
ghstack-source-id: 7fa94fea867371f3d77737dad80a321094f10600
Pull Request resolved: #30629
@mofeiZ mofeiZ deleted the gh/mofeiZ/17/head branch August 8, 2024 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants