Skip to content

[commit] Better error message for invalid hoisting #33504

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Squashed, review-friendly version of the stack from #33488.

This is new version of our mutability and inference model, designed to replace the core algorithm for determining the sets of instructions involved in constructing a given value or set of values. The new model replaces InferReferenceEffects, InferMutableRanges (and all of its subcomponents), and parts of AnalyzeFunctions. The new model does not use per-Place effect values, but in order to make this drop-in the end _result_ of the inference adds these per-Place effects.

I'll write up a larger document on the model, first i'm doing some housekeeping to rebase the PR.
AnalyzeFunctions had logic to reset the mutable ranges of context variables after visiting inner function expressions. However, there was a bug in that logic: InferReactiveScopeVariables makes all the identifiers in a scope point to the same mutable range instance. That meant that it was possible for a later function expression to indirectly cause an earlier function expressions' context variables to get a non-zero mutable range.

The fix is to not just reset start/end of context var ranges, but assign a new range instance. Thanks for the help on debugging, @mofeiZ!
We're already tracking which variables are hoisted context variables, so if we see a mutation of a frozen value we can emit a custom error message to help users identify the problem.
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.

2 participants