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

Join implicit concatenated strings when they fit on a line #13663

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion crates/ruff_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1454,7 +1454,7 @@ impl<Context> std::fmt::Debug for Group<'_, Context> {
/// layout doesn't exceed the line width too, in which case it falls back to the flat layout.
///
/// This IR is identical to the following [`best_fitting`] layout but is implemented as custom IR for
/// best performance.
/// better performance.
///
/// ```rust
/// # use ruff_formatter::prelude::*;
Expand Down
2 changes: 0 additions & 2 deletions crates/ruff_python_formatter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use ruff_source_file::Locator;
use std::fmt::{Debug, Formatter};
use std::ops::{Deref, DerefMut};

#[derive(Clone)]
pub struct PyFormatContext<'a> {
options: PyFormatOptions,
contents: &'a str,
Expand Down Expand Up @@ -52,7 +51,6 @@ impl<'a> PyFormatContext<'a> {
self.contents
}

#[allow(unused)]
pub(crate) fn locator(&self) -> Locator<'a> {
Locator::new(self.contents)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use ruff_formatter::FormatRuleWithOptions;
use ruff_formatter::GroupId;
use ruff_python_ast::ExprBytesLiteral;
use ruff_python_ast::{AnyNodeRef, StringLike};

Expand All @@ -8,15 +10,38 @@ use crate::prelude::*;
use crate::string::{FormatImplicitConcatenatedString, StringLikeExtensions};

#[derive(Default)]
pub struct FormatExprBytesLiteral;
pub struct FormatExprBytesLiteral {
layout: ExprBytesLiteralLayout,
}

#[derive(Default)]
pub struct ExprBytesLiteralLayout {
pub implicit_group_id: Option<GroupId>,
}

impl FormatRuleWithOptions<ExprBytesLiteral, PyFormatContext<'_>> for FormatExprBytesLiteral {
type Options = ExprBytesLiteralLayout;

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

impl FormatNodeRule<ExprBytesLiteral> for FormatExprBytesLiteral {
fn fmt_fields(&self, item: &ExprBytesLiteral, f: &mut PyFormatter) -> FormatResult<()> {
let ExprBytesLiteral { value, .. } = item;

match value.as_slice() {
[bytes_literal] => bytes_literal.format().fmt(f),
_ => in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item)).fmt(f),
_ => match self.layout.implicit_group_id {
Some(group_id) => group(&FormatImplicitConcatenatedString::new(item))
.with_group_id(Some(group_id))
.fmt(f),
None => {
in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item)).fmt(f)
}
},
}
}
}
Expand Down
31 changes: 28 additions & 3 deletions crates/ruff_python_formatter/src/expression/expr_f_string.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use ruff_formatter::{FormatRuleWithOptions, GroupId};
use ruff_python_ast::{AnyNodeRef, ExprFString, StringLike};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;
Expand All @@ -10,7 +11,23 @@ use crate::prelude::*;
use crate::string::{FormatImplicitConcatenatedString, Quoting, StringLikeExtensions};

#[derive(Default)]
pub struct FormatExprFString;
pub struct FormatExprFString {
layout: ExprFStringLayout,
}

#[derive(Default)]
pub struct ExprFStringLayout {
pub implicit_group_id: Option<GroupId>,
}

impl FormatRuleWithOptions<ExprFString, PyFormatContext<'_>> for FormatExprFString {
type Options = ExprFStringLayout;

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

impl FormatNodeRule<ExprFString> for FormatExprFString {
fn fmt_fields(&self, item: &ExprFString, f: &mut PyFormatter) -> FormatResult<()> {
Expand All @@ -22,7 +39,14 @@ impl FormatNodeRule<ExprFString> for FormatExprFString {
f_string_quoting(item, &f.context().locator()),
)
.fmt(f),
_ => in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item)).fmt(f),
_ => match self.layout.implicit_group_id {
Some(group_id) => group(&FormatImplicitConcatenatedString::new(item))
.with_group_id(Some(group_id))
.fmt(f),
None => {
in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item)).fmt(f)
}
},
}
}
}
Expand All @@ -35,6 +59,7 @@ impl NeedsParentheses for ExprFString {
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
}
// TODO(dhruvmanila): Ideally what we want here is a new variant which
// is something like:
// - If the expression fits by just adding the parentheses, then add them and
Expand All @@ -53,7 +78,7 @@ impl NeedsParentheses for ExprFString {
// ```
// This isn't decided yet, refer to the relevant discussion:
// https://github.com/astral-sh/ruff/discussions/9785
} else if StringLike::FString(self).is_multiline(context.source()) {
else if StringLike::FString(self).is_multiline(context.source()) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use ruff_formatter::FormatRuleWithOptions;
use ruff_formatter::{FormatRuleWithOptions, GroupId};
use ruff_python_ast::{AnyNodeRef, ExprStringLiteral, StringLike};

use crate::expression::parentheses::{
Expand All @@ -10,14 +10,29 @@ use crate::string::{FormatImplicitConcatenatedString, StringLikeExtensions};

#[derive(Default)]
pub struct FormatExprStringLiteral {
kind: StringLiteralKind,
layout: ExprStringLiteralLayout,
}

#[derive(Default)]
pub struct ExprStringLiteralLayout {
pub kind: StringLiteralKind,
pub implicit_group_id: Option<GroupId>,
}

impl ExprStringLiteralLayout {
pub const fn docstring() -> Self {
Self {
kind: StringLiteralKind::Docstring,
implicit_group_id: None,
}
}
}

impl FormatRuleWithOptions<ExprStringLiteral, PyFormatContext<'_>> for FormatExprStringLiteral {
type Options = StringLiteralKind;
type Options = ExprStringLiteralLayout;

fn with_options(mut self, options: Self::Options) -> Self {
self.kind = options;
self.layout = options;
self
}
}
Expand All @@ -27,15 +42,23 @@ impl FormatNodeRule<ExprStringLiteral> for FormatExprStringLiteral {
let ExprStringLiteral { value, .. } = item;

match value.as_slice() {
[string_literal] => string_literal.format().with_options(self.kind).fmt(f),
[string_literal] => string_literal
.format()
.with_options(self.layout.kind)
.fmt(f),
_ => {
// This is just a sanity check because [`DocstringStmt::try_from_statement`]
// ensures that the docstring is a *single* string literal.
assert!(!self.kind.is_docstring());
assert!(!self.layout.kind.is_docstring());

in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item))
match self.layout.implicit_group_id {
Some(group_id) => group(&FormatImplicitConcatenatedString::new(item))
.with_group_id(Some(group_id))
.fmt(f),
None => in_parentheses_only_group(&FormatImplicitConcatenatedString::new(item))
.fmt(f),
}
}
.fmt(f),
}
}
}
Expand All @@ -48,7 +71,7 @@ impl NeedsParentheses for ExprStringLiteral {
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
} else if StringLike::String(self).is_multiline(context.source()) {
} else if StringLike::from(self).is_multiline(context.source()) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
12 changes: 8 additions & 4 deletions crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -768,15 +768,18 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
Expr::StringLiteral(ast::ExprStringLiteral { value, .. })
if value.is_implicit_concatenated() =>
{
self.update_max_precedence(OperatorPrecedence::String);
// FIXME make this a preview only change
// self.update_max_precedence(OperatorPrecedence::String);
return;
}
Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. })
if value.is_implicit_concatenated() =>
{
self.update_max_precedence(OperatorPrecedence::String);
// self.update_max_precedence(OperatorPrecedence::String);
return;
}
Expr::FString(ast::ExprFString { value, .. }) if value.is_implicit_concatenated() => {
self.update_max_precedence(OperatorPrecedence::String);
// self.update_max_precedence(OperatorPrecedence::String);
return;
}

Expand Down Expand Up @@ -1254,8 +1257,9 @@ pub(crate) fn is_splittable_expression(expr: &Expr, context: &PyFormatContext) -
}

// String like literals can expand if they are implicit concatenated.
// TODO REVIEW
Expr::FString(fstring) => fstring.value.is_implicit_concatenated(),
Expr::StringLiteral(string) => string.value.is_implicit_concatenated(),
Expr::StringLiteral(_) => false,
Expr::BytesLiteral(bytes) => bytes.value.is_implicit_concatenated(),

// Expressions that have no split points per se, but they contain nested sub expressions that might expand.
Expand Down
19 changes: 10 additions & 9 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ mod tests {
use ruff_python_trivia::CommentRanges;
use ruff_text_size::{TextRange, TextSize};

use crate::{format_module_ast, format_module_source, format_range, PyFormatOptions};
use crate::{
format_module_ast, format_module_source, format_range, PreviewMode, PyFormatOptions,
};

/// Very basic test intentionally kept very similar to the CLI
#[test]
Expand Down Expand Up @@ -188,13 +190,11 @@ if True:
#[test]
fn quick_test() {
let source = r#"
def main() -> None:
if True:
some_very_long_variable_name_abcdefghijk = Foo()
some_very_long_variable_name_abcdefghijk = some_very_long_variable_name_abcdefghijk[
some_very_long_variable_name_abcdefghijk.some_very_long_attribute_name
== "This is a very long string abcdefghijk"
]
fstring = (
f"We have to remember to escape {braces}."
" Like {these}."
f" But not {this}."
)
"#;
let source_type = PySourceType::Python;
Expand All @@ -203,7 +203,8 @@ def main() -> None:
let source_path = "code_inline.py";
let parsed = parse(source, source_type.as_mode()).unwrap();
let comment_ranges = CommentRanges::from(parsed.tokens());
let options = PyFormatOptions::from_extension(Path::new(source_path));
let options = PyFormatOptions::from_extension(Path::new(source_path))
.with_preview(PreviewMode::Enabled);
let formatted = format_module_ast(&parsed, &comment_ranges, source, options).unwrap();

// Uncomment the `dbg` to print the IR.
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_python_formatter/src/other/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ fn is_huggable_string_argument(
arguments: &Arguments,
context: &PyFormatContext,
) -> bool {
// TODO: Implicit concatenated could become regular string. Although not if it is multiline. So that should be fine?
// Double check
if string.is_implicit_concatenated() || !string.is_multiline(context.source()) {
return false;
}
Expand Down
11 changes: 3 additions & 8 deletions crates/ruff_python_formatter/src/other/f_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,9 @@ impl Format<PyFormatContext<'_>> for FormatFString<'_> {
let quotes = StringQuotes::from(string_kind);
write!(f, [string_kind.prefix(), quotes])?;

f.join()
.entries(
self.value
.elements
.iter()
.map(|element| FormatFStringElement::new(element, context)),
)
.finish()?;
for element in &self.value.elements {
FormatFStringElement::new(element, context).fmt(f)?;
}

// Ending quote
quotes.fmt(f)
Expand Down
10 changes: 4 additions & 6 deletions crates/ruff_python_formatter/src/other/f_string_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl<'a> FormatFStringLiteralElement<'a> {
impl Format<PyFormatContext<'_>> for FormatFStringLiteralElement<'_> {
fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> {
let literal_content = f.context().locator().slice(self.element.range());
let normalized = normalize_string(literal_content, 0, self.context.flags(), true);
let normalized = normalize_string(literal_content, 0, self.context.flags(), true, false);
match &normalized {
Cow::Borrowed(_) => source_text_slice(self.element.range()).fmt(f),
Cow::Owned(normalized) => text(normalized).fmt(f),
Expand Down Expand Up @@ -231,11 +231,9 @@ impl Format<PyFormatContext<'_>> for FormatFStringExpressionElement<'_> {
if let Some(format_spec) = format_spec.as_deref() {
token(":").fmt(f)?;

f.join()
.entries(format_spec.elements.iter().map(|element| {
FormatFStringElement::new(element, self.context.f_string())
}))
.finish()?;
for element in &format_spec.elements {
FormatFStringElement::new(element, self.context.f_string()).fmt(f)?;
}

// These trailing comments can only occur if the format specifier is
// present. For example,
Expand Down
16 changes: 10 additions & 6 deletions crates/ruff_python_formatter/src/pattern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,18 +289,22 @@ impl<'a> CanOmitOptionalParenthesesVisitor<'a> {

Pattern::MatchValue(value) => match &*value.value {
Expr::StringLiteral(string) => {
self.update_max_precedence(OperatorPrecedence::String, string.value.len());
// TODO update?

// self.update_max_precedence(OperatorPrecedence::String, string.value.len());
}
Expr::BytesLiteral(bytes) => {
self.update_max_precedence(OperatorPrecedence::String, bytes.value.len());
// TODO update?
// self.update_max_precedence(OperatorPrecedence::String, bytes.value.len());
}
// F-strings are allowed according to python's grammar but fail with a syntax error at runtime.
// That's why we need to support them for formatting.
Expr::FString(string) => {
self.update_max_precedence(
OperatorPrecedence::String,
string.value.as_slice().len(),
);
// TODO update?
// self.update_max_precedence(
// OperatorPrecedence::String,
// string.value.as_slice().len(),
// );
}

Expr::NumberLiteral(_) | Expr::Attribute(_) | Expr::UnaryOp(_) => {
Expand Down
8 changes: 8 additions & 0 deletions crates/ruff_python_formatter/src/preview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,11 @@ pub(crate) fn is_docstring_code_block_in_docstring_indent_enabled(
) -> bool {
context.is_preview()
}

/// Returns `true` if implicitly concatenated strings should be joined if they all fit on a single line.
/// See [#9457](https://github.com/astral-sh/ruff/issues/9457)
/// WARNING: This preview style depends on `is_empty_parameters_no_unnecessary_parentheses_around_return_value_enabled`
/// because it relies on the new semantic of `IfBreaksParenthesized`.
pub(crate) fn is_join_implicit_concatenated_string_enabled(context: &PyFormatContext) -> bool {
context.is_preview()
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ impl FormatNodeRule<StmtAssert> for FormatStmtAssert {
[
token(","),
space(),
maybe_parenthesize_expression(msg, item, Parenthesize::IfBreaks),
maybe_parenthesize_expression(msg, item, Parenthesize::IfBreaksParenthesized),
]
)?;
}
Expand Down
Loading
Loading