Expand if-expressions with leading and trailing comments

[charliermarsh]

Summary

Black appears to expand if-expressions when they contain leading or trailing own-line comments, if they're the only expression in a set of brackets (parenthesized, or the only element in a list or tuple) -- see the playground.

This PR applies the same logic to our own if-expression formatting.

Closes #7066.

No change in similarity:

project	similarity index	total files	changed files
cpython	0.76083	1789	1632
django	0.99957	2760	67
transformers	0.99927	2587	468
twine	0.99982	33	1
typeshed	0.99978	3496	2173
warehouse	0.99818	648	24
zulip	0.99942	1437	32

[charliermarsh]

It appears that this only applies when the if-exp is the only expression within the brackets (i.e., it's a single argument, or a single element in a list literal).

Black's preview style seems more consistent in that it always expands them, but it also parenthesizes whenever they expand (even if it's, e.g., a single function argument).

[charliermarsh]

This probably needs a decision then as to whether we want to implement the preview style (always expand, as we are here, but also parenthesize) or the non-preview style (only expand like this if there are no other elements in the brackets).

[charliermarsh]

Alright, I went with Black's non-preview style.

[charliermarsh]

Here's the change that modified Black's handling in preview mode, to parenthesize these more aggressively: psf/black#2278. (We don't yet implement this, but we could.)

Note that if we implement Black's preview style, we should be sure to avoid a few of these cases of "unnecessary parentheses", which look like they plan to change in Black too: psf/black#3545.

[MichaReiser]

The code change looks good to me but I'm not convinced that we want to implement this behavior.

Our principal is to only expand nodes if their content contain any comments. This isn't the case here because the comment belongs to the ExprIf and not the value. Meaning, we now format ExprIf inconsistently with any other node. I also don't see a reason why splitting helps with readability except if you argue that conditionals should always be split over multiple lines.

This makes me wonder if this is just unintentional formatting in black and whether it is worth aligning our formatting .

[MichaReiser]

We already track whether we are inside of parentheses in NodeLevel

Suggested change

	&& is_expression_bracketed(item.into(), f.context().source())
	&& f.context().node_level().is_parenthesized()

[charliermarsh]

I don’t think this is quite the same — the condition here is “parenthesized or the only item in a set of brackets (e.g., the only item in a tuple literal or list literal or call).”

[MichaReiser]

Suggested change

	/// Returns `true` if the expression is surrounded by brackets (e.g., parenthesized, or the only
	/// expression in a list, tuple, or set).
	fn is_expression_bracketed(expr: ExpressionRef, contents: &str) -> bool {
	let mut tokenizer = SimpleTokenizer::starts_at(expr.end(), contents)
	.skip_trivia()
	.skip_while(|token| matches!(token.kind, SimpleTokenKind::Comma));
	if matches!(
	tokenizer.next(),
	Some(SimpleToken {
	kind: SimpleTokenKind::RParen | SimpleTokenKind::RBrace | SimpleTokenKind::RBracket,
	..
	})
	) {
	let mut tokenizer =
	SimpleTokenizer::up_to_without_back_comment(expr.start(), contents).skip_trivia();
	matches!(
	tokenizer.next_back(),
	Some(SimpleToken {
	kind: SimpleTokenKind::LParen | SimpleTokenKind::LBrace | SimpleTokenKind::LBracket,
	..
	})
	)
	} else {
	false
	}

[charliermarsh]

Yeah, you’re probably right and I don’t feel strongly, I just biased towards fixing a deviation. We can close.

Should we implement Black’s preview style to always parenthesize these when split? It seems like a significant improvement for default function parameters. (Can consider separately.)

[MichaReiser]

Should we implement Black’s preview style to always parenthesize these when split? It seems like a significant improvement for default function parameters. (Can consider separately.)

Preview style formatting is tracked here #6935 and should generally be gated behind a preview flag.