From 21a7f64855ee8ff61d584f835c8775ab6546fdc7 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Sun, 3 Dec 2023 22:11:51 +0000 Subject: [PATCH] fix(js_formatter): Remove conditional expanded content in RemoveSoftLinesBuffer --- crates/biome_formatter/src/buffer.rs | 57 +++++++- .../src/js/bindings/constructor_parameters.rs | 8 +- .../src/js/bindings/formal_parameter.rs | 18 +-- .../src/js/bindings/parameters.rs | 128 ++++-------------- .../expressions/arrow_function_expression.rs | 11 +- .../src/js/lists/parameter_list.rs | 46 +++---- .../src/utils/object_pattern_like.rs | 18 +-- crates/biome_js_formatter/tests/quick_test.rs | 28 ++-- .../tests/specs/js/module/call_expression.js | 8 +- .../specs/js/module/call_expression.js.snap | 14 +- 10 files changed, 142 insertions(+), 194 deletions(-) diff --git a/crates/biome_formatter/src/buffer.rs b/crates/biome_formatter/src/buffer.rs index 544ac34e9daf..ff9dcda38d79 100644 --- a/crates/biome_formatter/src/buffer.rs +++ b/crates/biome_formatter/src/buffer.rs @@ -1,6 +1,6 @@ use super::{write, Arguments, FormatElement}; use crate::format_element::Interned; -use crate::prelude::LineMode; +use crate::prelude::{LineMode, PrintMode, Tag}; use crate::{Format, FormatResult, FormatState}; use rustc_hash::FxHashMap; use std::any::{Any, TypeId}; @@ -487,6 +487,11 @@ pub struct RemoveSoftLinesBuffer<'a, Context> { /// It's fine to not snapshot the cache. The worst that can happen is that it holds on interned elements /// that are now unused. But there's little harm in that and the cache is cleaned when dropping the buffer. interned_cache: FxHashMap, + + /// Marker for whether a `StartConditionalContent(mode: Expanded)` has been + /// written but not yet closed. Expanded content gets removed from this + /// buffer, so anything written while in this state simply gets dropped. + is_in_expanded_conditional_content: bool, } impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> { @@ -495,6 +500,7 @@ impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> { Self { inner, interned_cache: FxHashMap::default(), + is_in_expanded_conditional_content: false, } } @@ -512,7 +518,8 @@ fn clean_interned( match interned_cache.get(interned) { Some(cleaned) => cleaned.clone(), None => { - // Find the first soft line break element or interned element that must be changed + // Find the first soft line break element, interned element, or conditional expanded + // content that must be changed. let result = interned .iter() .enumerate() @@ -522,6 +529,13 @@ fn clean_interned( cleaned.extend_from_slice(&interned[..index]); Some((cleaned, &interned[index..])) } + FormatElement::Tag(Tag::StartConditionalContent(condition)) + if condition.mode == PrintMode::Expanded => + { + let mut cleaned = Vec::new(); + cleaned.extend_from_slice(&interned[..index]); + Some((cleaned, &interned[index..])) + } FormatElement::Interned(inner) => { let cleaned_inner = clean_interned(inner, interned_cache); @@ -541,10 +555,32 @@ fn clean_interned( let result = match result { // Copy the whole interned buffer so that becomes possible to change the necessary elements. Some((mut cleaned, rest)) => { + let mut is_in_expanded_conditional_content = false; for element in rest { + if is_in_expanded_conditional_content { + continue; + } + let element = match element { + FormatElement::Tag(Tag::StartConditionalContent(condition)) + if condition.mode == PrintMode::Expanded => + { + is_in_expanded_conditional_content = true; + continue; + } + FormatElement::Tag(Tag::EndConditionalContent) + if is_in_expanded_conditional_content => + { + is_in_expanded_conditional_content = false; + continue; + } + // All content within an expanded conditional gets dropped. If there's a + // matching flat variant, that will still get kept. + _ if is_in_expanded_conditional_content => continue, + FormatElement::Line(LineMode::Soft) => continue, FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, + FormatElement::Interned(interned) => { FormatElement::Interned(clean_interned(interned, interned_cache)) } @@ -570,8 +606,25 @@ impl Buffer for RemoveSoftLinesBuffer<'_, Context> { fn write_element(&mut self, element: FormatElement) -> FormatResult<()> { let element = match element { + FormatElement::Tag(Tag::StartConditionalContent(condition)) + if condition.mode == PrintMode::Expanded => + { + self.is_in_expanded_conditional_content = true; + return Ok(()); + } + FormatElement::Tag(Tag::EndConditionalContent) + if self.is_in_expanded_conditional_content => + { + self.is_in_expanded_conditional_content = false; + return Ok(()); + } + // All content within an expanded conditional gets dropped. If there's a + // matching flat variant, that will still get kept. + _ if self.is_in_expanded_conditional_content => return Ok(()), + FormatElement::Line(LineMode::Soft) => return Ok(()), FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, + FormatElement::Interned(interned) => { FormatElement::Interned(self.clean_interned(&interned)) } diff --git a/crates/biome_js_formatter/src/js/bindings/constructor_parameters.rs b/crates/biome_js_formatter/src/js/bindings/constructor_parameters.rs index d4e4d1994611..8bf4dbe9e5d1 100644 --- a/crates/biome_js_formatter/src/js/bindings/constructor_parameters.rs +++ b/crates/biome_js_formatter/src/js/bindings/constructor_parameters.rs @@ -3,18 +3,12 @@ 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 for FormatJsConstructorParameters { fn fmt_fields(&self, node: &JsConstructorParameters, f: &mut JsFormatter) -> FormatResult<()> { - FormatAnyJsParameters::new( - AnyJsParameters::JsConstructorParameters(node.clone()), - FormatJsParametersOptions::default(), - ) - .fmt(f) + FormatAnyJsParameters::from(node.clone()).fmt(f) } fn fmt_dangling_comments( diff --git a/crates/biome_js_formatter/src/js/bindings/formal_parameter.rs b/crates/biome_js_formatter/src/js/bindings/formal_parameter.rs index 0df8765c6f64..e873da1ee4c8 100644 --- a/crates/biome_js_formatter/src/js/bindings/formal_parameter.rs +++ b/crates/biome_js_formatter/src/js/bindings/formal_parameter.rs @@ -3,12 +3,9 @@ use biome_formatter::write; use crate::utils::FormatInitializerClause; -use crate::js::bindings::parameters::{ - should_hug_function_parameters, AnyJsParameters, FormatAnyJsParameters, - FormatJsParametersOptions, -}; +use crate::js::bindings::parameters::{should_hug_function_parameters, FormatAnyJsParameters}; +use biome_js_syntax::JsFormalParameter; use biome_js_syntax::JsFormalParameterFields; -use biome_js_syntax::{JsFormalParameter, JsParameters}; #[derive(Debug, Clone, Default)] pub(crate) struct FormatJsFormalParameter; @@ -37,16 +34,9 @@ impl FormatNodeRule for FormatJsFormalParameter { let is_hug_parameter = node .syntax() .grand_parent() - .and_then(JsParameters::cast) + .and_then(FormatAnyJsParameters::cast) .map_or(false, |parameters| { - should_hug_function_parameters( - &FormatAnyJsParameters::new( - AnyJsParameters::JsParameters(parameters), - FormatJsParametersOptions::default(), - ), - f.comments(), - ) - .unwrap_or(false) + should_hug_function_parameters(¶meters, f.comments()).unwrap_or(false) }); if is_hug_parameter && decorators.is_empty() { diff --git a/crates/biome_js_formatter/src/js/bindings/parameters.rs b/crates/biome_js_formatter/src/js/bindings/parameters.rs index b60a0c357252..984b9257132d 100644 --- a/crates/biome_js_formatter/src/js/bindings/parameters.rs +++ b/crates/biome_js_formatter/src/js/bindings/parameters.rs @@ -1,5 +1,5 @@ use crate::prelude::*; -use biome_formatter::{write, CstFormatContext, FormatRuleWithOptions}; +use biome_formatter::{write, CstFormatContext}; use crate::js::expressions::arrow_function_expression::can_avoid_parentheses; use crate::js::lists::parameter_list::FormatJsAnyParameterList; @@ -13,59 +13,21 @@ use biome_js_syntax::{ use biome_rowan::{declare_node_union, AstNode, SyntaxResult}; #[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 for FormatJsParameters { - type Options = FormatJsParametersOptions; - - fn with_options(mut self, options: Self::Options) -> Self { - self.options = options; - self - } -} +pub(crate) struct FormatJsParameters(); impl FormatNodeRule for FormatJsParameters { fn fmt_fields(&self, node: &JsParameters, f: &mut JsFormatter) -> FormatResult<()> { - FormatAnyJsParameters::new(AnyJsParameters::JsParameters(node.clone()), self.options).fmt(f) + FormatAnyJsParameters::from(node.clone()).fmt(f) } fn fmt_dangling_comments(&self, _: &JsParameters, _: &mut JsFormatter) -> FormatResult<()> { - // Formatted inside of `FormatJsAnyParameters + // Formatted inside of `FormatJsAnyParameters` Ok(()) } } declare_node_union! { - pub(crate) AnyJsParameters = JsParameters | JsConstructorParameters -} - -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, - } - } + pub(crate) FormatAnyJsParameters = JsParameters | JsConstructorParameters } impl Format for FormatAnyJsParameters { @@ -81,8 +43,6 @@ impl Format 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 }; @@ -100,7 +60,7 @@ impl Format for FormatAnyJsParameters { f, [ l_paren_token.format(), - format_dangling_comments(self.parameters_syntax()).with_soft_block_indent(), + format_dangling_comments(self.syntax()).with_soft_block_indent(), r_paren_token.format() ] ) @@ -149,29 +109,6 @@ impl Format 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)])?; - } - - 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(()) } } @@ -180,57 +117,61 @@ impl Format for FormatAnyJsParameters { impl FormatAnyJsParameters { fn l_paren_token(&self) -> SyntaxResult { - match &self.parameters { - AnyJsParameters::JsParameters(parameters) => parameters.l_paren_token(), - AnyJsParameters::JsConstructorParameters(parameters) => parameters.l_paren_token(), + match self { + FormatAnyJsParameters::JsParameters(parameters) => parameters.l_paren_token(), + FormatAnyJsParameters::JsConstructorParameters(parameters) => { + parameters.l_paren_token() + } } } fn list(&self) -> AnyJsParameterList { - match &self.parameters { - AnyJsParameters::JsParameters(parameters) => { + match self { + FormatAnyJsParameters::JsParameters(parameters) => { AnyJsParameterList::from(parameters.items()) } - AnyJsParameters::JsConstructorParameters(parameters) => { + FormatAnyJsParameters::JsConstructorParameters(parameters) => { AnyJsParameterList::from(parameters.parameters()) } } } fn r_paren_token(&self) -> SyntaxResult { - match &self.parameters { - AnyJsParameters::JsParameters(parameters) => parameters.r_paren_token(), - AnyJsParameters::JsConstructorParameters(parameters) => parameters.r_paren_token(), + match self { + FormatAnyJsParameters::JsParameters(parameters) => parameters.r_paren_token(), + FormatAnyJsParameters::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 { - let result = match &self.parameters { - AnyJsParameters::JsParameters(parameters) => match parameters.syntax().parent() { + let result = match self { + FormatAnyJsParameters::JsParameters(parameters) => match parameters.syntax().parent() { Some(function) => is_test_call_argument(&function)?, None => false, }, - AnyJsParameters::JsConstructorParameters(_) => false, + FormatAnyJsParameters::JsConstructorParameters(_) => false, }; Ok(result) } fn as_arrow_function_expression(&self) -> Option { - match &self.parameters { - AnyJsParameters::JsParameters(parameters) => parameters + match self { + FormatAnyJsParameters::JsParameters(parameters) => parameters .syntax() .parent() .and_then(JsArrowFunctionExpression::cast), - AnyJsParameters::JsConstructorParameters(_) => None, + FormatAnyJsParameters::JsConstructorParameters(_) => None, } } - fn parameters_syntax(&self) -> &JsSyntaxNode { - match &self.parameters { - AnyJsParameters::JsParameters(parameters) => parameters.syntax(), - AnyJsParameters::JsConstructorParameters(parameters) => parameters.syntax(), + fn syntax(&self) -> &JsSyntaxNode { + match self { + FormatAnyJsParameters::JsParameters(parameters) => parameters.syntax(), + FormatAnyJsParameters::JsConstructorParameters(parameters) => parameters.syntax(), } } } @@ -266,17 +207,6 @@ 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( diff --git a/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs b/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs index 099cbe955838..b27f38552cd1 100644 --- a/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs +++ b/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs @@ -1,4 +1,4 @@ -use crate::js::bindings::parameters::{has_only_simple_parameters, FormatJsParametersOptions}; +use crate::js::bindings::parameters::has_only_simple_parameters; use crate::prelude::*; use biome_formatter::{ format_args, write, CstFormatContext, FormatRuleWithOptions, RemoveSoftLinesBuffer, @@ -255,15 +255,10 @@ fn format_signature( } } AnyJsArrowFunctionParameters::JsParameters(params) => { - let formatted_params = - params.format().with_options(FormatJsParametersOptions { - prevent_soft_line_breaks: is_first_or_last_call_argument, - }); - if ancestor_call_expr_or_logical_expr && !is_first_or_last_call_argument { - write!(f, [group(&dedent(&formatted_params))])?; + write!(f, [group(&dedent(¶ms.format()))])?; } else { - write!(f, [formatted_params])?; + write!(f, [params.format()])?; } } }; diff --git a/crates/biome_js_formatter/src/js/lists/parameter_list.rs b/crates/biome_js_formatter/src/js/lists/parameter_list.rs index aac57e427273..1ee731fd6494 100644 --- a/crates/biome_js_formatter/src/js/lists/parameter_list.rs +++ b/crates/biome_js_formatter/src/js/lists/parameter_list.rs @@ -38,11 +38,7 @@ impl<'a> FormatJsAnyParameterList<'a> { impl Format for FormatJsAnyParameterList<'_> { fn fmt(&self, f: &mut Formatter) -> FormatResult<()> { match self.layout { - None - | Some( - ParameterLayout::Default | ParameterLayout::Compact | ParameterLayout::NoParameters, - ) => { - let is_compact = matches!(self.layout, Some(ParameterLayout::Compact)); + None | Some(ParameterLayout::Default | ParameterLayout::NoParameters) => { let has_trailing_rest = match self.list.last() { Some(elem) => matches!( elem?, @@ -54,40 +50,30 @@ impl Format for FormatJsAnyParameterList<'_> { None => false, }; - // If the list is being printed compactly, then the closing - // bracket _must_ appear immediately next to the last element, - // so a trailing separator would appear incorrect. - // // If it's a rest parameter, the assumption is no more // parameters could be added afterward, so no separator is // added there either. - let trailing_separator = if is_compact || has_trailing_rest { + let trailing_separator = if has_trailing_rest { TrailingSeparator::Disallowed } else { FormatTrailingComma::All.trailing_separator(f.options()) }; - if is_compact { - let mut joiner = f.join_nodes_with_space(); - join_parameter_list(&mut joiner, self.list, trailing_separator)?; - joiner.finish() + let has_modifiers = self.list.iter().any(|node| { + matches!( + node, + Ok(AnyParameter::AnyJsConstructorParameter( + AnyJsConstructorParameter::TsPropertyParameter(_), + )) + ) + }); + let mut joiner = if has_modifiers { + f.join_nodes_with_hardline() } else { - let has_modifiers = self.list.iter().any(|node| { - matches!( - node, - Ok(AnyParameter::AnyJsConstructorParameter( - AnyJsConstructorParameter::TsPropertyParameter(_), - )) - ) - }); - let mut joiner = if has_modifiers { - f.join_nodes_with_hardline() - } else { - f.join_nodes_with_soft_line() - }; - join_parameter_list(&mut joiner, self.list, trailing_separator)?; - joiner.finish() - } + f.join_nodes_with_soft_line() + }; + join_parameter_list(&mut joiner, self.list, trailing_separator)?; + joiner.finish() } Some(ParameterLayout::Hug) => { let mut join = f.join_with(space()); diff --git a/crates/biome_js_formatter/src/utils/object_pattern_like.rs b/crates/biome_js_formatter/src/utils/object_pattern_like.rs index ba1b5f6d6b90..e6647303bc7a 100644 --- a/crates/biome_js_formatter/src/utils/object_pattern_like.rs +++ b/crates/biome_js_formatter/src/utils/object_pattern_like.rs @@ -1,7 +1,4 @@ -use crate::js::bindings::parameters::{ - should_hug_function_parameters, AnyJsParameters, FormatAnyJsParameters, - FormatJsParametersOptions, -}; +use crate::js::bindings::parameters::{should_hug_function_parameters, FormatAnyJsParameters}; use crate::prelude::*; use crate::JsFormatContext; use biome_formatter::formatter::Formatter; @@ -10,7 +7,7 @@ use biome_formatter::{Format, FormatResult}; use biome_js_syntax::{ AnyJsAssignmentPattern, AnyJsBindingPattern, AnyJsFormalParameter, AnyJsObjectAssignmentPatternMember, AnyJsObjectBindingPatternMember, JsObjectAssignmentPattern, - JsObjectBindingPattern, JsParameters, JsSyntaxKind, JsSyntaxToken, + JsObjectBindingPattern, JsSyntaxKind, JsSyntaxToken, }; use biome_rowan::{declare_node_union, AstNode, SyntaxNodeOptionExt, SyntaxResult}; @@ -132,16 +129,9 @@ impl JsObjectPatternLike { JsObjectPatternLike::JsObjectBindingPattern(binding) => binding .parent::() .and_then(|parameter| parameter.syntax().grand_parent()) - .and_then(JsParameters::cast) + .and_then(FormatAnyJsParameters::cast) .map_or(false, |parameters| { - should_hug_function_parameters( - &FormatAnyJsParameters::new( - AnyJsParameters::JsParameters(parameters), - FormatJsParametersOptions::default(), - ), - comments, - ) - .unwrap_or(false) + should_hug_function_parameters(¶meters, comments).unwrap_or(false) }), } } diff --git a/crates/biome_js_formatter/tests/quick_test.rs b/crates/biome_js_formatter/tests/quick_test.rs index e71fdc0ab672..38483afefdef 100644 --- a/crates/biome_js_formatter/tests/quick_test.rs +++ b/crates/biome_js_formatter/tests/quick_test.rs @@ -1,4 +1,4 @@ -use biome_formatter::IndentStyle; +use biome_formatter::{IndentStyle, LineWidth}; use biome_formatter_test::check_reformat::CheckReformat; use biome_js_formatter::context::{ArrowParentheses, JsFormatOptions, QuoteStyle, Semicolons}; use biome_js_formatter::format_node; @@ -9,32 +9,25 @@ mod language { include!("language.rs"); } +#[ignore] #[test] // use this test check if your snippet prints as you wish, without using a snapshot fn quick_test() { let src = r#" - -const makeSomeFunction = -(services = {logger:null}) => - (a, b, c) => - services.logger(a,b,c) - -const makeSomeFunction2 = -(services = { - logger: null -}) => - (a, b, c) => - services.logger(a, b, c) - + aLongFunctionName(({ a, eventHeff }: ReallyLongNameForATypeThingHereOptionNameApplicationCommandOptionKeepsGoingOnn) => { + const a = 1; + } + ); "#; - let syntax = JsFileSource::tsx(); + let source_type = JsFileSource::tsx(); let tree = parse( src, - syntax, + source_type, JsParserOptions::default().with_parse_class_parameter_decorators(), ); - let options = JsFormatOptions::new(syntax) + let options = JsFormatOptions::new(source_type) .with_indent_style(IndentStyle::Space) + .with_line_width(LineWidth::try_from(120).unwrap()) .with_semicolons(Semicolons::Always) .with_quote_style(QuoteStyle::Double) .with_jsx_quote_style(QuoteStyle::Single) @@ -42,7 +35,6 @@ const makeSomeFunction2 = let doc = format_node(options.clone(), &tree.syntax()).unwrap(); let result = doc.print().unwrap(); - let source_type = JsFileSource::js_module(); println!("{}", doc.into_document()); eprintln!("{}", result.as_code()); diff --git a/crates/biome_js_formatter/tests/specs/js/module/call_expression.js b/crates/biome_js_formatter/tests/specs/js/module/call_expression.js index e601d6b751f8..c7533b26d71e 100644 --- a/crates/biome_js_formatter/tests/specs/js/module/call_expression.js +++ b/crates/biome_js_formatter/tests/specs/js/module/call_expression.js @@ -61,4 +61,10 @@ const FilterButton = forwardRefWithLongFunctionName(function FilterButton(props, return