---
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, ...);
}
```
I experimented with more variations but prettier collapses most of them: any run
of consecutive whitespace within a jsxtext node will get collapsed into a single
space, for example. So the main difference in practice is whether a jsxtext node
(preceding a value) has a trailing space/newline or not:
```
Text <fbt:param /> Text
// vs
Text<fbt:param />Text
```
which we now have tests for.
Turns out this will cause an OOM in node.js when running eslint as part of the
IDE
``` Before: 16M packages/eslint-plugin-react-forget/dist/index.js After: 2.2M
packages/eslint-plugin-react-forget/dist/index.js ```
Anytime we have a nested `.scope` in code my brain hurts. For example
`scope.scope.dependencies`. This PR updates the scope merging pass to use the
name `scopeBlock` for a ReactiveScopeBlock and `scope` only for ReactiveScope
values, to make things a bit more clear.
Addresses T168684688 (#2242). MergeReactiveScopesThatInvalidateTogether does not
merge scopes if their output is not guaranteed to change when their inputs do.
So for example a case such as `{session_id: bar(props.bar)}` will not merge the
scopes for `t0 = bar(props.bar)` and `t1 = {session_id: t0}`, because t0 isn't
guaranteed to change when `props.bar` does, and we want to avoid recreating the
t1 object unless it semantically changes.
But there's a special case: if a scope has no dependencies, then we'll never
execute it again anyway. So it doesn't matter what kind of value it produces and
it's safe to merge with subsequent scopes:
```javascript
return {session_id: bar()}
```
Without the reactive input, `bar()` will always return the same value since
we'll only ever call it once anyway. So it's safe to then merge with the scope
for the outer object literal.
Current sprout output (if you remove the line in `SproutTodoFilter`):
```
Failures:
FAIL: bug-fbt-preserve-whitespace
Difference in forget and non-forget results.
Expected result: {
"kind": "ok",
"value": "Before text hello world",
"logs": []
}
Found: {
"kind": "ok",
"value": "Before texthello world",
"logs": []
}
```
`fbt` transforms run before jsx ones, so our lowering should also account for
[fbt's whitespace
rules](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtImplicitParamNode.js#L230-L233)
in `BuildHIR:trimJsxText`.
Fbt + typescript [seems](https://github.com/facebook/fbt/issues/49) [to
be](https://github.com/facebook/sfbt/issues/72) a non-blessed workflow. We do
want to allow for both flow and typescript tests, so I followed some
instructions [from a
guide](https://dev.to/retyui/how-to-add-support-typescript-for-fbt-an-internationalization-framework-3lo0)
to add support.
- fbt tags desugar to.. "fbt" strings. By default, typescript removes unused
imports at parse step (and on autoformat steps). We pass special
babel-typescript configs and change vscode settings to mitigate this.
- I tried adding `fbt` to the global scope, but the fbt transform asserts that
`fbt` is actually imported in the source program.
- Other hacks are available, like saying we'll only allow for fbt in flow files,
or always patching the source code to have an "fbt" import. This seemed the most
reasonable and easiest to debug / follow when writing tests
---
Refactor selection logic to be easier to read; add support for .jsx test files
(feels a bit weird adding a `.jsx` fixture and not seeing it get run)
Mofei Zhang
Joe Savona
Sathya Gunasekaran
Lauren Tan