This is a start to porting HIRBuilder, with a largely complete implementation of
`build()`. Notably this includes all the passes which build() calls, and the
helper functions those passes call in turn:
```rust
reverse_postorder_blocks(&mut hir);
remove_unreachable_for_updates(&mut hir);
remove_unreachable_fallthroughs(&mut hir);
remove_unreachable_do_while_statements(&mut hir);
mark_instruction_ids(&mut hir)?;
mark_predecessors(&mut hir);
```
This was pretty straightforward. I ran into one borrow checker issue with (iirc)
`remove_unreachable_for_updates` where i needed to simultaneously hold a mutable
reference into the HIR (to write the ForTerminal) and also get an immutable
reference to check if the update block is reachable. The challenge is this:
```rust
for block in hir.blocks.values_mut() {
^^^^^^^^^^^^^^^^^^^^^^^ hir borrowed mutably here
if let TerminalValue::ForTerminal(terminal) = &mut block.terminal.value {
if let Some(update) = terminal.update {
if !hir.blocks.contains(&update) {
^^^^^^^^^^ borrowed immutably here
terminal.update = None;
^^^^^^^^ mutable borrow still active here (and also bc of the loop)
}
}
}
}
```
I quickly worked around this as we do in the other passes here by first building
a set of the block ids contained in the function (so that the
`hir.blocks.contains()` call becomes a call to the copied set of block ids).
An alternative would be to add a helper function for mutable iteration which
takes the desired value out of the data structure so that you can safely mutate
it and reference the rest of the HIR. Usage might look like this:
```rust
hir.blocks.each_mut(|mut block, hir| {
if let TerminalValue::ForTerminal(terminal) = &mut block.terminal.value {
if let Some(update) = terminal.update {
if !hir.blocks.contains(&update) {
terminal.update = None;
}
}
}
})
```
The lambda would receive the current `block` as a mutable reference, and for the
duration of the call `hir.blocks[block.id]` would be set to a sentinel value
that would crash if accessed. Meanwhile, `hir` would be a readonly reference to
the HIR, allowing the lambda to otherwise lookup information on the HIR but not
mutate it. This seems...kinda fine? But also not immediately necessary as
there's an easy and efficient-enough-workaround for the cases i've encountered
so far.
This is an initial translation of HIR (minus the ReactiveFunction bits). It's
mostly a straightforward translation. A few differences:
* Instead of Effect having an Unknown variant, we type `Place.effect:
Option<Effect>`. Maybe i'll revert that but it seems right.
* Terminal is divided into `Terminal { id: InstructionId, value: TerminalValue
}` and `enum TerminalValue { ... }`, so that we can reference the terminal's id
without caring about which kind of terminal it is.
* Each instruction value variant gets a named struct, whereas in JS we had lots
of anonymous InstructionValue variants.
* Rust generators are still an unstable nightly feature and likely to change
syntax enough that it seems safer not to rely on them. So
`eachTerminalSuccessor()` returns a `Vec<BlockId>`. If the perf of this is bad
enough we can switch to something like SmallVec (a popular crate) which stores a
few elements inline on the stack to optimize for small vecs (which our case
should be).
Perhaps more importantly, what's _not different_: I kept the existing approach
of BasicBlocks having a Vec of owned instruction instances, with
InstructionValue variant operands as Places rather than indices into a shared
instruction array. If this works out it will make porting straightforward.
However there's one aspect of this that is incorrect: right now each `Place`
owns its `Identifier`, whereas in JS the Identifier instances are shared mutable
references. I'll definitely need to refactor the data structures to allow
sharing Identifiers in some form, at which point we may want to change other
things too.
Start of an `estree` crate for representing ESTree with serialization to/from
ESTree JSON. To get the serialization to work quickly I took some shortcuts and
uses a slightly less precise modeling. Longer-term we can write some custom
serialization code and get more precise types (eg avoid the `ExpressionLike`
enum in favor of proper Expression and ExpressionOrFoo types).
lowerIdentifierForAssignment does extra checks such as checking if globals are
lvalues.
UpdateExpression lowers into an assignment and previously we missed out on such
checks.
I incorrectly included these as deps, we were only using these to verify
codegen. It seems fine to leave in but when I imported it internally vscode
would error, so just remove it
- Made most static methods on CompilerError take a single options object as an
argument. With the exception of invariant which takes a condition and an options
object. - Added a new `suggestions` field on CompilerErrorDetail, which we'll
use to provide eslint suggestions - Updated eslint-plugin-react-forget to handle
suggestions
Most of these errors were incorrectly using InvalidInput as a catchall for
rejecting code. I went through each one and manually updated them to be more
accurate
Found this while running Forget on the React tests.
This isn't a high priority because the ESLint plugin would've caught this. But
it'd be nice if either our validation rules caught this or if our compiler did
correctly eliminate the dep array.
This allows us to mimic the `invariant` api, which means you can just assert
that something holds true after execution proceeds to the next line without
throwing
These were effectively unused since we almost always passed in null when
creating errors, so remove them and instead pass in a `loc` explicitly. The
downside is that we no longer will see Babel codeframes in our test fixtures,
which doesn't seem like a huge loss
Fixes the mutable range validation in PrintHIR: when we checked the identifier
scope's range we failed to allow end=start=0 cases which are also valid.
While I was here, I created a separate assertion pass to check all ranges, that
way we aren't relying on the printer to validate them. The pass is on for
playground and tests, but disabled by default so it can't break internal apps.
Distinguishes between two types of "validation" passes:
- Passes which assert the validity of the HIR. These remain in HIR/ but are
renamed "assertFoo".
- Passes which validate that the user code is correct. These move to
Validation/.
Fixes#1751. Function ids can be plain strings, and we can refer to them as
globals rather than via a local identifier. In addition to the bug from $1751
this also cleans up an existing todo.
This is a fix specifically for the `enableOptimizeFunctionExpression` feature
(disabled by default). I tried running all of our fixtures with that flag on
everywhere, and this was the only issue. It's actually extracted from another
fixture which is more complicated, this is a distilled version. There were two
bugs:
* DCE was running after LeaveSSA when it needs to run before. Fixing the order
means code inside the function expression stops getting removed.
* In the feature flag, context variables share Identifier instances with the
surrounding code, whereas they are distinct instances without the feature. This
meant that mutable ranges from inside the function propagated outside the
function, throwing off our inference. The fix was to reset the ranges of context
variables after inferring the function expression's effects.
inference
Integrates the new inference to detect definitive mutations of context
variables. This allows us to more precisely infer mutable function expressions
and validate that they aren't passed where frozen values are expected.
New pass to infer context variables which are definitively mutated,
distinguishing from context variables which _may_ have been mutated. See the
code comment on the new pass for more details.
Small tweak necessary for the subsequent PRs, refs and ref values take
precedence over other mutations when computing the effect type of context
variables. We always want to record ref access within a function as capture
(since we have later validation) rather than a mutation. For now this has no
impact, either order records a Capture. But it allows later diffs to make
non-ref cases have other effects.
The original version of the code wasn't checking return values. I missed this
since my examples were passing functions _into_ the return value, as opposed to
return the functions directly. This revealed some existing test fixtures that
were technically invalid, but easy to fix by changing the return value.
In InferReferenceEffects, locations that are ConditionallyMutate are either
recorded as a Mutate or Read, which means we lose the distinction btw
conditional/unconditional mutation in later passes. This PR changes to remember
that these places were conditionally mutable, used in later analysis.
Defaults to false, ie it runs the codegen pass. When enabled it will simply run
all passes up to codegen and then skip over it. Naming of this option is copied
from [TypeScript](https://www.typescriptlang.org/tsconfig#noEmit) which has the
same named option that makes the compiler only perform typechecking.
I'm adding this option primarily to get around some issues running the eslint
plugin on Meta code. The plugin would error because Forget would report
duplicate Babel AST nodes, which I presume would only occur during codegen. It
should also make it a tiny bit faster to not run codegen, which is a nice plus.
Joe Savona
Sathya Gunasekaran
Lauren Tan