This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
refactor(rome_js_formatter): Move node formatting into its own trait #2476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaReiser
commented
Apr 21, 2022
| use rome_js_syntax::JsAnyArrayAssignmentPatternElement; | ||
| impl ToFormatElement for JsAnyArrayAssignmentPatternElement { | ||
| fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> { | ||
| impl Format for JsAnyArrayAssignmentPatternElement { |
There was a problem hiding this comment.
Unions don't need to implement FormatNode because the "node" logic will be handled by the inner node.
MichaReiser
commented
Apr 21, 2022
| use rome_js_syntax::JsArrayAssignmentPatternElementList; | ||
|
|
||
| impl ToFormatElement for JsArrayAssignmentPatternElementList { | ||
| fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> { | ||
| impl Format for JsArrayAssignmentPatternElementList { |
There was a problem hiding this comment.
I believe it's fine to not implement FormatNode for lists because suppressions either apply to the whole wrapper node or a specific element:
// rome-ingore
[ a, b, c] // ignores whole array expression
[
// rome-ignore
a, // Ignores first node
b,
c
]
MichaReiser
commented
Apr 21, 2022
crates/rome_js_formatter/src/lib.rs
Outdated
Comment on lines
39
to
68
| impl<T> Format for &T | ||
| where | ||
| T: Format + ?Sized, | ||
| { | ||
| fn format(&self, formatter: &Formatter) -> FormatResult<FormatElement> { | ||
| Format::format(&**self, formatter) | ||
| } | ||
| } | ||
|
|
||
| impl<T> Format for &mut T | ||
| where | ||
| T: ?Sized + Format, | ||
| { | ||
| fn format(&self, formatter: &Formatter) -> FormatResult<FormatElement> { | ||
| Format::format(&**self, formatter) | ||
| } | ||
| } | ||
|
|
||
| impl<T: ?Sized + Format> Format for Ref<'_, T> { | ||
| fn format(&self, f: &Formatter) -> FormatResult<FormatElement> { | ||
| Format::format(&**self, f) | ||
| } | ||
| } | ||
|
|
||
| impl<T: ?Sized + Format> Format for RefMut<'_, T> { | ||
| fn format(&self, f: &Formatter) -> FormatResult<FormatElement> { | ||
| Format::format(&*(self.deref()), f) | ||
| } | ||
| } |
There was a problem hiding this comment.
I plan to introduce a format_args[node1, node2] macro that accepts anything implementing Format. This implementations will be necessary for such a trait to work
Deploying with
|
| Latest commit: |
5558a65
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8acf7a7b.tools-8rn.pages.dev |
MichaReiser
force-pushed
the
refactor/format-node
branch
from
451029e
to
ce5b9c0
Compare
MichaReiser
requested review from
ematipico and
leops
and removed request for
ematipico and
leops
MichaReiser
force-pushed
the
refactor/format-node
branch
from
ce5b9c0
to
dcf87f3
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
ts/babel
ts/microsoft
|
MichaReiser
force-pushed
the
refactor/format-node
branch
from
dcf87f3
to
64d9426
Compare
MichaReiser
commented
Apr 21, 2022
Comment on lines
-239
to
-278
| impl<F: FormatTokenAndNode> FormatTokenAndNode for SyntaxResult<F> { | ||
| fn format_with<With, WithResult>( | ||
| &self, | ||
| formatter: &Formatter, | ||
| with: With, | ||
| ) -> FormatResult<FormatElement> | ||
| where | ||
| With: FnOnce(FormatElement) -> WithResult, | ||
| WithResult: IntoFormatResult, | ||
| { | ||
| match self { | ||
| Ok(token) => with(token.format(formatter)?).into_format_result(), | ||
| Err(err) => Err(err.into()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl FormatTokenAndNode for JsSyntaxToken { | ||
| fn format_with<With, WithResult>( | ||
| &self, | ||
| formatter: &Formatter, | ||
| with: With, | ||
| ) -> FormatResult<FormatElement> | ||
| where | ||
| With: FnOnce(FormatElement) -> WithResult, | ||
| WithResult: IntoFormatResult, | ||
| { | ||
| cfg_if::cfg_if! { | ||
| if #[cfg(debug_assertions)] { | ||
| assert!(formatter.printed_tokens.borrow_mut().insert(self.clone()), "You tried to print the token '{:?}' twice, and this is not valid.", self); | ||
| } | ||
| } | ||
|
|
||
| with(format_elements![ | ||
| formatter.print_leading_trivia(self, TriviaPrintMode::Full), | ||
| Token::from(self), | ||
| formatter.print_trailing_trivia(self), | ||
| ]) | ||
| .into_format_result() | ||
| } |
There was a problem hiding this comment.
Moved to Format and FormatNode
|
!bench_formatter |
Today, the formatting that must happen for each JS node happens inside `Formatter.format_syntax_node` and the `formatter_traits`. This PR moves the common logic applied by all nodes and tokens into the `ToFormatElement` implementation and renames it to `Format`. * Rename `ToFormatElement` to `Format` and `to_format_element` to `format` * Implement `Format` for `JsSyntaxToken`. CSS, JSON etc. can implement their own generic token formatting logic * Introduce a new `FormatNode` trait (language specific) that implements the formatting of a node and calls into `format_fields` that is implemented for each field. * Make the `FormatterTraits` node and language independent. Rename to `FormatTraits`. `Format`, `FormatterTraits`, `FormatResult`, and `FormatError` are now all language independent. However, some still depend on the `Formatter`. The next step is to move many of the `Formatter`'s helper function out of the Formatter so that it becomes language independent.
MichaReiser
force-pushed
the
refactor/format-node
branch
from
64d9426
to
ef1ad81
Compare
Formatter Benchmark Results |
ematipico
reviewed
Apr 21, 2022
ematipico
approved these changes
Apr 21, 2022
leops
approved these changes
Apr 21, 2022
There was a problem hiding this comment.
I still wonder if it would have been possible this trait hierarchy using only generics somehow instead of relying on codegen, but until specialization gets stabilized (and perhaps even after) I think this is probably the only way this can be expressed with the Rust type system
I would love that if you find a way. I even tried to go fancy (at least as far as I'm capable of) and played around with |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today, the formatting that must happen for each JS node happens inside
Formatter.format_syntax_nodeand theformatter_traits.This PR moves the common logic applied by all nodes and tokens into the
ToFormatElementimplementation and renames it toFormat.ToFormatElementtoFormatandto_format_elementtoformatFormatforJsSyntaxToken. CSS, JSON etc. can implement their own generic token formatting logicFormatNodetrait (language specific) that implements the formatting of a node and calls intoformat_fieldsthat is implemented for each field.FormatterTraitsnode and language independent. Rename toFormatTraits.Format,FormatterTraits,FormatResult, andFormatErrorare now all language independent. However, some still depend on theFormatter.The next step is to move many of the
Formatter's helper function out of the Formatter so that it becomes language independent.How to review
rome_js_formatter/libandformat_traitsTest Plan
cargo testDeleted a union, a list, and a node formatter implementation and regenerated the formatter files. Verified that the generated files have no compile errors.