We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
React Compiler's program traversal logic is pretty lengthy and complex
as we've added a lot of features piecemeal. `compileProgram` is 300+
lines long and has confusing control flow (defining helpers inline,
invoking visitors, mutating-asts-while-iterating, mutating global
`ALREADY_COMPILED` state).
- Moved more stuff to `ProgramContext`
- Separated `compileProgram` into a bunch of helpers
Tested by syncing this stack to a Meta codebase and observing no
compilation output changes (D74487851, P1806855669, P1806855379)
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33147).
* #33149
* #33148
* __->__ #33147
We've occassionally added logic that extends mutable ranges into InferReactiveScopeVariables to handle a specific case, but inevitably discover that the logic needs to be part of the InferMutableRanges fixpoint loop. That happened in the past with extending the range of phi operands to account for subsequent mutations, which I moved to InferMutableRanges a while back. But InferReactiveScopeVariables also has logic to group co-mutations in the same scope, which also extends ranges of the co-mutating operands to have the same end point. Recently mofeiz found some cases where this is insufficient, where a closure captures a value that could change via a co-mutation, and where failure to extend the ranges in the fixpoint meant the function expression appeared independently memoizable when it wasn't.
The fix is to make InferMutableRanges update ranges to account for co-mutations. That is relatively straightforward, but not enough! The problem is that the fixpoint loop stopped once the alias sets coalesced, but co-mutations only affect ranges and not aliases. So the other part of the fix is to have the fixpoint condition use a custom canonicalization that describes each identifiers root _and_ the mutable range of that root.
[ghstack-poisoned]
Stacked on #33150.
We use `noop` functions in a lot of places as place holders. I don't
think there's any real optimizations we get from having separate
instances. This moves them to use a common instance in `shared/noop`.
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
New take on #29716
## Summary
Template literals consisting entirely of constant values will be inlined
to a string literal, effectively replacing the backticks with a double
quote.
This is done primarily to make the resulting instruction a string
literal, so it can be processed further in constant propatation. So this
is now correctly simplified to `true`:
```js
`` === "" // now true
`a${1}` === "a1" // now true
```
If a template string literal can only partially be comptime-evaluated,
it is not that useful for dead code elimination or further constant
folding steps and thus, is left as-is in that case. Same is true if the
literal contains an array, object, symbol or function.
## How did you test this change?
See added tests.
(Almost) all pragmas are now one of the following:
- `@...TestOnly`: custom pragma for test fixtures
- `@<configName>` | `@<configName>:true`: enables with either true or a
default enabled value
- `@<configName>:<json value>`
This is a stab at addressing a pattern that @mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.
From an example @mofeiz added:
```js
const x = {};
const y = {};
const f = () => {
const a = [y];
const b = x;
// this sets y.x = x
a[0].x = b;
}
f();
mutate(y.x); // which means this mutates x!
```
Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.
The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:
* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.
This isn't quite right yet, just sharing early hacking.
[ghstack-poisoned]
The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value.
With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable.
[ghstack-poisoned]
The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value.
With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable.
[ghstack-poisoned]
`fragmentInstance.dispatchEvent(evt)` calls `element.dispatchEvent(evt)`
on the fragment's host parent. This mimics bubbling if the
`fragmentInstance` could receive an event itself.
If the parent is disconnected, there is a dev warning and no event is
dispatched.
## Summary
`-constant` is represented as a `UnaryExpression` node that is currently
not part of constant folding. If the operand is a constant number, the
node is folded to `constant * -1`. This also coerces `-0` to `0`,
resulting in `0 === -0` being folded to `true`.
## How did you test this change?
See attached tests
Follow up to #33136.
This clarifies in the types where the conversion happens from a CallSite
which we use to simulate getting the enclosing line/col to a
FunctionLocation which doesn't represent a CallSite but actually just
the function which only has an enclosing line/col.
This enables `focus` and `focusLast` methods on FragmentInstances to
search nested host components, depth first. Attempts focus on each child
and bails if one is successful. Previously, only the first level of host
children would attempt focus.
Now if we have an example like
```
component MenuItem() {
return (<div><a>{...}</a></div>)
}
component Menu() {
return <Fragment>{items.map(i => <MenuItem i={i} />)}</Fragment>
}
```
We can target focus on the first or last a tag, rather than checking
each wrapping div and then noop.
Stacked on #33135.
This encodes the line/column of the enclosing function as part of the
stack traces. When that information is available.
I adjusted the fake function code generation so that the beginning of
the arrow function aligns with these as much as possible.
This ensures that when the browser tries to look up the line/column of
the enclosing function, such as for getting the function name, it gets
the right one. If we can't get the enclosing line/column, then we encode
it at the beginning of the file. This is likely to get a miss in the
source map identifiers, which means that the function name gets
extracted from the runtime name instead which is better.
Another thing where this is used is the in the Performance Track.
Ideally that would be fixed by
https://issues.chromium.org/u/1/issues/415968771 but the enclosing
information is useful for other things like the function name resolution
anyway.
We can also use this for the "View source for this element" in React
DevTools.
This is first step to include more enclosing line/column in the parsed
data.
We install our own `prepareStackTrace` to collect structured callsite
data and only fall back to parsing the string if it was already
evaluated or if `prepareStackTrace` doesn't work in this environment.
We still mirror the default V8 format for encoding the function name
part. A lot of this is covered by tests already.
## Summary
When using React DevTools to highlight component updates, the highlights
would sometimes appear behind elements that use the browser's
[top-layer](https://developer.mozilla.org/en-US/docs/Glossary/Top_layer)
(such as `<dialog>` elements or components using the Popover API). This
made it difficult to see which components were updating when they were
inside or behind top-layer elements.
This PR fixes the issue by using the Popover API to ensure that
highlighting appears on top of all content, including elements in the
top-layer. The implementation maintains backward compatibility with
browsers that don't support the Popover API.
## How did you test this change?
I tested this change in the following ways:
1. Manually tested in Chrome (which supports the Popover API) with:
- Created a test application with React components inside `<dialog>`
elements and custom elements using the Popover API
- Verified that component highlighting appears above these elements when
they update
- Confirmed that highlighting displays correctly for nested components
within top-layer elements
2. Verified backward compatibility:
- Tested in browsers without Popover API support to ensure fallback
behavior works correctly
- Confirmed that no errors occur and highlighting still functions as
before
3. Ran the React DevTools test suite:
- All tests pass successfully
- No regressions were introduced
[demo-page](https://devtools-toplayer-demo.vercel.app/)
[demo-repo](https://github.com/yongsk0066/devtools-toplayer-demo)
### AS-IS
https://github.com/user-attachments/assets/dc2e1281-969f-4f61-82c3-480153916969
### TO-BE
https://github.com/user-attachments/assets/dd52ce35-816c-42f0-819b-0d5d0a8a21e5
This adds `compareDocumentPosition(otherNode)` to fragment instances.
The semantics implemented are meant to match typical element
positioning, with some fragment specifics. See the unit tests for all
expectations.
- An element preceding a fragment is `Node.DOCUMENT_POSITION_PRECEDING`
- An element after a fragment is `Node.DOCUMENT_POSITION_FOLLOWING`
- An element containing the fragment is
`Node.DOCUMENT_POSITION_PRECEDING` and
`Node.DOCUMENT_POSITION_CONTAINING`
- An element within the fragment is
`Node.DOCUMENT_POSITION_CONTAINED_BY`
- An element compared against an empty fragment will result in
`Node.DOCUMENT_POSITION_DISCONNECTED` and
`Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC`
Since we assume a fragment instances target children are DOM siblings
and we want to compare the full fragment as a pseudo container, we can
compare against the first target child outside of handling the special
cases (empty fragments and contained elements).
Multiple things here:
- Improve the mean calculation for metrics so we don't report 0 when
web-vitals fail to be retrieved
- improve ui chaos monkey to use puppeteer APIs since only those trigger
INP/CLS metrics since we need emulated mouse clicks
- Add logic to navigate to a temp page after render since some
web-vitals metrics are only calculated when the page is backgrounded
- Some readability improvements
Originally I thought it was important that SSR used the same View
Transition name as the client so that the Fizz runtime could emit those
names and then the client could pick up and take over. However, I no
longer believe that approach is feasible. Instead, the names can be
generated only during that particular animation.
Therefore we can simplify the auto name assignment to not have to
consider the hydration.
Stacked on #33129. Flagged behind `enableHydrationChangeEvent`.
If you type into a controlled input before hydration and something else
rerenders like a setState in an effect, then the controlled input will
reset to whatever React thought it was. Even with event replaying that
this is stacked on, if the second render happens before event replaying
has fired in a separate task.
We don't want to flush inside the commit phase because then things like
flushSync in these events wouldn't work since they're inside the commit
stack.
This flushes all event replaying between renders by flushing it at the
end of `flushSpawned` work. We've already committed at that point and is
about to either do subsequent renders or yield to event loop for passive
effects which could have these events fired anyway. This just ensures
that they've already happened by the time subsequent renders fire. This
means that there's now a type of event that fire between sync render
passes.
This fixes a long standing issue that controlled inputs gets out of sync
with the browser state if it's changed before we hydrate.
This resolves the issue by replaying the change events (click, input and
change) if the value has changed by the time we commit the hydration.
That way you can reflect the new value in state to bring it in sync. It
does this whether controlled or uncontrolled.
The idea is that this should be ok to replay because it's similar to the
continuous events in that it doesn't replay a sequence but only reflects
the current state of the tree.
Since this is a breaking change I added it behind
`enableHydrationChangeEvent` flag.
There is still an additional issue remaining that I intend to address in
a follow up. If a `useLayoutEffect` triggers an sync rerender on
hydration (always a bad idea) then that can rerender before we have had
a chance to replay the change events. If that renders through a input
then that input will always override the browser value with the
controlled value. Which will reset it before we've had a change to
update to the new value.
Note that bailing out adds false positives for hoisted functions whose
only references are within other functions. For example, this rewrite
would be safe.
```js
// source program
function foo() {
return bar();
}
function bar() {
return 42;
}
// compiler output
let bar;
if (/* deps changed */) {
function foo() {
return bar();
}
bar = function bar() {
return 42;
}
}
```
These false positives are difficult to detect because any maybe-call of
foo before the definition of bar would be invalid.
Instead of bailing out, we should rewrite hoisted function declarations
to the following form.
```js
let bar$0;
if (/* deps changed */) {
// All references within the declaring memo block
// or before the function declaration should use
// the original identifier `bar`
function foo() {
return bar();
}
function bar() {
return 42;
}
bar$0 = bar;
}
// All references after the declaring memo block
// or after the function declaration should use
// the rewritten declaration `bar$0`
```
I noticed that we increase this in the recursive part of the algorithm.
This would mean that we'd count a key more than once if it has Server
Components inside it recursively resolving. This moves it out to where
we enter from toJSON. Which is called once per JSON entry (and therefore
once per key).
The issue in the previous PR was due to a ContextMutation function effect having a place that wasn't one of the functions' context variables. What was happening is that the `getContextRefOperand()` helper wasn't following aliases. If an operand had a context type, we recorded the operand as the context place — but instead we should be looking through to the context places of the abstract value.
With this change the fixture now fails for a different reason — we infer this as a mutation of `params` and reject it because `params` is frozen (hook return value). This case is clearly a false positive: the mutation is on the outer, new `nextParams` object and can't possibly mutate `params`. Need to think more about what to do here but this is clearly more precise in terms of which variable we record as the context variable.
[ghstack-poisoned]
Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away.
Anyway I'll keep debugging, putting up a repro for now.
[ghstack-poisoned]
Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away.
Anyway I'll keep debugging, putting up a repro for now.
[ghstack-poisoned]
This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal.
We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error.
This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default.
ghstack-source-id: 075a731444
Pull Request resolved: https://github.com/facebook/react/pull/33079
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: 5d8158246a
Pull Request resolved: https://github.com/facebook/react/pull/33078
Joe Savona
mofeiZ
Sebastian Markbåge
Jack Pope
Niklas Mollenhauer
Dawid Małecki
Ruslan Lesiutin
YongSeok Jang (장용석)
Jorge Cabiedes
Matt Carroll
Sebastian "Sebbie" Silbermann
Stephen Zhou