[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo #34855

josephsavona · 2025-10-15T04:53:50Z

Potential fix for #34750. With a useMemo with multiple returns we can sometimes end up thinking the FinishMemoize value wasn't memoized, even though it definitely is. But the same case works if you rewrite to have a let binding, assign to the let instead of returning, and then have a single return with the let — which is the exact shape we convert to when we inline the useMemo! The only difference is that when we inline, we end up with an extra LoadLocal. Adding an extra LoadLocal "fixes" the issue but this is definitely a hack.

However, along the way this helped me find that we also have a bug in PruneAlwaysInvalidatingScopes, where we were forgetting to set FinishMemoize instructions to pruned if their declaration always invalidates.

Putting up for feedback, not necessarily expecting to land as-is.

Stack created with Sapling. Best reviewed with ReviewStack.

…34847) In InferTypes when we infer types for properties during destructuring, we were breaking out of the loop when we encounter a hole in the array. Instead we should just skip that element and continue inferring later properties. Closes #34748 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34847). * #34855 * __->__ #34847

…34847) In InferTypes when we infer types for properties during destructuring, we were breaking out of the loop when we encounter a hole in the array. Instead we should just skip that element and continue inferring later properties. Closes #34748 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34847). * #34855 * __->__ #34847 DiffTrain build for [e096403](e096403)

We now do a single pass over the HIR, building up two data structures: * One tracks values that are known macro tags or macro calls. * One tracks operands of macro-related instructions so that we can later group them. After building up these data structures, we do a pass over the latter structure. For each macro call instruction, we recursively traverse its operands to ensure they're in the same scope. Thus, something like `fbt('hello' + fbt.param(foo(), "..."))` will correctly merge the fbt call, the `+` binary expression, the `fbt.param()` call, and `foo()` into a single scope. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34865). * #34855 * __->__ #34865

We now do a single pass over the HIR, building up two data structures: * One tracks values that are known macro tags or macro calls. * One tracks operands of macro-related instructions so that we can later group them. After building up these data structures, we do a pass over the latter structure. For each macro call instruction, we recursively traverse its operands to ensure they're in the same scope. Thus, something like `fbt('hello' + fbt.param(foo(), "..."))` will correctly merge the fbt call, the `+` binary expression, the `fbt.param()` call, and `foo()` into a single scope. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34865). * #34855 * __->__ #34865 DiffTrain build for [85f415e](85f415e)

We now do a single pass over the HIR, building up two data structures: * One tracks values that are known macro tags or macro calls. * One tracks operands of macro-related instructions so that we can later group them. After building up these data structures, we do a pass over the latter structure. For each macro call instruction, we recursively traverse its operands to ensure they're in the same scope. Thus, something like `fbt('hello' + fbt.param(foo(), "..."))` will correctly merge the fbt call, the `+` binary expression, the `fbt.param()` call, and `foo()` into a single scope. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34865). * facebook#34855 * __->__ facebook#34865 DiffTrain build for [85f415e](facebook@85f415e)

Two additional validations for useMemo: * Disallow reassigning to values declared outside the useMemo callback (always on) * Disallow unused useMemo calls (part of the validateNoVoidUseMemo feature flag, which in turn is off by default) We should probably enable this flag though! --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34868). * #34855 * #34882 * __->__ #34868

This is a great validation, so let's enable by default. Changes: * Move the validation logic into ValidateUseMemo alongside the new check that the useMemo result is used * Update the lint description * Make the void memo errors lint-only, they don't require us to skip compilation (as evidenced by the fact that we've had this validation off)

…urn in useMemo Potential fix for #34750. With a useMemo with multiple returns we can sometimes end up thinking the FinishMemoize value wasn't memoized, even though it definitely is. But the same case works if you rewrite to have a let binding, assign to the let instead of returning, and then have a single return with the let — which is the exact shape we convert to when we inline the useMemo! The only difference is that when we inline, we end up with an extra LoadLocal. Adding an extra LoadLocal "fixes" the issue but this is definitely a hack. However, along the way this helped me find that we also have a bug in PruneAlwaysInvalidatingScopes, where we were forgetting to set FinishMemoize instructions to pruned if their declaration always invalidates. Putting up for feedback, not necessarily expecting to land as-is.

This is a great validation, so let's enable by default. Changes: * Move the validation logic into ValidateUseMemo alongside the new check that the useMemo result is used * Update the lint description * Make the void memo errors lint-only, they don't require us to skip compilation (as evidenced by the fact that we've had this validation off) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34882). * #34855 * __->__ #34882

Two additional validations for useMemo: * Disallow reassigning to values declared outside the useMemo callback (always on) * Disallow unused useMemo calls (part of the validateNoVoidUseMemo feature flag, which in turn is off by default) We should probably enable this flag though! --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34868). * #34855 * #34882 * __->__ #34868 DiffTrain build for [7f5ea1b](7f5ea1b)

This is a great validation, so let's enable by default. Changes: * Move the validation logic into ValidateUseMemo alongside the new check that the useMemo result is used * Update the lint description * Make the void memo errors lint-only, they don't require us to skip compilation (as evidenced by the fact that we've had this validation off) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34882). * #34855 * __->__ #34882 DiffTrain build for [1324e1b](1324e1b)

We now do a single pass over the HIR, building up two data structures: * One tracks values that are known macro tags or macro calls. * One tracks operands of macro-related instructions so that we can later group them. After building up these data structures, we do a pass over the latter structure. For each macro call instruction, we recursively traverse its operands to ensure they're in the same scope. Thus, something like `fbt('hello' + fbt.param(foo(), "..."))` will correctly merge the fbt call, the `+` binary expression, the `fbt.param()` call, and `foo()` into a single scope. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34865). * facebook#34855 * __->__ facebook#34865 DiffTrain build for [85f415e](facebook@85f415e)

josephsavona mentioned this pull request Oct 15, 2025

[compiler] Infer types for properties after holes in array patterns #34847

Merged

meta-cla bot added the CLA Signed label Oct 15, 2025

github-actions bot added the React Core Team Opened by a member of the React Core Team label Oct 15, 2025

josephsavona requested review from mofeiZ, mvitousek and poteto October 15, 2025 15:37

josephsavona mentioned this pull request Oct 15, 2025

[compiler] Fix fbt for the ∞th time #34865

Merged

josephsavona force-pushed the pr34855 branch 4 times, most recently from a695dfe to 6772dc0 Compare October 15, 2025 23:12

josephsavona mentioned this pull request Oct 16, 2025

[compiler] More useMemo validation #34868

Merged

josephsavona force-pushed the pr34855 branch 3 times, most recently from 64e2484 to 3800821 Compare October 16, 2025 19:27

josephsavona mentioned this pull request Oct 16, 2025

[compiler] Cleanup and enable validateNoVoidUseMemo #34882

Merged

josephsavona force-pushed the pr34855 branch from 3800821 to a16291f Compare October 16, 2025 19:57

josephsavona added 2 commits October 16, 2025 13:08

josephsavona force-pushed the pr34855 branch from a16291f to 55bf051 Compare October 16, 2025 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo #34855

[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo #34855

josephsavona commented Oct 15, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo #34855

Are you sure you want to change the base?

[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo #34855

Conversation

josephsavona commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josephsavona commented Oct 15, 2025 •

edited

Loading