Formatter collapses overlong assignment's parenthesized value

[cnpryer]

Closest report I could find is #6271

Line-length: 88

# Input
def f():
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = (
        True
    )

# Black (23.7.0)
def f():
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = (
        True
    )

# Ruff (0.0.287)
def f():
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = True

Black Playground

Ruff Playground

[cnpryer]

Hmm. What's the best implementation here?

Would ExprConstant's NeedsParentheses need to be updated to consider if the expression breaks? The way maybe_parenthesize for Expr is called from FormatStmtAssign seems right to me.

ruff/crates/ruff_python_formatter/src/expression/parentheses.rs

Lines 68 to 69 in 577280c

	/// Parenthesizes the expression only if it doesn't fit on a line.
	IfBreaks,

Parenthesize if the expression breaks. But IIUC since ExprConstant's NeedParentheses will return Never then the Parenthesize::IfBreaks strategy will never be used.

Would it make more sense to add conditional logic to consider this when needs_parentheses is evaluated?

ruff/crates/ruff_python_formatter/src/expression/mod.rs

Lines 199 to 206 in 577280c

	let needs_parentheses = match expression.needs_parentheses(*parent, f.context()) {
	OptionalParentheses::Always => OptionalParentheses::Always,
	// The reason to add parentheses is to avoid a syntax error when breaking an expression over multiple lines.
	// Therefore, it is unnecessary to add an additional pair of parentheses if an outer expression
	// is parenthesized.
	_ if f.context().node_level().is_parenthesized() => OptionalParentheses::Never,
	needs_parentheses => needs_parentheses,
	};

In order to avoid reaching the Never match?

ruff/crates/ruff_python_formatter/src/expression/mod.rs

Lines 290 to 299 in 577280c

	OptionalParentheses::Never => match parenthesize {
	Parenthesize::IfBreaksOrIfRequired => {
	parenthesize_if_expands(&expression.format().with_options(Parentheses::Never))
	.fmt(f)
	}
	Parenthesize::Optional | Parenthesize::IfBreaks | Parenthesize::IfRequired => {
	expression.format().with_options(Parentheses::Never).fmt(f)
	}
	},

[cnpryer]

Using OptionalParentheses::Multiline would resolve this, but then the following would need to be addressed

Snapshot file: crates/ruff_python_formatter/tests/snapshots/format@expression__unsplittable.py.snap
Snapshot: unsplittable
Source: crates/ruff_python_formatter/tests/fixtures.rs:160
Input file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unsplittable.py
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  109   109 │ aaaaaaa = (
  110   110 │     1111111111111111111111111111111111111111111111111111111111111111111111111111111
  111   111 │ )
  112   112 │ 
  113       │-aaaaaaa = """111111111111111111111111111111111111111111111111111111111111111111111111111
        113 │+aaaaaaa = (
        114 │+    """111111111111111111111111111111111111111111111111111111111111111111111111111
  114   115 │ 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""
        116 │+)
  115   117 │ 
  116   118 │ # Never parenthesize multiline strings
  117       │-aaaaaaa = """1111111111111111111111111111111111111111111111111111111111111111111111111111
        119 │+aaaaaaa = (
        120 │+    """1111111111111111111111111111111111111111111111111111111111111111111111111111
  118   121 │ 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""
        122 │+)

[MichaReiser]

This is related to #6816 and specifically this "optimisation"

ruff/crates/ruff_python_formatter/src/expression/parentheses.rs

Lines 38 to 50 in c05e462

	// Only use best fits if:
	// * The text is longer than 5 characters:
	// This is to align the behavior with `True` and `False`, that don't use best fits and are 5 characters long.
	// It allows to avoid [`OptionalParentheses::BestFit`] for most numbers and common identifiers like `self`.
	// The downside is that it can result in short values not being parenthesized if they exceed the line width.
	// This is considered an edge case not worth the performance penalty and IMO, breaking an identifier
	// of 5 characters to avoid it exceeding the line width by 1 reduces the readability.
	// * The text is know to never fit: The text can never fit even when parenthesizing if it is longer
	// than the configured line width (minus indent).
	text_len > 5
	&& text_len
	<= context.options().line_width().value() as usize
	- context.options().indent_width() as usize

My assumptions were:

• This pattern is rare
• Breaking doesn't improve readability significantly for very short right hand sides.

Which is why I ultimately opted for better performance than using best fit for every constant/identifier (which are very common)