I refactored the code in `tasks/prettier_conformance` primarily to make
the output more readable when using `--filter`.
But I also discovered that our previous implementation did not correctly
handle Prettier's behavior of adding a blank line at the EOF.
In addition, I resolved a problem where test specs that used patterns
like `runFormatTest(_, parsers)` were unable to locate the correct
snapshot output.
As a result, compatibility has also improved slightly. 😉
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Follow-on after #8024.
Move code out of `visit_expression`.
`visit_expression` is marked `#[inline]`, which is a good idea because it's a hot path and most of the time the `Expression` is not `super()` and the function does nothing.
But this means we want `visit_expression` to be as small as possible to reduce the cost of inlining it. So only retain the fast path for "Is it `super()`? No it's not, so just continue traversal." and move everything else out into a separate function.
Follow-on after #8024. Don't inline `visit_statements` visitor method. This visitor is not so small, and always executes unconditionally. The compiler can decide whether to inline it or not. Where `#[inline]` is really valuable is:
1. Where the function is tiny so the cost of moving it into the caller is small. and
2. The function gets called a lot e.g. `visit_expression`, `visit_statement`, `visit_identifier_reference`.
3. Most commonly the function checks something and exits "nothing to do here". e.g. `visit_expression` where you only need to transform a particular type of `Expression`.
Follow-on after #8024. Pure refactor.
Re-order the methods of the `ConstructorBodyThisAfterSuperInserter` visitor. Intent is that the call path flows from top to bottom, so the "story" makes sense when you read the code starting from the top.
Follow-on after #8024.
Use `NonEmptyStack` instead of `Stack`.
`NonEmptyStack` is cheaper as `last` / `last_mut` always returns `T`, rather than `Option<T>`, so there's no need to branch on whether it's `Some` or not.
The only downside of `NonEmptyStack` is that it always contains 1 dummy entry at the start (so that it's never empty). This means that it always allocates, unlike `Stack` which doesn't allocate unless you push something into it.
So `NonEmptyStack` is always the better choice unless either:
1. The stack will likely never have anything pushed to it. or
2. The type the stack holds is large or expensive to construct, so there's a high cost in having to create an initial dummy value.
In this case:
* The type is `bool` - very small and cheap to construct.
* The stack will be pushed to when entering a function. Very few JS files contain no functions at all, so the stack will always need to allocate, even if we used `Stack`.
So this case doesn't satisfy either of the circumstances in which `Stack` is the better choice.
It's an invariant of `Source` that only a single instance of `Source` can exist at any time. `Source` was erroneously implementing `Clone`, which makes it trivial to break this invariant. `Clone` is unnecessary, so remove it.
Always use `SourcePosition<'a>` not `SourcePosition`, to ensure not reading from a `Source` with a `SourcePosition` which is from *another* `Source`. This doesn't definitively guard against UB - that is the job of `UniquePromise` - but it adds another level of defence.
`AstBuilder` method names have an `_` added on end if method name is not a valid identifier (e.g. `super`). But no need for trailing underscore on `alloc_super`.
Currently this only applies to `super`. But future-proof by checking against all Rust's reserved words.
This is a breaking change, because `alloc_super` method was previously called `alloc_super_`. But probably no-one uses that method anyway - usually you'd use `expression_super` method to get an `Expression::Super`.
Pure refactor. `AstBuilder` methods can use `alloc_*` methods directly, instead of calling `alloc`. Probably this makes no difference to performance as all these methods are marked `#[inline]` anyway. But in general it's better to use methods which don't return large objects on the stack.
All node creation is meant to go via `AstBuilder`. This is necessary to support Node IDs. Remove this method which breaks that rule. It's not used anyway.
Pure refactor. Our convention in transformer is to pass `TraverseCtx` around, and not deal with `AstBuilder` directly. This will make it easier to make changes to `AstBuilder` to support Node IDs later on.
Pure refactor. Enable `clippy::needless_lifetimes` lint rule in `oxc_transformer`, and fix what the rule flags - remove unnecessary lifetimes which can be elided.
Also rename a few lifetimes `'b` and `'c` to more descriptive `'ctx`.
