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): avoid introducing linebreaks for single line string interpolations #2500

Merged
merged 13 commits into from
May 10, 2024
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

### Formatter

#### Bug fixes

- Fix [#2470](https://github.com/biomejs/biome/issues/2470) by avoid introducing linebreaks in single line string interpolations. Contributed by @ah-yu

### JavaScript APIs

### Linter
Expand Down
161 changes: 91 additions & 70 deletions crates/biome_formatter/src/buffer.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::{write, Arguments, FormatElement};
use crate::format_element::Interned;
use crate::prelude::tag::Condition;
use crate::prelude::{LineMode, PrintMode, Tag};
use crate::{Format, FormatResult, FormatState};
use rustc_hash::FxHashMap;
Expand Down Expand Up @@ -488,10 +489,8 @@ pub struct RemoveSoftLinesBuffer<'a, Context> {
/// 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,
/// Store the conditional content stack to help determine if the current element is within expanded conditional content.
conditional_content_stack: Vec<Condition>,
}

impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> {
Expand All @@ -500,20 +499,34 @@ impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> {
Self {
inner,
interned_cache: FxHashMap::default(),
is_in_expanded_conditional_content: false,
conditional_content_stack: Vec::new(),
}
}

/// Removes the soft line breaks from an interned element.
fn clean_interned(&mut self, interned: &Interned) -> Interned {
clean_interned(interned, &mut self.interned_cache)
clean_interned(
interned,
&mut self.interned_cache,
&mut self.conditional_content_stack,
)
}

/// Marker for whether a `StartConditionalContent(mode: Expanded)` has been
/// written but not yet closed.
fn is_in_expanded_conditional_content(&self) -> bool {
self.conditional_content_stack
.iter()
.last()
.is_some_and(|condition| condition.mode == PrintMode::Expanded)
}
}

// Extracted to function to avoid monomorphization
fn clean_interned(
interned: &Interned,
interned_cache: &mut FxHashMap<Interned, Interned>,
condition_content_stack: &mut Vec<Condition>,
) -> Interned {
match interned_cache.get(interned) {
Some(cleaned) => cleaned.clone(),
Expand All @@ -524,20 +537,18 @@ fn clean_interned(
.iter()
.enumerate()
.find_map(|(index, element)| match element {
FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace) => {
let mut cleaned = Vec::new();
cleaned.extend_from_slice(&interned[..index]);
Some((cleaned, &interned[index..]))
}
FormatElement::Tag(Tag::StartConditionalContent(condition))
if condition.mode == PrintMode::Expanded =>
{
FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace)
| FormatElement::Tag(
Tag::StartConditionalContent(_) | Tag::EndConditionalContent,
)
| FormatElement::BestFitting(_) => {
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);
let cleaned_inner =
clean_interned(inner, interned_cache, condition_content_stack);

if &cleaned_inner != inner {
let mut cleaned = Vec::with_capacity(interned.len());
Expand All @@ -548,41 +559,56 @@ fn clean_interned(
None
}
}

_ => None,
});

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 {
let element = match element {
FormatElement::Tag(Tag::StartConditionalContent(condition))
if condition.mode == PrintMode::Expanded =>
{
is_in_expanded_conditional_content = true;
let mut element_stack = rest.iter().rev().collect::<Vec<_>>();
while let Some(element) = element_stack.pop() {
match element {
FormatElement::Tag(Tag::StartConditionalContent(condition)) => {
condition_content_stack.push(condition.clone());
continue;
}
FormatElement::Tag(Tag::EndConditionalContent)
if is_in_expanded_conditional_content =>
{
is_in_expanded_conditional_content = false;
FormatElement::Tag(Tag::EndConditionalContent) => {
condition_content_stack.pop();
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,
_ if condition_content_stack
.iter()
.last()
.is_some_and(|condition| condition.mode == PrintMode::Expanded) =>
{
continue
}

FormatElement::Line(LineMode::Soft) => continue,
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
FormatElement::Line(LineMode::SoftOrSpace) => {
cleaned.push(FormatElement::Space)
}

FormatElement::Interned(interned) => {
FormatElement::Interned(clean_interned(interned, interned_cache))
cleaned.push(FormatElement::Interned(clean_interned(
interned,
interned_cache,
condition_content_stack,
)))
}
// Since this buffer aims to simulate infinite print width, we don't need to retain the best fitting.
// Just extract the flattest variant and then handle elements within it.
FormatElement::BestFitting(best_fitting) => {
let most_flat = best_fitting.most_flat();
most_flat
.iter()
.rev()
.for_each(|element| element_stack.push(element));
}
element => element.clone(),
element => cleaned.push(element.clone()),
};
cleaned.push(element)
}

Interned::new(cleaned)
Expand All @@ -601,47 +627,42 @@ impl<Context> Buffer for RemoveSoftLinesBuffer<'_, Context> {
type Context = Context;

fn write_element(&mut self, element: FormatElement) -> FormatResult<()> {
let element = match element {
FormatElement::Tag(Tag::StartConditionalContent(condition)) => {
match condition.mode {
PrintMode::Expanded => {
// Mark that we're within expanded content so that it
// can be dropped in all future writes until the ending
// tag.
self.is_in_expanded_conditional_content = true;
return Ok(());
}
PrintMode::Flat => {
// Flat groups have the conditional tag dropped as
// well, since the content within it will _always_ be
// printed by this buffer.
return Ok(());
}
}
}
FormatElement::Tag(Tag::EndConditionalContent) => {
// NOTE: This assumes that conditional content cannot be nested.
// This is true for all practical cases, but it's _possible_ to
// write IR that breaks this.
self.is_in_expanded_conditional_content = false;
// No matter if this was flat or expanded content, the ending
// tag gets dropped, since the starting tag was also dropped.
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(()),
let mut element_statck = Vec::new();
element_statck.push(element);

FormatElement::Line(LineMode::Soft) => return Ok(()),
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
while let Some(element) = element_statck.pop() {
match element {
FormatElement::Tag(Tag::StartConditionalContent(condition)) => {
self.conditional_content_stack.push(condition.clone());
}
FormatElement::Tag(Tag::EndConditionalContent) => {
self.conditional_content_stack.pop();
}
// 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() => continue,

FormatElement::Interned(interned) => {
FormatElement::Interned(self.clean_interned(&interned))
FormatElement::Line(LineMode::Soft) => continue,
FormatElement::Line(LineMode::SoftOrSpace) => {
self.inner.write_element(FormatElement::Space)?
}
FormatElement::Interned(interned) => {
let cleaned = self.clean_interned(&interned);
self.inner.write_element(FormatElement::Interned(cleaned))?
}
// Since this buffer aims to simulate infinite print width, we don't need to retain the best fitting.
// Just extract the flattest variant and then handle elements within it.
FormatElement::BestFitting(best_fitting) => {
let most_flat = best_fitting.most_flat();
most_flat
.iter()
.rev()
.for_each(|element| element_statck.push(element.clone()));
}
element => self.inner.write_element(element)?,
}
element => element,
};

self.inner.write_element(element)
}
Ok(())
}

fn elements(&self) -> &[FormatElement] {
Expand Down
71 changes: 61 additions & 10 deletions crates/biome_js_formatter/src/js/auxiliary/template_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,19 @@ use biome_formatter::{

use crate::context::TabWidth;
use crate::js::expressions::array_expression::FormatJsArrayExpressionOptions;
use crate::js::lists::template_element_list::{TemplateElementIndention, TemplateElementLayout};
use crate::js::lists::template_element_list::TemplateElementIndention;
use biome_js_syntax::{
AnyJsExpression, JsSyntaxNode, JsSyntaxToken, JsTemplateElement, TsTemplateElement,
};
use biome_rowan::{declare_node_union, AstNode, SyntaxResult};
use biome_rowan::{declare_node_union, AstNode, NodeOrToken, SyntaxResult};

enum TemplateElementLayout {
/// Tries to format the expression on a single line regardless of the print width.
SingleLine,

/// Tries to format the expression on a single line but may break the expression if the line otherwise exceeds the print width.
Fit,
}

#[derive(Debug, Clone, Default)]
pub(crate) struct FormatJsTemplateElement {
Expand Down Expand Up @@ -44,8 +52,6 @@ declare_node_union! {

#[derive(Debug, Copy, Clone, Default)]
pub struct TemplateElementOptions {
pub(crate) layout: TemplateElementLayout,

/// The indention to use for this element
pub(crate) indention: TemplateElementIndention,

Expand Down Expand Up @@ -82,10 +88,33 @@ impl Format<JsFormatContext> for FormatTemplateElement {
}
});

let format_inner = format_with(|f: &mut JsFormatter| match self.options.layout {
let interned_expression = f.intern(&format_expression)?;

let layout = if !self.element.has_new_line_in_range() {
let will_break = if let Some(element) = &interned_expression {
element.will_break()
} else {
false
};

// make sure the expression won't break to prevent reformat issue
if will_break {
TemplateElementLayout::Fit
} else {
TemplateElementLayout::SingleLine
}
} else {
TemplateElementLayout::Fit
};

let format_inner = format_with(|f: &mut JsFormatter| match layout {
TemplateElementLayout::SingleLine => {
let mut buffer = RemoveSoftLinesBuffer::new(f);
write!(buffer, [format_expression])

match &interned_expression {
Some(element) => buffer.write_element(element.clone()),
None => Ok(()),
}
}
TemplateElementLayout::Fit => {
use AnyJsExpression::*;
Expand All @@ -111,13 +140,21 @@ impl Format<JsFormatContext> for FormatTemplateElement {
| JsLogicalExpression(_)
| JsInstanceofExpression(_)
| JsInExpression(_)
| JsIdentifierExpression(_)
)
);

if indent {
write!(f, [soft_block_indent(&format_expression)])
} else {
write!(f, [format_expression])
match &interned_expression {
Some(element) if indent => {
write!(
f,
[soft_block_indent(&format_with(
|f| f.write_element(element.clone())
))]
)
}
Some(element) => f.write_element(element.clone()),
None => Ok(()),
}
}
});
Expand Down Expand Up @@ -179,6 +216,20 @@ impl AnyTemplateElement {
AnyTemplateElement::TsTemplateElement(template) => template.r_curly_token(),
}
}

fn has_new_line_in_range(&self) -> bool {
fn has_new_line_in_node(node: &JsSyntaxNode) -> bool {
node.children_with_tokens().any(|child| match child {
NodeOrToken::Token(token) => token
// no need to check for trailing trivia as it's not possible to have a new line
.leading_trivia()
.pieces()
.any(|trivia| trivia.is_newline()),
NodeOrToken::Node(node) => has_new_line_in_node(&node),
})
}
has_new_line_in_node(self.syntax())
}
}

/// Writes `content` with the specified `indention`.
Expand Down
Loading