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

fix(js_formatter): remove conditional expanded content in RemoveSoftLinesBuffer #1032

Merged
merged 3 commits into from
Dec 4, 2023
Merged

fix(js_formatter): remove conditional expanded content in RemoveSoftLinesBuffer #1032

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21a7f64
fix(js_formatter): Remove conditional expanded content in RemoveSoftL…
faultyserver Dec 3, 2023
0f628f9
Merge branch 'main' into faulty/destructured-arrow-param
faultyserver Dec 3, 2023
c9d982e
Merge branch 'main' into faulty/destructured-arrow-param
faultyserver Dec 4, 2023
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
57 changes: 55 additions & 2 deletions crates/biome_formatter/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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<Interned, Interned>,

/// 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> {
Expand All @@ -495,6 +500,7 @@ impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> {
Self {
inner,
interned_cache: FxHashMap::default(),
is_in_expanded_conditional_content: false,
}
}

Expand All @@ -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()
Expand All @@ -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);

Expand All @@ -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))
}
Expand All @@ -570,8 +606,25 @@ impl<Context> 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))
}
Expand Down
8 changes: 1 addition & 7 deletions crates/biome_js_formatter/src/js/bindings/constructor_parameters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<JsConstructorParameters> 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(
Expand Down
18 changes: 4 additions & 14 deletions crates/biome_js_formatter/src/js/bindings/formal_parameter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -37,16 +34,9 @@ impl FormatNodeRule<JsFormalParameter> 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(&parameters, f.comments()).unwrap_or(false)
});

if is_hug_parameter && decorators.is_empty() {
Expand Down
128 changes: 29 additions & 99 deletions crates/biome_js_formatter/src/js/bindings/parameters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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<JsParameters> for FormatJsParameters {
type Options = FormatJsParametersOptions;

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

impl FormatNodeRule<JsParameters> 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<JsFormatContext> for FormatAnyJsParameters {
Expand All @@ -81,8 +43,6 @@ 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 @@ -100,7 +60,7 @@ impl Format<JsFormatContext> 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()
]
)
Expand Down Expand Up @@ -149,29 +109,6 @@ 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)])?;
}

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 @@ -180,57 +117,61 @@ impl Format<JsFormatContext> for FormatAnyJsParameters {

impl FormatAnyJsParameters {
fn l_paren_token(&self) -> SyntaxResult<JsSyntaxToken> {
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<JsSyntaxToken> {
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<bool> {
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<JsArrowFunctionExpression> {
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(),
}
}
}
Expand Down Expand Up @@ -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(
Expand Down
Loading