The new mutation/aliasing model significantly expands on the idea of
FunctionEffect. The type (and its usage in HIRFunction.effects) was only
necessary for the now-deleted old inference model so we can clean up
this code now.
Hints are meant as additional information to present to the developer
about an error. The first use-case here is for the suggestion to name
refs with "-Ref" if we encounter a mutation that looks like it might be
a ref. The original error printing used a second error detail which
printed the source code twice, a hint with just extra text is less
noisy.
If you have a ref that the compiler doesn't know is a ref (say, a value
returned from a custom hook) and try to assign its `.current = ...`, we
currently fail with a generic error that hook return values are not
mutable. However, an assignment to `.current` specifically is a very
strong hint that the value is likely to be a ref. So in this PR, we
track the reason for the mutation and if it ends up being an error, we
use it to show an additional hint to the user. See the fixture for an
example of the message.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34125).
* #34126
* __->__ #34125
* #34124
We try to merge consecutive reactive scopes that will always invalidate
together, but there's one common case that isn't handled.
```js
const y = [[x]];
```
Here we'll create two consecutive scopes for the inner and outer array
expressions. Because the input to the second scope is a temporary,
they'll merge into one scope.
But if we name the inner array, the merging stops:
```js
const array = [x];
const y = [array];
```
This is because the merging logic checks if all the dependencies of the
second scope are outputs of the first scope, but doesn't account for
renaming due to LoadLocal/StoreLocal. The fix is to track these
temporaries.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34049).
* __->__ #34049
* #34047
* #34044
Fixes remaining issue in #32261, where passing a previously useMemo()-d
value to `Object.entries()` makes the compiler think the value is
mutated and fail validatePreserveExistingMemo. While I was there I added
Object.keys() and Object.values() too.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34047).
* #34049
* __->__ #34047
* #34044
This currently throws an invariant which may be misleading. I checked
the ecma262 spec and used the same list of reserved words in our check.
To err on the side of being conservative, we also error when strict mode
reserved words are used.
Allows assigning a ref-accessing function to an object so long as that
object is not subsequently transitively mutated. We should likely
rewrite the ref validation to use the new mutation/aliasing effects,
which would provide a more consistent behavior across instruction types
and require fewer special cases like this.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34026).
* #34027
* __->__ #34026
Fixes#30782
When developers do an `if (ref.current == null)` guard for lazy ref
initialization, the "safe" blocks should extend up to the if's
fallthrough. Previously we only allowed writing to the ref in the if
consequent, but this meant that you couldn't use a ternary, logical, etc
in the if body.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34024).
* #34027
* #34026
* #34025
* __->__ #34024
We infer render helpers as functions whose result is immediately
interpolated into jsx. This is a very conservative approximation, to
help with common cases like `<Foo>{props.renderItem(ref)}</Foo>`. The
idea is similar to hooks that it's ultimately on the developer to catch
ref-in-render validations (and the runtime detects them too), so we can
be a bit more relaxed since there are valid reasons to use this pattern.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34006).
* #34027
* #34026
* #34025
* #34024
* #34005
* __->__ #34006
* #34004
Two related changes:
* ValidateNoRefAccessInRender now allows the mergeRefs pattern, ie a
function that aggregates multiple refs into a new ref. This is the main
case where we have seen false positive no-ref-in-render errors.
* Behind `@enableTreatRefLikeIdentifiersAsRefs`, we infer values passed
as the `ref` prop to some JSX as refs.
The second change is potentially helpful for situations such as
```js
function Component({ref: parentRef}) {
const childRef = useRef(null);
const mergedRef = mergeRefs(parentRef, childRef);
useEffect(() => {
// generally accesses childRef, not mergedRef
}, []);
return <Foo ref={mergedRef} />;
}
```
Ie where you create a merged ref but don't access its `.current`
property. Without inferring `ref` props as refs, we'd fail to allow this
merge refs case.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34004).
* #34027
* #34026
* #34025
* #34024
* #34005
* #34006
* __->__ #34004
We added the `@enableTreatRefLikeIdentifiersAsRefs` feature a while back
but never enabled it. Since then we've continued to see examples that
motivate this mode, so here we're fixing it up to prepare to enable by
default. It now works as follows:
* If we find a property load or property store where both a) the
object's name is ref-like (`ref` or `-Ref`) and b) the property is
`current`, we infer the object itself as a ref and the value of the
property as a ref value. Originally the feature only detected property
loads, not stores.
* Inferred refs are not considered stable (this is a change from the
original implementation). The only way to get a stable ref is by calling
`useRef()`. We've seen issues with assuming refs are stable.
With this change, cases like the following now correctly error:
```js
function Foo(props) {
const fooRef = props.fooRef;
fooRef.current = true;
^^^^^^^^^^^^^^ cannot modify ref in render
}
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34000).
* #34027
* #34026
* #34025
* #34024
* #34005
* #34006
* #34004
* #34003
* __->__ #34000
Noticed this from my previous PR that this pass was throwing on the
first error. This PR is a small refactor to aggregate every violation
and report them all at once.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34002).
* #34022
* __->__ #34002
Much of the logic in the new validation pass is already implemented in
DropManualMemoization, so let's combine them. I opted to keep the
environment flag so we can more precisely control the rollout.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34001).
* #34022
* #34002
* __->__ #34001
Adds a new validation pass to validate against `useMemo`s that don't
return anything. This usually indicates some kind of "useEffect"-like
code that has side effects that need to be memoized to prevent
overfiring, and is an anti-pattern.
A follow up validation could also look at the return value of `useMemo`s
to see if they are being used.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33990).
* #34022
* #34002
* #34001
* __->__ #33990
* #33989
Adds a new property to ReturnTerminals to disambiguate whether it was
explicit, implicit (arrow function expressions), or void (where it was
omitted). I will use this property in the next PR adding a new
validation pass.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33989).
* #34022
* #34002
* #34001
* #33990
* __->__ #33989
The test case here previously reported a "Cannot modify local variables
after render completes" error (from
ValidateNoFreezingKnownMutableFunctions). This happens because one of
the functions passed to a hook clearly mutates a ref — except that we
try to ignore mutations of refs! The problem in this case is that the
`const ref = ...` was getting converted to a context variable since the
ref is accessed in a function before its declaration. We don't infer
types for context variables at all, and our ref handling is based on
types, so we failed to ignore this ref mutation.
The fix is to recognize that `StoreLocal const ...` is a special case:
the variable may be referenced in code before the declaration, but at
runtime it's either a TDZ error or the variable will have the type from
the declaration. So we can safely infer a type.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33993).
* __->__ #33993
* #33991
* #33984
Fixes two related cases of mutation of potentially frozen values.
The first is method calls on frozen values. Previously, we modeled
unknown function calls as potentially aliasing their receiver+args into
the return value. If the receiver or argument were known to be frozen,
then we would downgrade the `Alias` effect into an `ImmutableCapture`.
However, within a function expression it's possible to call a function
using a frozen value as an argument (that gets `Alias`-ed into the
return) but where we don't have the context locally to know that the
value is frozen.
This results in cases like this:
```js
const frozen = useContext(...);
useEffect(() => {
frozen.method().property = true;
^^^^^^^^^^^^^^^^^^^^^^^^ cannot mutate frozen value
}, [...]);
```
Within the function we would infer:
```
t0 = MethodCall ...
Create t0 = mutable
Alias t0 <- frozen
t1 = PropertyStore ...
Mutate t0
```
And then transitively infer the function expression as having a `Mutate
'frozen'` effect, which when evaluated against the outer context
(`frozen` is frozen) is an error.
The fix is to model unknown function calls as _maybe_ aliasing their
receiver/args in the return, and then considering mutations of a
maybe-aliased value to only be a conditional mutation of the source:
```
t0 = MethodCall ...
Create t0 = mutable
MaybeAlias t0 <- frozen // maybe alias now
t1 = PropertyStore ...
Mutate t0
```
Then, the `Mutate t0` turns into a `MutateConditional 'frozen'`, which
just gets ignored when we process the outer context.
The second, related fix is for known mutation of phis that may be a
frozen value. The previous inference model correctly recorded these as
errors, the new model does not. We now correctly report a validation
error for this case in the new model.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33984).
* #33993
* #33991
* __->__ #33984
This is mostly to kick off conversation, i think we should go with a
modified version of the implemented approach that i'll describe here.
The playground currently serves two roles. The primary one we think
about is for verifying compiler output. We use it for this sometimes,
and developers frequently use it for this, including to send us repros
if they have a potential bug. The second mode is to help developers
learn about React. Part of that includes learning how to use React
correctly — where it's helpful to see feedback about problematic code —
and also to understand what kind of tools we provide compared to other
frameworks, to make an informed choice about what tools they want to
use.
Currently we primarily think about the first role, but I think we should
emphasize the second more. In this PR i'm doing the worst of both:
enabling all the validations used by both the compiler and the linter by
default. This means that code that would actually compile can fail with
validations, which isn't great.
What I think we should actually do is compile twice, one in
"compilation" mode and once in "linter" mode, and combine the results as
follows:
* If "compilation" mode succeeds, show the compiled output _and_ any
linter errors.
* If "compilation" mode fails, show only the compilation mode failures.
We should also distinguish which case it is when we show errors:
"Compilation succeeded", "Compilation succeeded with linter errors",
"Compilation failed".
This lets developers continue to verify compiler output, while also
turning the playground into a much more useful tool for learning React.
Thoughts?
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33777).
* #33981
* __->__ #33777
Uses the new diagnostic infrastructure for this validation, which lets
us provide a more targeted message on the text that we highlight (eg
"This dependency may be mutated later") separately from the overall
error message.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33759).
* #33981
* #33777
* #33767
* #33765
* #33760
* __->__ #33759
* #33758
This PR uses the new diagnostic type for most of the error messages
produced in our explicit validation passes (`Validation/` directory).
One of the validations produced multiple errors as a hack to showing
multiple related locations, which we can now consolidate into a single
diagnostic.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33758).
* #33981
* #33777
* #33767
* #33765
* #33760
* #33759
* __->__ #33758
Work in progress, i'm experimenting with revamping our diagnostic infra.
Starting with a better format for representing errors, with an ability
to point ot multiple locations, along with better printing of errors. Of
course, Babel still controls the printing in the majority case so this
still needs more work.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33751).
* #33981
* #33777
* #33767
* #33765
* #33760
* #33759
* #33758
* __->__ #33751
* #33752
* #33753
When destructuring, spread creates a new mutable object that _captures_
part of the original rvalue. This new value is safe to modify.
When making this change I realized that we weren't inferring array
pattern spread as creating an array (in type inference) so I also added
that here.
## Summary
The `TSAsExpression` and `TSNonNullExpression` nodes are supported by
`lowerExpression()` but `isReorderableExpression()` does not check if
they can be reordered. This PR updates `isReorderableExpression()` to
handle these two node types by adding cases that fall through to the
existing `TypeCastExpression` case.
We ran `react-compiler-healthcheck` at scale on several of our repos and
found dozens of `` (BuildHIR::node.lowerReorderableExpression)
Expression type `TSAsExpression` cannot be safely reordered`` errors and
a handful for `TSNonNullExpression`.
## How did you test this change?
In this case I added two fixture tests
import, export, and TS namespace statements can only be used at the
top-level of a module, which is enforced by parsers already. Here we add
a backup validation of that. As of this PR, we now have only major
statement type (class declarations) listed as a todo.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33748).
* #33753
* #33752
* #33751
* #33750
* __->__ #33748
Supports inline enum declarations in both Flow and TS by treating the
node as pass-through (enums can't capture values mutably). Related, this
PR extends the set of type-related declarations that we ignore.
Previously we threw a todo for things like DeclareClass or
DeclareVariable, but these are type related and can simply be dropped
just like we dropped TypeAlias.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33747).
* #33753
* #33752
* #33751
* #33750
* #33748
* __->__ #33747
In playground it's helpful to show all errors, even those that don't
completely abort compilation. For example, to help demonstrate that the
compiler catches things like setState in effects. This detects these
errors and ensures we show them.
We currently inline IIFEs by creating a temporary and a labeled block w
the original code. The original return statements turn into an
assignment to the temporary and break out of the label. However, many
cases of IIFEs are due to inlining of manual `useMemo()`, and these
cases often have only a single return statement. Here, the output is
cleaner if we avoid the temporary and label - so that's what we do in
this PR.
Note that the most complex part of the change is actually around
ValidatePreserveExistingMemo - we have some logic to track the IIFE
temporary reassignmetns which needs to be updated to handle the simpler
version of inlining.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33726).
* __->__ #33726
* #33725
This is an optimized version of @asmjmp0's fix in
https://github.com/facebook/react/pull/31940. When we merge consecutive
blocks we need to take care to rewrite later phis whose operands will
now be different blocks due to merging. Rather than iterate all the
blocks on each merge as in #31940, we can do a single iteration over all
the phis at the end to fix them up.
Note: this is a redo of #31959
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33725).
* #33726
* __->__ #33725
