[compiler] Fix bug w functions depending on hoisted primitives #35284
+126
−8
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
Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line.
Member
Author
|
Closes #35122 |
github-actions
bot
added
the
React Core Team
josephsavona
commented
Dec 4, 2025
Comment on lines
-392
to
-399
| if ( | ||
| instr.value.kind === 'FunctionExpression' || | ||
| instr.value.kind === 'ObjectMethod' | ||
| ) { | ||
| if (operand.identifier.type.kind === 'Primitive') { | ||
| continue; | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
intuitively this makes sense: we don't usually create scopes for primitives, but if we assigned a scope then it should follow all the regular merging logic of this pass
github-actions bot
pushed a commit
that referenced
this pull request
Dec 5, 2025
Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line. Closes #35122 DiffTrain build for [2cb08e6](2cb08e6)
github-actions bot
pushed a commit
that referenced
this pull request
Dec 5, 2025
Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line. Closes #35122 DiffTrain build for [2cb08e6](2cb08e6)
github-actions bot
pushed a commit
to code/lib-react
that referenced
this pull request
Dec 7, 2025
…ook#35284) Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line. Closes facebook#35122 DiffTrain build for [2cb08e6](facebook@2cb08e6)
github-actions bot
pushed a commit
to code/lib-react
that referenced
this pull request
Dec 7, 2025
…ook#35284) Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted `const` inferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables. This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to `object.value = ...` and reading the variable is equivalent to `object.value` property access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line. Closes facebook#35122 DiffTrain build for [2cb08e6](facebook@2cb08e6)
unstubbable
pushed a commit
to vercel/next.js
that referenced
this pull request
Dec 10, 2025
[diff facebook/react@378973b3...55480b4d](facebook/react@378973b...55480b4) <details> <summary>React upstream changes</summary> - facebook/react#35317 - facebook/react#35306 - facebook/react#35285 - facebook/react#35316 - facebook/react#35313 - facebook/react#35308 - facebook/react#35284 - facebook/react#35296 </details>
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.
Fixes an edge case where a function expression would fail to take a dependency if it referenced a hoisted
constinferred as a primitive value. We were incorrectly skipping primitve-typed operands when determing scopes for merging in InferReactiveScopeVariables.This was super tricky to debug, for posterity the trick is that Context variables (StoreContext etc) are modeled just like a mutable object, where assignment to the variable is equivalent to
object.value = ...and reading the variable is equivalent toobject.valueproperty access. Comparing to an equivalent version of the repro case replaced with an object and property read/writes showed that everything was exactly right, except that InferReactiveScopeVariables wasn't merging the scopes of the function and the context variable, which led me right to the problematic line.Closes #35122