Extends the validation that effect deps are memoized to handle an additional
case that @gsathya pointed out: when a dependency has a reactive scope but that
scope ends up being pruned. We track reactive scopes which actually exist in the
ReactiveFunction, and reject useEffect deps that have an associated reactive
scope but where that scope does not exist (bc it got pruned).
This is one approach to testing whether useEffect dependencies are memoized. The
idea is based off the observation that the only reason dependencies wouldn't be
memoized (other than compiler bugs) is that they are mutated later. If they're
mutated later, then the dep array will have a mutable range which encompasses
the InstructionId of the useEffect call. So we look for that pattern and throw a
validation error.
The downside of this approach is that we might reject code that happens to be
valid: specifically, that the lack of memoization isn't a problem in practice
because the effect won't trigger a loop. But (per test plan) this doesn't seem
to introduce that many new bailouts on www. Rather than implement a complex
validation that checks whether we un-memoized something that was memoized in the
input, it seems more practical to:
1. Enable this more comprehensive validation against any form of un-memo'd
effect dependency
2. Flip the default for hooks (to assume they follow the rules), which will fix
the primary cause of Forget pessimistically not memoizing dependencies.
## Test Plan
Synced to www and checked output via the upgrade script: a few components stop
getting memoized bc they have un-memoized effect dependencies. Let's chat!
We were modifying the Babel AST as a shortcut to lowering function declarations,
instead we can explicitly lower them equivalently to a `let <id> =
<function-expression>`.
When you have your panic threshold set to "NONE" as we recommend, it's easy to
miss that your config is wrong (which makes everything not compile) because
those errors were being silenced. This made debugging FluentUI and the
forget-feedback testapp pretty difficult to figure out at first, and defeats the
purpose of having config validation in the first place.
This pr makes it so InvalidConfig errors always throw, regardless of the panic
threshold set. In general our plugin should never throw at build time due to
component bailouts, but because an InvalidConfig would bailout everything from
being compiled at all, it seems reasonable to throw here
Babel doesn't attach Comment nodes to anything, so they dangle off of
the Program node while only specifying a range. This meant that
previously we first had to traverse all of the Program's comments to
find an eslint suppression of the rules of React, then during traversal
of the individual functions, we would check if there were any *global*
eslint suppressions, then bailout all components.
This PR updates our logic to determine if individual functions are
affected by an eslint suppression range:
- If an eslint suppression range falls within its body; or
- If an eslint suppression wraps the function
---
16 out of ~150 recently added sprout fixtures have exceptions that reflect
easy-to-miss mistakes in the fixture input, like forgetting to import
`useState`.
This PR adds snapshot files for sprout to prevent these mistakes (or catch them
at diff review time). This describes what it implements, but happy to take other
suggestions as well!
1. One sprout snapshot file for each input.
This significantly increases the number of files we have, but makes it clear
what the fixture is testing. I was hoping to expand on these snapshots to record
the result of multiple re-renders, or mounts (with different parameters), but
the tradeoff is that adding / changing fixtures will now require running `yarn
sprout --mode update`.
Some alternatives I've considered:
- Only record snapshots for fixtures that error (`result.kind == "exception"`).
This change would be pretty easy. This doesn't help sanity check sprout results
for general mistakes though ("am I testing what I want to test?)
- Warn loudly for fixtures that error -- seems like these are easy to ignore,
but.. worth it for the convenience?
- Some github action that runs and comments on your PR with sprout fixture
changes; i.e. never manually update sprout files?
2. Sprout and snap share a common snapshot file, with some parsing hackery.
Previously, the implementation added new files to a separate sprout snapshot
directory, assuming that devs don't need to read these often.
After feedback from @josephsavona, I updated to have snap and sprout write to
the same file for ease of reviewing / debugging (to be able to see the input /
output side by side).
- `snap --mode update` will update snap's part of the file.
- `sprout --mode update` will update sprout's part
- `snap` and `sprout` will both compare oldSnapshot (read from the file) with
`newSnapshot = merge(oldSnapshot, newData)` to determine if a test pass.
3. Some hacks 😅 Absolute paths to project directory (e.g. stack traces) are
replaced with `<project_root>` in a mocked `console.log`
---
jsdom and other libraries seem to cause jest workers to exit with
`forceExit:true`. Not sure what option I set (or global I've overwritten) but
console logs aren't flushed as a result, making debugging a bit confusing.
This PR:
- Moves some code from `eval(...)` to a typechecked real js function. I always
had trouble debugging the `eval`ed code, so smaller code snippet is better here.
- waits for jest workers to end before exiting
I ran the plugin with the extended version of ValidateNoSetStateInRender enabled
(incl. function expressions) and there are no false positives. Let's remove the
flag for the function expression case since the whole rule is working
accurately.
Repro from T169063835. This works, but since it came up it seems good to add a
test case for it just in case there's something funny here that we could regress
on w/o realizing it.
Fixes one category of bugs with const hoisting. The algorithm finds all consts
that need to be hoisted, then looks through the statements of a block to find
the first statement which references that const, delaying the emission of the
HoistedConst instruction until its actually used. To determine if a statement
references a const we find every identifier in the statement and check if its
binding is one of the hoisted bindings.
There's a very small bug here: when we resolve the binding of each identifier,
we need to resolve it in its own scope. We're currently resolving these
identifiers agains the outer block statement's scope, which can cause us to
misattribute identifiers when there is shadowing:
```
const items = props.items.map(x => x); // we scan this statement, resolve 'x' in
the block statement scope, and mis-attribute it to the outer x.
const x = 42; // (1) this x is a candidate for hoisting, so the binding is in
the set of hoisted consts
```
I had added a repro for this earlier but hadn't realized it was due to const
hoisting. Renaming this test to clarify what's causing the problem and to make
it easier to find.
This is non ideal but at least it's a step in the right direction.
Getting the correct error requires us to track every identifier and global,
which seems a bit excessive for now.
We can revisit and improve this error if this is starting to confuse folks.
---
We were throwing `InvalidReact` errors on valid inputs.
```js
// Input
function Foo() {
log("block0");
for (const _ of foo) {
log("loop");
}
useBar();
}
// IR
bb0:
// log("block0");
ForOf init=bb2 loop=bb3 fallthrough=bb1
bb2:
// init
Branch: then:bb3 else:bb1
bb3:
// log("loop")
Goto(Continue) bb2
bb1:
// useBar();
Return
```
We correctly compute post dominators here.
```js
// In validateUnconditionalHooks
console.log(dominators.debug());
/* Output:
(read x => y as x is the post-dominator for y (all paths from x to the exit must
go through y))
"bb4" => "bb4",
"bb1" => "bb4",
"bb2" => "bb1",
"bb3" => "bb2",
"bb0" => "bb2",
*/
```
However, `findBlocksWithBackEdges` prevented us from adding `bb1` to the
`unconditionalBlocks` set as `bb2` (its post dominator) has a back edge. I'm not
sure what the `findBlocksWithBackEdges` was doing previously, so I replaced it
with an invariant asserting that the loop terminates.
---
Snap should compile all fixtures to record changes in results, even `todo`
prefixed ones. Previously, they were skipped as we noted the correlation of `//
@skip` pragmas and file naming.
Now, no fixture should be skipped as our compiler pipeline should be able to
handle every kind of error.
Noticed from our paste that we weren't correctly rolling up hoisting related
errors due to specific information being in the error title, so this PR moves
them into description instead.
Updates the approach used in ValidateNoSetStateInRender to detect function
expressions called during render. We now do the following:
* Track function expression which are known to unconditionally call setState
themselves- if these functions get called, that’s equivalent to calling
setState. We call the validation recursively to compute this.
* Track LoadLocal/StoreLocal indirections for such function expressions.
* Check CallExpressions where the callee is either a known SetState (via type
info) _or_ (new) where the callee is in the set of known-to-setState function
expressions.
The Set is shared throughout the analysis, so we can even find multiple levels
of indirection (see new test case).
I found an interesting edge case in the previous diff with mutation of a value
that appears in the expression of an object key:
```javascript
const key = {}
const object = {
[mutateAndReturnOtherValue(key)]: 42,
};
mutate(key);
```
We analyze and represent this correctly all the way through to codegen, but then
we hit the bug that @mofeiZ has noticed before: the temporary for `t =
mutateAndReturnOtherValue(key)` isn't emitted immediately (bc its a temporary).
It gets emitted inside the memo block for `object`, which is incorrect.
I tried to reproduce that here with JSX and it works as expected. It's an
interesting case though so let's land this to ensure we don't regress.
Adds a separate compiler flag for enabling the incomplete validation of ref
access within function expressions. Unlike the previous PR for
set-state-in-render validation, ref access in render can be okay in some
circumstances so i'm leaving this off by default. The point of splitting this up
is that our linting will be able to enable the rule without risk of false
positives.
The approach i initially took to validating function expressions was to try to
extend the mutable range if they are called during render, and then use the
mutable range of a function to determine if it's called during render later.
However there are cases where the range can be extended for other reasons, as
@poteto discovered, so we can't rely on the range extension. We've had several
of our validations completely off as a result of this.
In this PR i'm re-enabling @poteto's ValidateNoSetStateInRender pass by default,
but making the function expression checking use a separate compiler flag. This
means we'll have some false negatives, but should guarantee that we avoid false
positives. This means we can definitely catch things like:
```
const [state, setState] = useState(false);
setState(true);
```
Which we would have allowed by default before.
This reverts commit 10d129a8406e9d226abdb6943bf8512e34ce91db
---
Reverts #2311 due to undocumented assumptions being broken. I also added some
comments to `LoggerEvents` to explain each event type.
In `Program.ts`, we have something like the following code. `compile` could
produce any number of errors (not just expected errors / instances of
`CompilerError`). As an example, we sometimes error in `Codegen` due to babel
version incompatibilities (`Error: ObjectMethod: Too many arguments passed.
Received 7 but can receive no more than 5`).
```js
try {
// any error could be thrown here
compile(input);
} catch (e) {
// unknown type for e
handleError(e, ...);
}
```
Sathya Gunasekaran
Joe Savona
Mofei Zhang
Lauren Tan