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.

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

fix(js_formatter): Match Prettier's breaking strategy for ArrowChain layouts #934

Merged
merged 5 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
17 changes: 15 additions & 2 deletions crates/biome_formatter/src/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'buf, Context> Formatter<'buf, Context> {
/// Specialized version of [crate::Formatter::join_with] for joining SyntaxNodes separated by a space, soft
/// line break or empty line depending on the input file.
///
/// This functions inspects the input source and separates consecutive elements with either
/// This function inspects the input source and separates consecutive elements with either
/// a [crate::builders::soft_line_break_or_space] or [crate::builders::empty_line] depending on how many line breaks were
/// separating the elements in the original file.
pub fn join_nodes_with_soft_line<'a>(
Expand All @@ -126,13 +126,26 @@ impl<'buf, Context> Formatter<'buf, Context> {
/// Specialized version of [crate::Formatter::join_with] for joining SyntaxNodes separated by one or more
/// line breaks depending on the input file.
///
/// This functions inspects the input source and separates consecutive elements with either
/// This function inspects the input source and separates consecutive elements with either
/// a [crate::builders::hard_line_break] or [crate::builders::empty_line] depending on how many line breaks were separating the
/// elements in the original file.
pub fn join_nodes_with_hardline<'a>(&'a mut self) -> JoinNodesBuilder<'a, 'buf, Line, Context> {
JoinNodesBuilder::new(hard_line_break(), self)
}

/// Specialized version of [crate::Formatter::join_with] for joining SyntaxNodes separated by a simple space.
///
/// This function *disregards* the input source and always separates consecutive elements with a plain
/// [crate::builders::space], forcing a flat layout regardless of any line breaks or spaces were separating
/// the elements in the original file.
///
/// This function should likely only be used in a `best_fitting!` context, where one variant attempts to
/// force a list of nodes onto a single line without any possible breaks, then falls back to a broken
/// out variant if the content does not fit.
pub fn join_nodes_with_space<'a>(&'a mut self) -> JoinNodesBuilder<'a, 'buf, Space, Context> {
JoinNodesBuilder::new(space(), self)
}

/// Concatenates a list of [crate::Format] objects with spaces and line breaks to fit
/// them on as few lines as possible. Each element introduces a conceptual group. The printer
/// first tries to print the item in flat mode but then prints it in expanded mode if it doesn't fit.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@ use crate::prelude::*;
use crate::js::bindings::parameters::FormatAnyJsParameters;
use biome_js_syntax::JsConstructorParameters;

use super::parameters::{AnyJsParameters, FormatJsParametersOptions};

#[derive(Debug, Clone, Default)]
pub(crate) struct FormatJsConstructorParameters;

impl FormatNodeRule<JsConstructorParameters> for FormatJsConstructorParameters {
fn fmt_fields(&self, node: &JsConstructorParameters, f: &mut JsFormatter) -> FormatResult<()> {
FormatAnyJsParameters::from(node.clone()).fmt(f)
FormatAnyJsParameters::new(
AnyJsParameters::JsConstructorParameters(node.clone()),
FormatJsParametersOptions::default(),
)
.fmt(f)
}

fn fmt_dangling_comments(
Expand Down
18 changes: 14 additions & 4 deletions crates/biome_js_formatter/src/js/bindings/formal_parameter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ use biome_formatter::write;

use crate::utils::FormatInitializerClause;

use crate::js::bindings::parameters::{should_hug_function_parameters, FormatAnyJsParameters};
use biome_js_syntax::JsFormalParameter;
use crate::js::bindings::parameters::{
should_hug_function_parameters, AnyJsParameters, FormatAnyJsParameters,
FormatJsParametersOptions,
};
use biome_js_syntax::JsFormalParameterFields;
use biome_js_syntax::{JsFormalParameter, JsParameters};

#[derive(Debug, Clone, Default)]
pub(crate) struct FormatJsFormalParameter;
Expand Down Expand Up @@ -34,9 +37,16 @@ impl FormatNodeRule<JsFormalParameter> for FormatJsFormalParameter {
let is_hug_parameter = node
.syntax()
.grand_parent()
.and_then(FormatAnyJsParameters::cast)
.and_then(JsParameters::cast)
.map_or(false, |parameters| {
should_hug_function_parameters(&parameters, f.comments()).unwrap_or(false)
should_hug_function_parameters(
&FormatAnyJsParameters::new(
AnyJsParameters::JsParameters(parameters),
FormatJsParametersOptions::default(),
),
f.comments(),
)
.unwrap_or(false)
Comment on lines -37 to +49
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 might be a more Rust-y way to do this, but i'm not at all sure. it can't just be cast any more since the options field of the struct now has to be specified.

});

if is_hug_parameter && decorators.is_empty() {
Expand Down
139 changes: 109 additions & 30 deletions crates/biome_js_formatter/src/js/bindings/parameters.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::prelude::*;
use biome_formatter::{write, CstFormatContext};
use biome_formatter::{write, CstFormatContext, FormatRuleWithOptions};

use crate::js::expressions::arrow_function_expression::can_avoid_parentheses;
use crate::js::lists::parameter_list::FormatJsAnyParameterList;
Expand All @@ -8,16 +8,40 @@ use biome_js_syntax::parameter_ext::{AnyJsParameterList, AnyParameter};
use biome_js_syntax::{
AnyJsBinding, AnyJsBindingPattern, AnyJsConstructorParameter, AnyJsExpression,
AnyJsFormalParameter, AnyJsParameter, AnyTsType, JsArrowFunctionExpression,
JsConstructorParameters, JsParameters, JsSyntaxToken,
JsConstructorParameters, JsParameters, JsSyntaxNode, JsSyntaxToken,
};
use biome_rowan::{declare_node_union, SyntaxResult};
use biome_rowan::{AstNode, SyntaxResult};

#[derive(Debug, Clone, Default)]
pub(crate) struct FormatJsParameters;
#[derive(Debug, Copy, Clone, Default)]
pub(crate) struct FormatJsParameters {
options: FormatJsParametersOptions,
}

#[derive(Debug, Clone, Copy, Default)]
pub(crate) struct FormatJsParametersOptions {
/// Whether the parameters should include soft line breaks to allow them to
/// break naturally over multiple lines when they can't fit on one line.
///
/// This is particularly important for arrow chains passed as arguments in
/// call expressions, where it must be set to false to avoid having the
/// parameters break onto lines before the entire expression expands.
///
/// When `true`, parameters will _not_ include any soft line break points.
pub prevent_soft_line_breaks: bool,
}

impl FormatRuleWithOptions<JsParameters> for FormatJsParameters {
type Options = FormatJsParametersOptions;

fn with_options(mut self, options: Self::Options) -> Self {
self.options = options;
self
}
}

impl FormatNodeRule<JsParameters> for FormatJsParameters {
fn fmt_fields(&self, node: &JsParameters, f: &mut JsFormatter) -> FormatResult<()> {
FormatAnyJsParameters::from(node.clone()).fmt(f)
FormatAnyJsParameters::new(AnyJsParameters::JsParameters(node.clone()), self.options).fmt(f)
}

fn fmt_dangling_comments(&self, _: &JsParameters, _: &mut JsFormatter) -> FormatResult<()> {
Expand All @@ -26,8 +50,23 @@ impl FormatNodeRule<JsParameters> for FormatJsParameters {
}
}

declare_node_union! {
pub(crate) FormatAnyJsParameters = JsParameters | JsConstructorParameters
pub(crate) enum AnyJsParameters {
JsParameters(JsParameters),
JsConstructorParameters(JsConstructorParameters),
}
faultyserver marked this conversation as resolved.
Show resolved Hide resolved

pub(crate) struct FormatAnyJsParameters {
pub(crate) parameters: AnyJsParameters,
pub(crate) options: FormatJsParametersOptions,
}

impl FormatAnyJsParameters {
pub(crate) fn new(parameters: AnyJsParameters, options: FormatJsParametersOptions) -> Self {
Self {
parameters,
options,
}
}
Comment on lines +57 to +68
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 copied most of this from FormatTemplateElement, as another example of a formatting node that takes in options for a union of node types.

}

impl Format<JsFormatContext> for FormatAnyJsParameters {
Expand All @@ -43,6 +82,8 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {
ParameterLayout::NoParameters
} else if can_hug || self.is_in_test_call()? {
ParameterLayout::Hug
} else if self.options.prevent_soft_line_breaks {
ParameterLayout::Compact
} else {
ParameterLayout::Default
};
Expand All @@ -60,7 +101,7 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {
f,
[
l_paren_token.format(),
format_dangling_comments(self.syntax()).with_soft_block_indent(),
format_dangling_comments(self.inner_syntax()).with_soft_block_indent(),
r_paren_token.format()
]
)
Expand Down Expand Up @@ -99,7 +140,7 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {
f,
[soft_block_indent(&FormatJsAnyParameterList::with_layout(
&list,
ParameterLayout::Default
ParameterLayout::Default,
))]
)?;

Expand All @@ -109,6 +150,29 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {
write!(f, [format_removed(&r_paren_token)])?;
}

Ok(())
}
ParameterLayout::Compact => {
if !parentheses_not_needed {
write!(f, [l_paren_token.format()])?;
} else {
write!(f, [format_removed(&l_paren_token)])?;
}
Comment on lines +155 to +159
Copy link
Contributor Author

@faultyserver faultyserver Nov 28, 2023

Choose a reason for hiding this comment

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

Seems like we could probably move these paren token writes outside of the match and reduce duplication, but not gonna do that for now.


write!(
f,
[FormatJsAnyParameterList::with_layout(
&list,
ParameterLayout::Compact
)]
)?;

if !parentheses_not_needed {
write!(f, [r_paren_token.format()])?;
} else {
write!(f, [format_removed(&r_paren_token)])?;
}

Ok(())
}
}
Expand All @@ -117,54 +181,57 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {

impl FormatAnyJsParameters {
fn l_paren_token(&self) -> SyntaxResult<JsSyntaxToken> {
match self {
FormatAnyJsParameters::JsParameters(parameters) => parameters.l_paren_token(),
FormatAnyJsParameters::JsConstructorParameters(parameters) => {
parameters.l_paren_token()
}
match &self.parameters {
AnyJsParameters::JsParameters(parameters) => parameters.l_paren_token(),
AnyJsParameters::JsConstructorParameters(parameters) => parameters.l_paren_token(),
}
}

fn list(&self) -> AnyJsParameterList {
match self {
FormatAnyJsParameters::JsParameters(parameters) => {
match &self.parameters {
AnyJsParameters::JsParameters(parameters) => {
AnyJsParameterList::from(parameters.items())
}
FormatAnyJsParameters::JsConstructorParameters(parameters) => {
AnyJsParameters::JsConstructorParameters(parameters) => {
AnyJsParameterList::from(parameters.parameters())
}
}
}

fn r_paren_token(&self) -> SyntaxResult<JsSyntaxToken> {
match self {
FormatAnyJsParameters::JsParameters(parameters) => parameters.r_paren_token(),
FormatAnyJsParameters::JsConstructorParameters(parameters) => {
parameters.r_paren_token()
}
match &self.parameters {
AnyJsParameters::JsParameters(parameters) => parameters.r_paren_token(),
AnyJsParameters::JsConstructorParameters(parameters) => parameters.r_paren_token(),
}
}

/// Returns `true` for function parameters if the function is an argument of a [test `CallExpression`](is_test_call_expression).
fn is_in_test_call(&self) -> SyntaxResult<bool> {
let result = match self {
FormatAnyJsParameters::JsParameters(parameters) => match parameters.syntax().parent() {
let result = match &self.parameters {
AnyJsParameters::JsParameters(parameters) => match parameters.syntax().parent() {
Some(function) => is_test_call_argument(&function)?,
None => false,
},
FormatAnyJsParameters::JsConstructorParameters(_) => false,
AnyJsParameters::JsConstructorParameters(_) => false,
};

Ok(result)
}

fn as_arrow_function_expression(&self) -> Option<JsArrowFunctionExpression> {
match self {
FormatAnyJsParameters::JsParameters(parameters) => parameters
match &self.parameters {
AnyJsParameters::JsParameters(parameters) => parameters
.syntax()
.parent()
.and_then(JsArrowFunctionExpression::cast),
FormatAnyJsParameters::JsConstructorParameters(_) => None,
AnyJsParameters::JsConstructorParameters(_) => None,
}
}

fn inner_syntax(&self) -> &JsSyntaxNode {
faultyserver marked this conversation as resolved.
Show resolved Hide resolved
match &self.parameters {
AnyJsParameters::JsParameters(parameters) => parameters.syntax(),
AnyJsParameters::JsConstructorParameters(parameters) => parameters.syntax(),
}
}
}
Expand All @@ -190,7 +257,8 @@ pub enum ParameterLayout {
Hug,

/// The default layout formats all parameters on the same line if they fit or breaks after the `(`
/// and before the `(`.
/// and before the `)`.
///
/// ```javascript
/// function test(
/// firstParameter,
Expand All @@ -199,6 +267,17 @@ pub enum ParameterLayout {
/// ) {}
/// ```
Default,

/// Compact layout forces all parameters to try to render on the same line,
/// with no breaks added around the brackets. This should likely only be
/// used in a `best_fitting!` context where one variant attempts to fit the
/// parameters on a single line, and a default expanded version that is
/// used in case that does not fit.
///
/// ```javascript
/// function test(firstParameter, secondParameter, thirdParameter, evenOverlyLong) {}
/// ```
Compact,
}

pub(crate) fn should_hug_function_parameters(
Expand Down
Loading