Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(js_formatter): provide the JS formatter with the ability to format other languages #3403

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat(js_formatter): provide the JS formatter with the ability to format other languages #3403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a27e7f6
ability to format other languages
ah-yu Jul 10, 2024
56a07f0
format with errors
ah-yu Jul 10, 2024
0a5a492
more
ah-yu Jul 10, 2024
94bd39b
tweak
ah-yu Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/biome_js_formatter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ smallvec = { workspace = true }
unicode-width = { workspace = true }

[dev-dependencies]
biome_css_formatter = { path = "../biome_css_formatter" }
biome_css_parser = { path = "../biome_css_parser" }
Comment on lines +33 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it could be a circular dependency problem at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I dont know. I'm not familiar with cargo. Does a dev dependency participate in the build phase? If there is a cycle, will cargo report it?

Copy link
Member

@ematipico ematipico Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycles aren't allowed, and rust won't compile until it's resolved.

As for dev deps, I believe that's the same because the code is compiled, even for tests.

We have options:

  • move tests inside biome_service
  • use the workspace

The first option is maybe the best one. I would also argue that embedded languages are an esoteric area and they should not be part of the testing of a specific language - in this example, the JavaScript language

Copy link
Contributor Author

@ah-yu ah-yu Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycles aren't allowed, and rust won't compile until it's resolved.

Thank you! TIL

The first option is maybe the best one

Agree

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, the more I am inclined to believe that it may be okay to have CSS dependencies in the dev dependencies for tests? It's simpler and prevents the JS formatter tests from being spread across the project. If we move it to another crate, how will we collect statistics about Prettier compatibility?

biome_formatter_test = { path = "../biome_formatter_test" }
biome_fs = { path = "../biome_fs" }
biome_js_factory = { path = "../biome_js_factory" }
Expand Down
14 changes: 13 additions & 1 deletion crates/biome_js_formatter/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub mod trailing_commas;

use crate::comments::{FormatJsLeadingComment, JsCommentStyle, JsComments};
use crate::JsForeignLanguageFormatter;
use biome_deserialize_macros::{Deserializable, Merge};
use biome_formatter::printer::PrinterOptions;
use biome_formatter::{
Expand Down Expand Up @@ -44,12 +45,19 @@ pub struct JsFormatContext {
cached_function_body: Option<(AnyJsFunctionBody, FormatElement)>,

source_map: Option<TransformSourceMap>,

foreign_language_formatter: Rc<dyn JsForeignLanguageFormatter>,
Copy link
Contributor

@denbezrukov denbezrukov Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious why Rc<dyn> is a better choice over other options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think about it deeply and just followed the comments. Any better suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also noticed the comments and couldn't find an explanation.
Unfortunately, I don't have any suggestions about it.
There are several options:

  • Have a <T> and have a constraint over T in the impls.
  • Box<dyn>?

Copy link
Contributor Author

@ah-yu ah-yu Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, you mean Rc<dyn>. I thought you were asking why use Rc instead of another smart pointer.

Initially, I wanted to use a closure, but I encountered a problem with deriving the Clone trait, so I chose to use Rc<dyn> instead. Rust is hard 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several options:

Have a and have a constraint over T in the impls.
Box?

Thank you! I will think about it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I got it. Indeed, if we need the Clone trait, we can't use Box :(
It may be fine to use Rc.

Copy link
Contributor Author

@ah-yu ah-yu Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems dyn has trade-offs.

Still need to think about it, convert this PR to a draft temporarily

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a T and have a constraint over T in the impls.

If we attach a generic type to JsFormatContext, we also need to attach a generic type to many other types associated with JsFormatContext, for example FormatJsSyntaxToken.

The rust book mentions that dyn has runtime cost but I have no idea how much the cost it is. Wonder if the cost is acceptable while keeping the code flexible

This comment was marked as off-topic.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rust book mentions that dyn has runtime cost but I have no idea how much the cost it is. Wonder if the cost is acceptable while keeping the code flexible

It looks like the overhead will be totally acceptable in this case, so I’d just stick with the Rc<dyn>. After all, it’s a single pointer to the formatter that should be used, but it shouldn’t affect the performance of the formatter itself.

Generally in Rust, don’t worry too much about performance unless you’re making clones all over the place or something is becoming noticeably slow :)

}

impl JsFormatContext {
pub fn new(options: JsFormatOptions, comments: JsComments) -> Self {
pub fn new(
options: JsFormatOptions,
foreign_language_formatter: Rc<dyn JsForeignLanguageFormatter>,
comments: JsComments,
) -> Self {
Self {
options,
foreign_language_formatter,
comments: Rc::new(comments),
cached_function_body: None,
source_map: None,
Expand Down Expand Up @@ -90,6 +98,10 @@ impl JsFormatContext {
self.source_map = source_map;
self
}

pub fn get_foreign_language_formatter(&self) -> &dyn JsForeignLanguageFormatter {
self.foreign_language_formatter.as_ref()
}
}

#[derive(Eq, PartialEq, Debug, Copy, Clone, Hash)]
Expand Down
71 changes: 62 additions & 9 deletions crates/biome_js_formatter/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ mod parentheses;
pub(crate) mod separated;
mod syntax_rewriter;

use std::rc::Rc;

use biome_formatter::format_element::tag::Label;
use biome_formatter::prelude::*;
use biome_formatter::{
Expand Down Expand Up @@ -442,13 +444,28 @@ impl IntoFormat<JsFormatContext> for JsSyntaxToken {
}
}

pub enum JsForeignLanguage {
Css,
}

pub trait JsForeignLanguageFormatter: std::fmt::Debug + 'static {
fn format(&self, language: JsForeignLanguage, source: &str) -> FormatResult<Document>;
}

#[derive(Debug, Clone)]
pub struct JsFormatLanguage {
options: JsFormatOptions,
foreign_language_formatter: Rc<dyn JsForeignLanguageFormatter>,
}
impl JsFormatLanguage {
pub fn new(options: JsFormatOptions) -> Self {
Self { options }
pub fn new(
options: JsFormatOptions,
foreign_language_formatter: impl JsForeignLanguageFormatter,
) -> Self {
Self {
options,
foreign_language_formatter: Rc::new(foreign_language_formatter),
}
}
}

Expand Down Expand Up @@ -490,7 +507,8 @@ impl FormatLanguage for JsFormatLanguage {
source_map: Option<TransformSourceMap>,
) -> Self::Context {
let comments = Comments::from_node(root, &JsCommentStyle, source_map.as_ref());
JsFormatContext::new(self.options, comments).with_source_map(source_map)
JsFormatContext::new(self.options, self.foreign_language_formatter, comments)
.with_source_map(source_map)
}
}

Expand All @@ -507,20 +525,29 @@ impl FormatLanguage for JsFormatLanguage {
/// range of the input that was effectively overwritten by the formatter
pub fn format_range(
options: JsFormatOptions,
foreign_language_formatter: impl JsForeignLanguageFormatter,
root: &JsSyntaxNode,
range: TextRange,
) -> FormatResult<Printed> {
biome_formatter::format_range(root, range, JsFormatLanguage::new(options))
biome_formatter::format_range(
root,
range,
JsFormatLanguage::new(options, foreign_language_formatter),
)
}

/// Formats a JavaScript (and its super languages) file based on its features.
///
/// It returns a [Formatted] result, which the user can use to override a file.
pub fn format_node(
options: JsFormatOptions,
foreign_language_formatter: impl JsForeignLanguageFormatter,
root: &JsSyntaxNode,
) -> FormatResult<Formatted<JsFormatContext>> {
biome_formatter::format_node(root, JsFormatLanguage::new(options))
biome_formatter::format_node(
root,
JsFormatLanguage::new(options, foreign_language_formatter),
)
}

/// Formats a single node within a file, supported by Biome.
Expand All @@ -533,8 +560,15 @@ pub fn format_node(
/// even if it's a mismatch from the rest of the block the selection is in
///
/// It returns a [Formatted] result
pub fn format_sub_tree(options: JsFormatOptions, root: &JsSyntaxNode) -> FormatResult<Printed> {
biome_formatter::format_sub_tree(root, JsFormatLanguage::new(options))
pub fn format_sub_tree(
options: JsFormatOptions,
foreign_language_formatter: impl JsForeignLanguageFormatter,
root: &JsSyntaxNode,
) -> FormatResult<Printed> {
biome_formatter::format_sub_tree(
root,
JsFormatLanguage::new(options, foreign_language_formatter),
)
}

#[derive(Copy, Clone, Debug)]
Expand All @@ -559,12 +593,25 @@ mod tests {

use super::format_range;

use crate::context::JsFormatOptions;
use biome_formatter::IndentStyle;
use crate::{context::JsFormatOptions, JsForeignLanguageFormatter};
use biome_formatter::{prelude::Document, FormatError, IndentStyle};
use biome_js_parser::{parse, parse_script, JsParserOptions};
use biome_js_syntax::JsFileSource;
use biome_rowan::{TextRange, TextSize};

#[derive(Debug, Clone)]
struct FakeFormatter;

impl JsForeignLanguageFormatter for FakeFormatter {
fn format(
&self,
_language: super::JsForeignLanguage,
_source: &str,
) -> biome_formatter::FormatResult<Document> {
Err(FormatError::SyntaxError)
}
}

#[test]
fn test_range_formatting() {
let input = "
Expand Down Expand Up @@ -600,6 +647,7 @@ while(
JsFormatOptions::new(JsFileSource::js_script())
.with_indent_style(IndentStyle::Space)
.with_indent_width(4.try_into().unwrap()),
FakeFormatter,
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand Down Expand Up @@ -634,6 +682,7 @@ function() {
JsFormatOptions::new(JsFileSource::js_script())
.with_indent_style(IndentStyle::Space)
.with_indent_width(4.try_into().unwrap()),
FakeFormatter,
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand Down Expand Up @@ -663,6 +712,7 @@ function() {
JsFormatOptions::new(JsFileSource::js_script())
.with_indent_style(IndentStyle::Space)
.with_indent_width(4.try_into().unwrap()),
FakeFormatter,
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand Down Expand Up @@ -693,6 +743,7 @@ function() {
JsFormatOptions::new(JsFileSource::js_script())
.with_indent_style(IndentStyle::Space)
.with_indent_width(4.try_into().unwrap()),
FakeFormatter,
&tree.syntax(),
range,
)
Expand Down Expand Up @@ -726,6 +777,7 @@ function() {
JsFormatOptions::new(JsFileSource::js_script())
.with_indent_style(IndentStyle::Space)
.with_indent_width(4.try_into().unwrap()),
FakeFormatter,
&tree.syntax(),
range,
)
Expand All @@ -747,6 +799,7 @@ function() {

let result = format_range(
JsFormatOptions::new(syntax),
FakeFormatter,
&tree.syntax(),
TextRange::new(TextSize::from(0), TextSize::of(src) + TextSize::from(5)),
);
Expand Down
25 changes: 21 additions & 4 deletions crates/biome_js_formatter/src/syntax_rewriter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,8 @@ fn has_type_cast_comment_or_skipped(trivia: &JsSyntaxTrivia) -> bool {
#[cfg(test)]
mod tests {
use super::JsFormatSyntaxRewriter;
use crate::{format_node, JsFormatOptions, TextRange};
use biome_formatter::{SourceMarker, TransformSourceMap};
use crate::{format_node, JsForeignLanguageFormatter, JsFormatOptions, TextRange};
use biome_formatter::{FormatError, SourceMarker, TransformSourceMap};
use biome_js_parser::{parse, parse_module, JsParserOptions};
use biome_js_syntax::{
JsArrayExpression, JsBinaryExpression, JsExpressionStatement, JsFileSource,
Expand Down Expand Up @@ -838,12 +838,29 @@ mod tests {
(transformed, source_map)
}

#[derive(Debug, Clone)]
struct FakeFormatter;

impl JsForeignLanguageFormatter for FakeFormatter {
fn format(
&self,
_language: crate::JsForeignLanguage,
_source: &str,
) -> biome_formatter::FormatResult<biome_formatter::prelude::Document> {
Err(FormatError::SyntaxError)
}
}

#[test]
fn mappings() {
let (transformed, source_map) = source_map_test("(((a * b) * c)) / 3");

let formatted =
format_node(JsFormatOptions::new(JsFileSource::default()), &transformed).unwrap();
let formatted = format_node(
JsFormatOptions::new(JsFileSource::default()),
FakeFormatter,
&transformed,
)
.unwrap();
let printed = formatted.print().unwrap();

assert_eq!(printed.as_code(), "(a * b * c) / 3;\n");
Expand Down
66 changes: 62 additions & 4 deletions crates/biome_js_formatter/tests/language.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use biome_css_formatter::context::CssFormatOptions;
use biome_css_parser::{parse_css, CssParserOptions};
use biome_formatter::FormatError;
use biome_formatter_test::TestFormatLanguage;
use biome_fs::BiomePath;
use biome_js_formatter::context::JsFormatContext;
use biome_js_formatter::JsFormatLanguage;
use biome_js_formatter::{context::JsFormatContext, JsForeignLanguageFormatter};
use biome_js_formatter::{JsForeignLanguage, JsFormatLanguage};
use biome_js_parser::{parse, JsParserOptions};
use biome_js_syntax::{JsFileSource, JsLanguage};
use biome_parser::AnyParse;
Expand All @@ -20,6 +23,32 @@ impl JsTestFormatLanguage {
}
}

#[derive(Debug, Clone)]
struct MultiLanguageFormatter {
format_with_errors: bool,
css_parse_options: CssParserOptions,
css_format_options: CssFormatOptions,
}

impl JsForeignLanguageFormatter for MultiLanguageFormatter {
fn format(
&self,
language: biome_js_formatter::JsForeignLanguage,
source: &str,
) -> biome_formatter::FormatResult<biome_formatter::prelude::Document> {
match language {
JsForeignLanguage::Css => {
let parse = parse_css(source, self.css_parse_options);
if !self.format_with_errors && parse.has_errors() {
return Err(FormatError::SyntaxError);
}
biome_css_formatter::format_node(self.css_format_options.clone(), &parse.syntax())
.map(|formatted| formatted.into_document())
}
}
}
}

impl TestFormatLanguage for JsTestFormatLanguage {
type ServiceLanguage = JsLanguage;
type Context = JsFormatContext;
Expand All @@ -37,13 +66,42 @@ impl TestFormatLanguage for JsTestFormatLanguage {
file_source: &DocumentFileSource,
) -> Self::FormatLanguage {
let language_settings = &settings.languages.javascript.formatter;
let options = Self::ServiceLanguage::resolve_format_options(
let js_formatter_options = Self::ServiceLanguage::resolve_format_options(
Some(&settings.formatter),
Some(&settings.override_settings),
Some(language_settings),
&BiomePath::new(""),
file_source,
);
JsFormatLanguage::new(options)
let css_parse_options = CssParserOptions {
css_modules: settings
.languages
.css
.parser
.css_modules
.unwrap_or_default(),
allow_wrong_line_comments: settings
.languages
.css
.parser
.allow_wrong_line_comments
.unwrap_or_default(),
grit_metavariables: true,
};
let css_format_options = CssFormatOptions::default().with_quote_style(
settings
.languages
.css
.formatter
.quote_style
.unwrap_or_default(),
);
let format_with_errors = settings.formatter.format_with_errors;
let multi_language_formatter = MultiLanguageFormatter {
css_parse_options,
css_format_options,
format_with_errors,
};
JsFormatLanguage::new(js_formatter_options, multi_language_formatter)
}
}
Loading
Loading