Skip to content

Commit

Permalink
Maybe parenthesize long constants and names
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Aug 24, 2023
1 parent e4c1384 commit 33a6c16
Show file tree
Hide file tree
Showing 20 changed files with 357 additions and 368 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions crates/ruff_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2534,17 +2534,17 @@ impl<'a, Context> BestFitting<'a, Context> {

impl<Context> Format<Context> for BestFitting<'_, Context> {
fn fmt(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
let mut buffer = VecBuffer::new(f.state_mut());
let variants = self.variants.items();

let mut formatted_variants = Vec::with_capacity(variants.len());

for variant in variants {
let mut buffer = VecBuffer::with_capacity(8, f.state_mut());
buffer.write_element(FormatElement::Tag(StartEntry));
buffer.write_fmt(Arguments::from(variant))?;
buffer.write_element(FormatElement::Tag(EndEntry));

formatted_variants.push(buffer.take_vec().into_boxed_slice());
formatted_variants.push(buffer.into_vec().into_boxed_slice());
}

// SAFETY: The constructor guarantees that there are always at least two variants. It's, therefore,
Expand Down
15 changes: 9 additions & 6 deletions crates/ruff_formatter/src/format_element/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,12 @@ impl Document {
expands
}

let mut enclosing: Vec<Enclosing> = Vec::new();
let mut interned: FxHashMap<&Interned, bool> = FxHashMap::default();
let mut enclosing = Vec::with_capacity(if self.is_empty() {
0
} else {
self.len().ilog2() as usize
});
let mut interned = FxHashMap::default();
propagate_expands(self, &mut enclosing, &mut interned);
}

Expand Down Expand Up @@ -657,18 +661,17 @@ impl Format<IrFormatContext<'_>> for ContentArrayEnd {

impl FormatElements for [FormatElement] {
fn will_break(&self) -> bool {
use Tag::{EndLineSuffix, StartLineSuffix};
let mut ignore_depth = 0usize;

for element in self {
match element {
// Line suffix
// Ignore if any of its content breaks
FormatElement::Tag(StartLineSuffix) => {
FormatElement::Tag(Tag::StartLineSuffix | Tag::StartFitsExpanded(_)) => {
ignore_depth += 1;
}
FormatElement::Tag(EndLineSuffix) => {
ignore_depth -= 1;
FormatElement::Tag(Tag::EndLineSuffix | Tag::EndFitsExpanded) => {
ignore_depth = ignore_depth.saturating_sub(1);
}
FormatElement::Interned(interned) if ignore_depth == 0 => {
if interned.will_break() {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_formatter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ clap = { workspace = true }
countme = "3.0.1"
is-macro = { workspace = true }
itertools = { workspace = true }
memchr = { workspace = true }
once_cell = { workspace = true }
rustc-hash = { workspace = true }
serde = { workspace = true, optional = true }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,9 @@
# Regression: https://github.com/astral-sh/ruff/issues/5338
if a and not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

if (
not
# comment
a):
...
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
x = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

x_aa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
xxxxx = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

while (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
):
pass

while aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
pass

# Only applies in `Parenthesize::IfBreaks` positions
raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

raise (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
)

raise a from aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
raise a from aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

# Can never apply on expression statement level
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

# Is it only relevant for items that can't break

aaaaaaa = 111111111111111111111111111111111111111111111111111111111111111111111111111111
aaaaaaa = (
1111111111111111111111111111111111111111111111111111111111111111111111111111111
)

aaaaaaa = """111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""

# Never parenthesize multiline strings
aaaaaaa = (
"""1111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""
)



aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbb
aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb


for converter in connection.ops.get_db_converters(
expression
) + expression.get_db_converters(connection):
...
5 changes: 4 additions & 1 deletion crates/ruff_python_formatter/src/expression/expr_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ impl NeedsParentheses for ExprCall {
{
OptionalParentheses::Multiline
} else {
self.func.needs_parentheses(self.into(), context)
match self.func.needs_parentheses(self.into(), context) {
OptionalParentheses::BestFit => OptionalParentheses::Never,
parentheses => parentheses,
}
}
}
}
22 changes: 13 additions & 9 deletions crates/ruff_python_formatter/src/expression/expr_constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use ruff_python_ast::{Constant, ExprConstant, Ranged};
use ruff_text_size::{TextLen, TextRange};

use crate::expression::number::{FormatComplex, FormatFloat, FormatInt};
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::expression::parentheses::{should_use_best_fit, NeedsParentheses, OptionalParentheses};
use crate::expression::string::{
AnyString, FormatString, StringLayout, StringPrefix, StringQuotes,
};
Expand Down Expand Up @@ -79,13 +79,16 @@ impl NeedsParentheses for ExprConstant {
_parent: AnyNodeRef,
context: &PyFormatContext,
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
// Don't wrap triple quoted strings
if is_multiline_string(self, context.source()) {
OptionalParentheses::Never
} else {
OptionalParentheses::Multiline
}
if is_multiline_string(self, context.source())
|| self.value.is_none()
|| self.value.is_bool()
|| self.value.is_ellipsis()
{
OptionalParentheses::Never
} else if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
} else if should_use_best_fit(self, context) {
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
Expand All @@ -99,7 +102,8 @@ pub(super) fn is_multiline_string(constant: &ExprConstant, source: &str) -> bool
let quotes =
StringQuotes::parse(&contents[TextRange::new(prefix.text_len(), contents.text_len())]);

quotes.is_some_and(StringQuotes::is_triple) && contents.contains(['\n', '\r'])
quotes.is_some_and(StringQuotes::is_triple)
&& memchr::memchr2(b'\n', b'\r', contents.as_bytes()).is_some()
} else {
false
}
Expand Down
16 changes: 13 additions & 3 deletions crates/ruff_python_formatter/src/expression/expr_f_string.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use super::string::{AnyString, FormatString};
use crate::context::PyFormatContext;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use memchr::memchr2;

use crate::expression::parentheses::{should_use_best_fit, NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::{FormatNodeRule, PyFormatter};
use ruff_formatter::FormatResult;
Expand All @@ -20,8 +22,16 @@ impl NeedsParentheses for ExprFString {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Multiline
if self.implicit_concatenated {
OptionalParentheses::Multiline
} else if memchr2(b'\n', b'\r', context.source()[self.range].as_bytes()).is_none()
&& should_use_best_fit(self, context)
{
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
}
}
10 changes: 7 additions & 3 deletions crates/ruff_python_formatter/src/expression/expr_name.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::comments::SourceComment;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::expression::parentheses::{should_use_best_fit, NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::FormatNodeRule;
use ruff_formatter::{write, FormatContext};
Expand Down Expand Up @@ -38,9 +38,13 @@ impl NeedsParentheses for ExprName {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
if should_use_best_fit(self, context) {
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ impl NeedsParentheses for ExprSubscript {
{
OptionalParentheses::Multiline
} else {
self.value.needs_parentheses(self.into(), context)
match self.value.needs_parentheses(self.into(), context) {
OptionalParentheses::BestFit => OptionalParentheses::Never,
parentheses => parentheses,
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_python_formatter/src/expression/expr_unary_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use ruff_python_ast::{ExprUnaryOp, Ranged};
use ruff_text_size::{TextLen, TextRange};

use ruff_formatter::prelude::{hard_line_break, space, text};
use ruff_formatter::{Format, FormatContext, FormatResult};
use ruff_formatter::{Format, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};

Expand Down Expand Up @@ -57,7 +57,7 @@ impl FormatNodeRule<ExprUnaryOp> for FormatExprUnaryOp {
// a)
// ```
if !leading_operand_comments.is_empty()
&& !is_operand_parenthesized(item, f.context().source_code().as_str())
&& !is_operand_parenthesized(item, f.context().source())
{
hard_line_break().fmt(f)?;
} else if op.is_not() {
Expand Down
61 changes: 60 additions & 1 deletion crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::cmp::Ordering;

use ruff_formatter::{
write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions,
format_args, write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions,
};
use ruff_python_ast as ast;
use ruff_python_ast::node::AnyNodeRef;
Expand Down Expand Up @@ -220,6 +220,64 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
}
}
},
OptionalParentheses::BestFit => match parenthesize {
Parenthesize::IfBreaksOrIfRequired => {
parenthesize_if_expands(&expression.format().with_options(Parentheses::Never))
.fmt(f)
}

Parenthesize::Optional | Parenthesize::IfRequired => {
expression.format().with_options(Parentheses::Never).fmt(f)
}
Parenthesize::IfBreaks => {
let group_id = f.group_id("optional_parentheses");
let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);
let mut format_expression = expression
.format()
.with_options(Parentheses::Never)
.memoized();

// Don't use best fitting if it is known that the expression can never fit
if format_expression.inspect(f)?.will_break() {
// The group here is necessary because `format_expression` may contain IR elements
// that refer to the group id
group(&format_expression)
.with_group_id(Some(group_id))
.should_expand(true)
.fmt(f)
} else {
// Only add parentheses if it makes the expression fit on the line.
// Using the flat version as the most expanded version gives a left-to-right splitting behavior
// which differs from when using regular groups, because they split right-to-left.
best_fitting![
// ---------------------------------------------------------------------
// Variant 1:
// Try to fit the expression without any parentheses
group(&format_expression).with_group_id(Some(group_id)),
// ---------------------------------------------------------------------
// Variant 2:
// Try to fit the expression by adding parentheses and indenting the expression.
group(&format_args![
text("("),
soft_block_indent(&format_expression),
text(")")
])
.with_group_id(Some(group_id))
.should_expand(true),
// ---------------------------------------------------------------------
// Variant 3: Fallback, no parentheses
// Expression doesn't fit regardless of adding the parentheses. Remove the parentheses again.
group(&format_expression)
.with_group_id(Some(group_id))
.should_expand(true)
]
// Measure all lines, to avoid that the printer decides that this fits right after hitting
// the `(`.
.with_mode(BestFittingMode::AllLines)
.fmt(f)
}
}
},
OptionalParentheses::Never => match parenthesize {
Parenthesize::IfBreaksOrIfRequired => {
parenthesize_if_expands(&expression.format().with_options(Parentheses::Never))
Expand All @@ -230,6 +288,7 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
expression.format().with_options(Parentheses::Never).fmt(f)
}
},

OptionalParentheses::Always => {
expression.format().with_options(Parentheses::Always).fmt(f)
}
Expand Down
Loading

0 comments on commit 33a6c16

Please sign in to comment.