Skip to content

Commit

Permalink
All tests are passing
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Oct 16, 2024
1 parent 7728d64 commit c7380d4
Show file tree
Hide file tree
Showing 18 changed files with 294 additions and 206 deletions.
10 changes: 4 additions & 6 deletions crates/ruff_python_formatter/src/expression/expr_f_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ impl NeedsParentheses for ExprFString {
_parent: AnyNodeRef,
context: &PyFormatContext,
) -> OptionalParentheses {
// if self.value.is_implicit_concatenated() {
// OptionalParentheses::Multiline
// } else
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 @@ -77,9 +77,7 @@ impl NeedsParentheses for ExprFString {
// ```
// This isn't decided yet, refer to the relevant discussion:
// https://github.com/astral-sh/ruff/discussions/9785
if !self.value.is_implicit_concatenated()
&& AnyString::FString(self).is_multiline(context.source())
{
else if AnyString::FString(self).is_multiline(context.source()) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,9 @@ impl NeedsParentheses for ExprStringLiteral {
_parent: AnyNodeRef,
context: &PyFormatContext,
) -> OptionalParentheses {
// TODO: This is complicated. We should only return multiline if we *know* that
// it won't fit because of how assignment works.
// if self.value.is_implicit_concatenated() {
// OptionalParentheses::Multiline
// } else
if AnyString::String(self).is_multiline(context.source()) {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
} else if AnyString::from(self).is_multiline(context.source()) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1257,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) => false,
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
7 changes: 4 additions & 3 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,10 @@ if True:
#[test]
fn quick_test() {
let source = r#"
(
f'{one}'
f'{two}'
fstring = (
f"We have to remember to escape {braces}."
" Like {these}."
f" But not {this}."
)
"#;
Expand Down
2 changes: 1 addition & 1 deletion 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
19 changes: 9 additions & 10 deletions crates/ruff_python_formatter/src/statement/stmt_assign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,12 @@ impl Format<PyFormatContext<'_>> for FormatStatementsLastExpression<'_> {
match self {
FormatStatementsLastExpression::LeftToRight { value, statement } => {
let can_inline_comment = should_inline_comments(value, *statement, f.context());
let is_implicit_concatenated = AnyString::try_from(*value).is_ok_and(|string| {
string.is_implicit_concatenated()
&& !string.is_implicit_and_cant_join(f.context())
});

if !can_inline_comment {
if !can_inline_comment && !is_implicit_concatenated {
return maybe_parenthesize_expression(
value,
*statement,
Expand All @@ -304,16 +308,9 @@ impl Format<PyFormatContext<'_>> for FormatStatementsLastExpression<'_> {
*statement,
&comments,
) {
let is_implicit_concatenated = AnyString::try_from(*value)
.is_ok_and(|string| string.is_implicit_concatenated());

let group_id = f.group_id("optional_parentheses");

let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);

if is_implicit_concatenated {
let implicit_group_id = f.group_id("implicit_concatenated");
best_fit_parenthesize(&format_with(|f| {
optional_parentheses(&format_with(|f| {
inline_comments.mark_formatted();

match value {
Expand Down Expand Up @@ -366,7 +363,6 @@ impl Format<PyFormatContext<'_>> for FormatStatementsLastExpression<'_> {

Ok(())
}))
.with_group_id(Some(group_id))
.fmt(f)?;

if !inline_comments.is_empty() {
Expand All @@ -376,6 +372,9 @@ impl Format<PyFormatContext<'_>> for FormatStatementsLastExpression<'_> {
.fmt(f)?;
}
} else {
let group_id = f.group_id("optional_parentheses");
let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);

best_fit_parenthesize(&format_once(|f| {
inline_comments.mark_formatted();

Expand Down
1 change: 0 additions & 1 deletion crates/ruff_python_formatter/src/statement/suite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use crate::comments::{
};
use crate::context::{NodeLevel, TopLevelStatementPosition, WithIndentLevel, WithNodeLevel};
use crate::expression::expr_string_literal::ExprStringLiteralLayout;
use crate::other::string_literal::StringLiteralKind;
use crate::prelude::*;
use crate::statement::stmt_expr::FormatStmtExpr;
use crate::verbatim::{
Expand Down
23 changes: 20 additions & 3 deletions crates/ruff_python_formatter/src/string/any.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@ use std::iter::FusedIterator;

use memchr::memchr2;

use ruff_formatter::Format;
use ruff_python_ast::{
self as ast, AnyNodeRef, AnyStringFlags, Expr, ExprBytesLiteral, ExprFString,
ExprStringLiteral, ExpressionRef, FStringElement, FStringPart, StringFlags, StringLiteral,
ExprStringLiteral, ExpressionRef, StringFlags, StringLiteral,
};
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

use crate::expression::expr_f_string::f_string_quoting;
use crate::string::Quoting;

use super::{PyFormatContext, PyFormatter};
use super::PyFormatContext;

/// Represents any kind of string expression. This could be either a string,
/// bytes or f-string.
Expand Down Expand Up @@ -50,6 +49,24 @@ impl<'a> AnyString<'a> {
matches!(self, Self::FString(_))
}

pub(crate) fn is_implicit_and_cant_join(self, context: &PyFormatContext) -> bool {
if !self.is_implicit_concatenated() {
return false;
}

for part in self.parts() {
if part.flags().is_triple_quoted() || part.flags().is_raw_string() {
return true;
}

if context.comments().leading_trailing(&part).next().is_some() {
return true;
}
}

false
}

/// Returns the quoting to be used for this string.
pub(super) fn quoting(self, locator: &Locator<'_>) -> Quoting {
match self {
Expand Down
51 changes: 29 additions & 22 deletions crates/ruff_python_formatter/src/string/mod.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,3 @@
pub(crate) use any::AnyString;
pub(crate) use normalize::{normalize_string, NormalizedString, StringNormalizer};
use ruff_formatter::{format_args, write};
use ruff_python_ast::str::Quote;
use ruff_python_ast::{
str_prefix::{AnyStringPrefix, StringLiteralPrefix},
AnyStringFlags, StringFlags,
};
use std::borrow::Cow;

use crate::comments::{leading_comments, trailing_comments};
use crate::expression::parentheses::in_parentheses_only_soft_line_break_or_space;
use crate::other::f_string::FormatFString;
Expand All @@ -17,6 +7,16 @@ use crate::preview::is_f_string_formatting_enabled;
use crate::string::any::AnyStringPart;
use crate::string::normalize::QuoteMetadata;
use crate::QuoteStyle;
pub(crate) use any::AnyString;
pub(crate) use normalize::{normalize_string, NormalizedString, StringNormalizer};
use ruff_formatter::{format_args, write};
use ruff_python_ast::str::Quote;
use ruff_python_ast::str_prefix::{ByteStringPrefix, FStringPrefix};
use ruff_python_ast::{
str_prefix::{AnyStringPrefix, StringLiteralPrefix},
AnyStringFlags, StringFlags,
};
use std::borrow::Cow;

mod any;
pub(crate) mod docstring;
Expand All @@ -42,7 +42,7 @@ impl<'a> FormatImplicitConcatenatedString<'a> {
}
}

fn merged_prefix(&self, context: &PyFormatContext) -> Option<Quote> {
fn merged_flags(&self, context: &PyFormatContext) -> Option<AnyStringFlags> {
// Early exit if it's known that this string can't be joined to avoid
// unnecessary computation of the quote metadata.
if self.string.parts().any(|part| {
Expand Down Expand Up @@ -79,6 +79,11 @@ impl<'a> FormatImplicitConcatenatedString<'a> {

// TODO: Do we need to respect the quoting?
let mut merged_quotes: Option<QuoteMetadata> = None;
let mut prefix = match self.string {
AnyString::String(_) => AnyStringPrefix::Regular(StringLiteralPrefix::Empty),
AnyString::Bytes(_) => AnyStringPrefix::Bytes(ByteStringPrefix::Regular),
AnyString::FString(_) => AnyStringPrefix::Format(FStringPrefix::Regular),
};

// TODO unify quote styles.
// Possibly run directly on entire string?
Expand All @@ -96,6 +101,10 @@ impl<'a> FormatImplicitConcatenatedString<'a> {
// Again, this takes a StringPart and not a `AnyStringPart`.
let part_quote_metadata = QuoteMetadata::from_part(part, preferred_quote, context);

if part.flags().is_f_string() {
prefix = AnyStringPrefix::Format(FStringPrefix::Regular);
}

if let Some(merged) = merged_quotes.as_mut() {
// FIXME: this is not correct.
*merged = part_quote_metadata.merge(merged)?;
Expand All @@ -104,7 +113,11 @@ impl<'a> FormatImplicitConcatenatedString<'a> {
}
}

Some(merged_quotes?.choose(preferred_quote))
Some(AnyStringFlags::new(
prefix,
merged_quotes?.choose(preferred_quote),
false,
))
}
}

Expand Down Expand Up @@ -143,28 +156,22 @@ impl Format<PyFormatContext<'_>> for FormatImplicitConcatenatedString<'_> {
joiner.finish()
});

if let Some(collapsed_quotes) = self.merged_prefix(f.context()) {
if let Some(flags) = self.merged_flags(f.context()) {
let format_flat = format_with(|f| {
let mut parts = self.string.parts();

let Some(first_part) = parts.next() else {
return Ok(());
};

// TODO handle different prefixes, e.g. f-string and non fstrings. We should probably handle this
// inside of `merged_prefix`
let flags = first_part.flags().with_quote_style(collapsed_quotes);
let quotes = StringQuotes::from(flags);

write!(f, [flags.prefix(), quotes])?;

// TODO: strings in expression statements aren't joined correctly because they aren't wrap in a group :(

for part in self.string.parts() {
let content = f.context().locator().slice(part.content_range());
let normalized = normalize_string(
content,
0,
flags,
is_f_string_formatting_enabled(f.context()),
flags.is_f_string() && !part.flags().is_f_string(),
);
match normalized {
Cow::Borrowed(_) => source_text_slice(part.content_range()).fmt(f)?,
Expand Down
32 changes: 22 additions & 10 deletions crates/ruff_python_formatter/src/string/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ impl<'a> NormalizedString<'a> {
start_offset,
new_flags,
is_f_string_formatting_enabled(context),
false,
),
}
}
Expand Down Expand Up @@ -557,6 +558,7 @@ pub(crate) fn normalize_string(
start_offset: usize,
new_flags: AnyStringFlags,
format_f_string: bool,
escape_braces: bool,
) -> Cow<str> {
// The normalized string if `input` is not yet normalized.
// `output` must remain empty if `input` is already normalized.
Expand All @@ -577,18 +579,27 @@ pub(crate) fn normalize_string(
let mut formatted_value_nesting = 0u32;

while let Some((index, c)) = chars.next() {
if is_fstring && matches!(c, '{' | '}') {
if chars.peek().copied().is_some_and(|(_, next)| next == c) {
// Skip over the second character of the double braces
chars.next();
} else if c == '{' {
formatted_value_nesting += 1;
} else {
// Safe to assume that `c == '}'` here because of the matched pattern above
formatted_value_nesting = formatted_value_nesting.saturating_sub(1);
if matches!(c, '{' | '}') {
if escape_braces {
// Escape `{` and `}` when converting a regular string literal to an f-string literal.
output.push_str(&input[last_index..=index]);
output.push(c);
last_index = index + c.len_utf8();
continue;
} else if is_fstring {
if chars.peek().copied().is_some_and(|(_, next)| next == c) {
// Skip over the second character of the double braces
chars.next();
} else if c == '{' {
formatted_value_nesting += 1;
} else {
// Safe to assume that `c == '}'` here because of the matched pattern above
formatted_value_nesting = formatted_value_nesting.saturating_sub(1);
}
continue;
}
continue;
}

if c == '\r' {
output.push_str(&input[last_index..index]);

Expand Down Expand Up @@ -847,6 +858,7 @@ mod tests {
false,
),
true,
false,
);

assert_eq!(r"\x89\x50\x4e\x47\x0d\x0a\x1a\x0a", &normalized);
Expand Down
Loading

0 comments on commit c7380d4

Please sign in to comment.