refactor(formatter): Introduce write, format, and format_args macros
#2634
Conversation
MichaReiser
changed the title
write, format, and format_args macros
|
!bench_formatter |
Deploying with
|
| Latest commit: |
00e8740
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://638ef447.tools-8rn.pages.dev |
Formatter Benchmark Results |
|
Hi, would you mind telling me what software did you use in |
MichaReiser
force-pushed
the
refactor/format-and-write-macros
branch
from
ce4c4d1 to
0b7fc35
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
💥 Failed to Panic (3):
|
| Test result | main count |
This PR count | Difference |
|---|---|---|---|
| Total | 588 | 588 | 0 |
| Passed | 519 | 519 | 0 |
| Failed | 69 | 69 | 0 |
| Panics | 0 | 0 | 0 |
| Coverage | 88.27% | 88.27% | 0.00% |
ts/microsoft
| Test result | main count |
This PR count | Difference |
|---|---|---|---|
| Total | 16257 | 16257 | 0 |
| Passed | 12391 | 12391 | 0 |
| Failed | 3866 | 3866 | 0 |
| Panics | 0 | 0 | 0 |
| Coverage | 76.22% | 76.22% | 0.00% |
I'm using cargo instruments on mac and the built in CLion tools on Linux |
|
Interesting how this change is close to zero added lines, considering that it adds quiet a bit of documentation. |
To allow types that implement `Format` for all option types.
Good idea, I added it to the rome_js_formatter README |
There was a problem hiding this comment.
I still plan to read through this in more depth later, but I have no objection to merging it as soon as you feel there's enough consensus (to avoid having to do another tricky rebase).
Assuming people are happy with the basic idea, we can iterate later on the implementation.
Sounds good. I don't plan to merge this before Monday or Tuesday. I want to spend some time to see if I can reduce the amount of code that depends on generic type parameters |
Please let's make Monday, not Tuesday, if possible. I will need to prepare the PR for the release I would like to have some time to prep things. And I would like to not delay the release again. |
| builder, | ||
| result: Ok(()), | ||
| options: PhantomData, | ||
| unsafe { |
There was a problem hiding this comment.
nit: would it be possible to also create a builder called best_fit?
While trying this API I was in need to the following case:
let mut list = f.join_with(soft_line_break_or_space());
if something == true {
let best_fit = best_fit([]);
list.entry(best_fit);
}Unless there's a better way to do it?
There was a problem hiding this comment.
I'm not sure if I understand the example. You initalise best fit with empty?
There's no technical reason not to have it. The builder could accept the most flat version when constructing the builder and the most expanded when finishing it.
The reason I'm hesitant from adding it is that the api should encourage devs to add as little variants as possible, ideally all variants should be known at compile time which is what the macro enforces. The reason for this is that best fitting has a quadratic complexity
One way to work around this is by having multiple format_with functions that you then pass to best fitting.
However, I haven't used best fitting myself yet. So there are probably some patterns that we first need to find that work best
There was a problem hiding this comment.
Yeah I am realizing that now. I am trying to see what we can do. There are also other patterns around separated lists that I am exploring, because there are cases that we have never encountered before (in call arguments for example)
There was a problem hiding this comment.
I would love to hear about those!
|
!bench_formatter |
Formatter Benchmark Results |
…nting the list formating in the parent node.
MichaReiser
force-pushed
the
refactor/format-and-write-macros
branch
from
2fee7bf to
1196545
Compare
Summary
First of all, I'm sorry for this massive PR.
The motivation behind this PR is to change our formatting to work from left to right. Something that may become useful when formatting comments. The current formatter formats the most-grouped elements first rather than left to right, mainly because all IR elements like
group_elements,filletc. accept aFormatElementas a parameter and not aFormatimplementation. The other motivation behind this PR is to make all formatting macros allocation free compared to the currentformatted!andformat_elementsmacro that requires at least one allocation (except the compiler optimises it away).This PR enforces left to right formatting by changing Format's signature from:
to
The main change is that
formatno longer returns the formatted result but instead writes it into theFormatter, similar to Rust'sDebugandDisplaytrait with thewriteandformat_argsmacros. The fact thatformatnow writes to a sharedFormatElementbuffer enforces format rules to write the element in order or they will appear out of order in the output.Second, the PR changes all builders (
group_elements,fill,space) to return objects that implementFormatrather thanFormatElements directly. This decouples the formatting DSL from the IR used by the printer. The other added benefit is that builders that accept some inner content no longer acceptFormatElementbut instead accept any object implementingFormat. This should remove the need for manyformatted!calls that were necessary just because some helper needs aFormatElementbecause it's the least common denominator.OK, but how do I write my formatting logic now:
The main differences are
write!(f, [])instead offormattedif you want to write something to the document.format_argsif you want to pass multipleFormatobjects to a helper likegroup_elements.f.join(),f.join_with(),f.fill()andf.join_nodes_with_softline_breaketc to format a sequence of objectsPart of #2571
Drawbacks
The main drawback in my view is that it becomes harder to debug formatter code because it's no longer possible todbgprint some sub-result. This has become less of a concern in my view with the new playground but can still make it difficult to debug some formatting if things go amiss.What would be nice at least is if all built inFormatimplementations could implementDebugso that it becomes possible to writewrite!(f, [dbg!(group_elements(...))]). However, the only way I can think of doing this is by callingFormatinside ofDebugwith the default formating options which may be misleading.SOLVED: I added a
dbg_write!macro, see this commentBenchmark
Build time
Reduces the release build time of
rome_js_formatteralmost by 40% from 10s to 6.4s for a clean build.Crate Size
Reduces the
rome_formatterandrome_js_formattercrate by 287.8Kib (-40%)Before
After
Allocations
Left: Main
Right: This PR
It mainly reduces the number of small transient object (~128kb) from 200k to 80k. My interpretation is that this is because it's no longer necessary to allocate a vector for every format functions call that return more than one element.
Future Improvements
BoxaFormatElement. It should be possible to re-write our IR to a flat IR that usesStartandEndmarkers and everything in between would be the content of that element. It should then become possible to only do a single allocation for formatting the whole document which should reduce another massive amount of allocations.VecBufferinternally anymoreOpen Question
format,format_argsandwritemacros clash with Rust'sstd::fmtmacros. It's, therefore, required to have an explicit use at the top of every file and usestd::formatin places where you want to format a string. Do we have better alternative names for these macros?format, andwritehave the signatureformat![options, [...formattable]]and require the explicit[]to separate the formattable. I like the distinction to Rust's built-in macros but it often is difficult to figure out if I've added the right amount of parentheses (and rustfmt formats the arguments a little less nice). Should we remove the[]tokens?Test Plan
cargo testTODO
rome_formatterandrome_js_formatter.BestFitting