Format `ExprYield`/`ExprYieldFrom` by qdegraaf · Pull Request #5921 · astral-sh/ruff

qdegraaf · 2023-07-20T14:34:30Z

Summary

Formats:

yield x
yield from x

expressions

Test Plan

Added fixtures for both expressions, ran black_comparison.

NOTE: This is my first formatter PR. From what I gather there are many many ways that interesting combinations of comments and parentheses can lead to unexpected results. I will try and run the formatter against some large codebases to see if any unexpected results pop up, but figured I'd open it for review in the meantime

Issue link:

Closes: #5916

konstin

nice work

github-actions · 2023-07-20T15:11:28Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.8±0.01ms     4.2 MB/sec    1.15     11.2±0.02ms     3.6 MB/sec
formatter/numpy/ctypeslib.py               1.00   1901.1±5.20µs     8.8 MB/sec    1.12      2.1±0.00ms     7.8 MB/sec
formatter/numpy/globals.py                 1.00    206.1±0.40µs    14.3 MB/sec    1.08    222.6±0.38µs    13.3 MB/sec
formatter/pydantic/types.py                1.00      4.2±0.01ms     6.0 MB/sec    1.12      4.7±0.00ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.02     13.8±0.02ms     3.0 MB/sec    1.00     13.6±0.04ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.5±0.00ms     4.8 MB/sec    1.00      3.4±0.01ms     4.8 MB/sec
linter/all-rules/numpy/globals.py          1.01    377.1±1.54µs     7.8 MB/sec    1.00    373.2±1.32µs     7.9 MB/sec
linter/all-rules/pydantic/types.py         1.02      6.2±0.01ms     4.1 MB/sec    1.00      6.1±0.01ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.04      7.3±0.01ms     5.6 MB/sec    1.00      7.0±0.01ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02   1465.9±3.97µs    11.4 MB/sec    1.00   1435.4±3.52µs    11.6 MB/sec
linter/default-rules/numpy/globals.py      1.02    153.1±0.19µs    19.3 MB/sec    1.00    149.8±0.27µs    19.7 MB/sec
linter/default-rules/pydantic/types.py     1.03      3.2±0.02ms     7.9 MB/sec    1.00      3.1±0.00ms     8.1 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.8±0.09ms     3.8 MB/sec    1.00     10.8±0.09ms     3.8 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.1±0.03ms     7.9 MB/sec    1.00      2.1±0.02ms     7.9 MB/sec
formatter/numpy/globals.py                 1.00    236.4±5.45µs    12.5 MB/sec    1.02   240.2±12.21µs    12.3 MB/sec
formatter/pydantic/types.py                1.00      4.6±0.04ms     5.5 MB/sec    1.01      4.7±0.06ms     5.5 MB/sec
linter/all-rules/large/dataset.py          1.00     15.1±0.08ms     2.7 MB/sec    1.00     15.0±0.10ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.0±0.05ms     4.2 MB/sec    1.00      4.0±0.03ms     4.2 MB/sec
linter/all-rules/numpy/globals.py          1.00    484.3±8.64µs     6.1 MB/sec    1.00    482.8±5.97µs     6.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.09ms     3.7 MB/sec    1.00      6.8±0.05ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.9±0.05ms     5.2 MB/sec    1.00      7.9±0.06ms     5.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1646.7±15.45µs    10.1 MB/sec    1.00  1648.8±14.28µs    10.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    188.9±2.26µs    15.6 MB/sec    1.00    189.3±2.37µs    15.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.5±0.03ms     7.3 MB/sec    1.00      3.5±0.02ms     7.3 MB/sec

konstin · 2023-07-20T15:11:57Z

The cpython check failed on Lib/test/test_asyncio/test_locks.py, which can be shrunk to the following snippet:

def a():
    with (yield):
        pass

Seem like yield can appear everywhere, even in e.g. if. Searching for yield_expression in https://docs.python.org/3/reference/grammar.html it seems that yield without parentheses is the exception (only for yield statements and right hand sides of assignment), in atom (through group) it has parentheses.

qdegraaf · 2023-07-20T22:03:02Z

Interesting. I've expanded the test cases with a bunch more examples, and inverted the needs_parentheses() logic, excluding only parents which are some type of assign

You mention yield statements also require no parentheses. How do you mean exactly? Looking at it now yield (yield l), yield from (yield l) yield (yield from l) and yield from (yield from l) all seem to need parentheses)`.

konstin · 2023-07-21T10:10:50Z

crates/ruff_python_formatter/src/expression/expr_yield.rs

-        if parent.is_stmt_return() || parent.is_expr_await() {
-            OptionalParentheses::Always
-        } else {
+        if parent.is_stmt_assign() {


This should get a longer explanation about how only assignment right hand side and yield statements don't need parentheses and the special casing in the grammar, but we only need to handle assignment RHS here because StmtExpr doesn't add parentheses anyway (i just had to check that last part myself)

Good point, added a comment describing the situation for both expr_yield and expr_yield_from. Thanks for the explanation in #5921 (comment) as well, helps me get a clearer image of how the formatter is structured and what to watch out for.

konstin · 2023-07-21T10:20:29Z

You mention yield statements also require no parentheses. How do you mean exactly? Looking at it now yield (yield l), yield from (yield l) yield (yield from l) and yield from (yield from l) all seem to need parentheses)`.

For

def f():
    yield 1

you get (simplified for readability) StmtFunctionDef { body: [StmtExpr(ExprYield(..))] }. StmtExpr gets formatted with

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

        if is_arithmetic_like(value) {
            maybe_parenthesize_expression(value, item, Parenthesize::Optional).fmt(f)
        } else {
            value.format().fmt(f)
        }
    }
}

so this doesn't become

def f():
    (yield 1)

MichaReiser

This is awesome. We may want to consider unifying the implementation in a follow up PR similar to AnyStmtWith (it could also implement NeedsParentheses so that we have all the logic in a single place).

ruff/crates/ruff_python_formatter/src/statement/stmt_with.rs

Lines 15 to 114 in 0301889

    
           pub(super) enum AnyStatementWith<'a> { 
        
               With(&'a StmtWith), 
        
               AsyncWith(&'a StmtAsyncWith), 
        
           } 
        
           impl<'a> AnyStatementWith<'a> { 
        
               const fn is_async(&self) -> bool { 
        
                   matches!(self, AnyStatementWith::AsyncWith(_)) 
        
               } 
        
               fn items(&self) -> &[WithItem] { 
        
                   match self { 
        
                       AnyStatementWith::With(with) => with.items.as_slice(), 
        
                       AnyStatementWith::AsyncWith(with) => with.items.as_slice(), 
        
                   } 
        
               } 
        
               fn body(&self) -> &Suite { 
        
                   match self { 
        
                       AnyStatementWith::With(with) => &with.body, 
        
                       AnyStatementWith::AsyncWith(with) => &with.body, 
        
                   } 
        
               } 
        
           } 
        
           impl Ranged for AnyStatementWith<'_> { 
        
               fn range(&self) -> TextRange { 
        
                   match self { 
        
                       AnyStatementWith::With(with) => with.range(), 
        
                       AnyStatementWith::AsyncWith(with) => with.range(), 
        
                   } 
        
               } 
        
           } 
        
           impl<'a> From<&'a StmtWith> for AnyStatementWith<'a> { 
        
               fn from(value: &'a StmtWith) -> Self { 
        
                   AnyStatementWith::With(value) 
        
               } 
        
           } 
        
           impl<'a> From<&'a StmtAsyncWith> for AnyStatementWith<'a> { 
        
               fn from(value: &'a StmtAsyncWith) -> Self { 
        
                   AnyStatementWith::AsyncWith(value) 
        
               } 
        
           } 
        
           impl<'a> From<&AnyStatementWith<'a>> for AnyNodeRef<'a> { 
        
               fn from(value: &AnyStatementWith<'a>) -> Self { 
        
                   match value { 
        
                       AnyStatementWith::With(with) => AnyNodeRef::StmtWith(with), 
        
                       AnyStatementWith::AsyncWith(with) => AnyNodeRef::StmtAsyncWith(with), 
        
                   } 
        
               } 
        
           } 
        
           impl Format<PyFormatContext<'_>> for AnyStatementWith<'_> { 
        
               fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> { 
        
                   let comments = f.context().comments().clone(); 
        
                   let dangling_comments = comments.dangling_comments(self); 
        
                   write!( 
        
                       f, 
        
                       [ 
        
                           self.is_async() 
        
                               .then_some(format_args![text("async"), space()]), 
        
                           text("with"), 
        
                           space() 
        
                       ] 
        
                   )?; 
        
                   if are_with_items_parenthesized(self, f.context())? { 
        
                       optional_parentheses(&format_with(|f| { 
        
                           let mut joiner = f.join_comma_separated(self.body().first().unwrap().start()); 
        
                           for item in self.items() { 
        
                               joiner.entry_with_line_separator( 
        
                                   item, 
        
                                   &item.format(), 
        
                                   in_parentheses_only_soft_line_break_or_space(), 
        
                               ); 
        
                           } 
        
                           joiner.finish() 
        
                       })) 
        
                       .fmt(f)?; 
        
                   } else { 
        
                       f.join_with(format_args![text(","), space()]) 
        
                           .entries(self.items().iter().formatted()) 
        
                           .finish()?; 
        
                   } 
        
                   write!( 
        
                       f, 
        
                       [ 
        
                           text(":"), 
        
                           trailing_comments(dangling_comments), 
        
                           block_indent(&self.body().format()) 
        
                       ] 
        
                   ) 
        
               } 
        
           }

crates/ruff_python_formatter/src/expression/expr_yield.rs

crates/ruff_python_formatter/src/expression/expr_yield_from.rs

qdegraaf · 2023-07-21T13:01:42Z

This is awesome. We may want to consider unifying the implementation in a follow up PR similar to AnyStmtWith (it could also implement NeedsParentheses so that we have all the logic in a single place).

Good idea @MichaReiser. I'll finish up my outstanding PRs, then move to work on that.

qdegraaf marked this pull request as draft July 20, 2023 14:35

konstin approved these changes Jul 20, 2023

View reviewed changes

konstin added the formatter Related to the formatter label Jul 20, 2023

konstin reviewed Jul 21, 2023

View reviewed changes

qdegraaf marked this pull request as ready for review July 21, 2023 10:56

qdegraaf added 11 commits July 21, 2023 12:56

[WIP] Add FormatExprYield and FormatExprYieldFrom

ddca7a7

Expand NeedsParentheses if parent is await or return

91f07ed

Remove unnecessary FormatRuleWithOptions review

0f047c0

cargo fmt && cargo clippy

e4653ca

rebase

17cf058

cargo fmt

1ad7ff4

Remove Yield token

207007f

Invert parenthesis logic, expand test cases

07454cb

Expand assignment checks

8ed8a31

Add comments to needs_parentheses() funcs

76bc00f

rebase

1e2d8ad

MichaReiser approved these changes Jul 21, 2023

View reviewed changes

crates/ruff_python_formatter/src/expression/expr_yield.rs Outdated Show resolved Hide resolved

crates/ruff_python_formatter/src/expression/expr_yield_from.rs Outdated Show resolved Hide resolved

MichaReiser added 2 commits July 21, 2023 13:58

Update crates/ruff_python_formatter/src/expression/expr_yield.rs

2546875

Update crates/ruff_python_formatter/src/expression/expr_yield_from.rs

de94c9e

MichaReiser enabled auto-merge (squash) July 21, 2023 11:58

MichaReiser merged commit 519dbdf into astral-sh:main Jul 21, 2023

qdegraaf deleted the feature/format-yield branch July 27, 2023 08:12

qdegraaf mentioned this pull request Jul 27, 2023

Consolidate ExprYield/ExprYieldFrom into AnyExpressionYield #6121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format `ExprYield`/`ExprYieldFrom`#5921

Format `ExprYield`/`ExprYieldFrom`#5921
MichaReiser merged 13 commits intoastral-sh:mainfrom
qdegraaf:feature/format-yield

qdegraaf commented Jul 20, 2023

Uh oh!

konstin left a comment

Uh oh!

github-actions bot commented Jul 20, 2023 •

edited

Loading

Uh oh!

konstin commented Jul 20, 2023

Uh oh!

qdegraaf commented Jul 20, 2023

Uh oh!

konstin Jul 21, 2023

Uh oh!

qdegraaf Jul 21, 2023

Uh oh!

konstin commented Jul 21, 2023

Uh oh!

MichaReiser left a comment

Uh oh!

Uh oh!

Uh oh!

qdegraaf commented Jul 21, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub(super) enum AnyStatementWith<'a> {
	With(&'a StmtWith),
	AsyncWith(&'a StmtAsyncWith),
	}

	impl<'a> AnyStatementWith<'a> {
	const fn is_async(&self) -> bool {
	matches!(self, AnyStatementWith::AsyncWith(_))
	}

	fn items(&self) -> &[WithItem] {
	match self {
	AnyStatementWith::With(with) => with.items.as_slice(),
	AnyStatementWith::AsyncWith(with) => with.items.as_slice(),
	}
	}

	fn body(&self) -> &Suite {
	match self {
	AnyStatementWith::With(with) => &with.body,
	AnyStatementWith::AsyncWith(with) => &with.body,
	}
	}
	}

	impl Ranged for AnyStatementWith<'_> {
	fn range(&self) -> TextRange {
	match self {
	AnyStatementWith::With(with) => with.range(),
	AnyStatementWith::AsyncWith(with) => with.range(),
	}
	}
	}

	impl<'a> From<&'a StmtWith> for AnyStatementWith<'a> {
	fn from(value: &'a StmtWith) -> Self {
	AnyStatementWith::With(value)
	}
	}

	impl<'a> From<&'a StmtAsyncWith> for AnyStatementWith<'a> {
	fn from(value: &'a StmtAsyncWith) -> Self {
	AnyStatementWith::AsyncWith(value)
	}
	}

	impl<'a> From<&AnyStatementWith<'a>> for AnyNodeRef<'a> {
	fn from(value: &AnyStatementWith<'a>) -> Self {
	match value {
	AnyStatementWith::With(with) => AnyNodeRef::StmtWith(with),
	AnyStatementWith::AsyncWith(with) => AnyNodeRef::StmtAsyncWith(with),
	}
	}
	}

	impl Format<PyFormatContext<'_>> for AnyStatementWith<'_> {
	fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> {
	let comments = f.context().comments().clone();
	let dangling_comments = comments.dangling_comments(self);

	write!(
	f,
	[
	self.is_async()
	.then_some(format_args![text("async"), space()]),
	text("with"),
	space()
	]
	)?;

	if are_with_items_parenthesized(self, f.context())? {
	optional_parentheses(&format_with(\|f\| {
	let mut joiner = f.join_comma_separated(self.body().first().unwrap().start());

	for item in self.items() {
	joiner.entry_with_line_separator(
	item,
	&item.format(),
	in_parentheses_only_soft_line_break_or_space(),
	);
	}
	joiner.finish()
	}))
	.fmt(f)?;
	} else {
	f.join_with(format_args![text(","), space()])
	.entries(self.items().iter().formatted())
	.finish()?;
	}

	write!(
	f,
	[
	text(":"),
	trailing_comments(dangling_comments),
	block_indent(&self.body().format())
	]
	)
	}
	}

Conversation

qdegraaf commented Jul 20, 2023

Summary

Test Plan

Issue link:

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Benchmark

Linux

Windows

Uh oh!

konstin commented Jul 20, 2023

Uh oh!

qdegraaf commented Jul 20, 2023

Uh oh!

konstin Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

qdegraaf Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

konstin commented Jul 21, 2023

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

qdegraaf commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jul 20, 2023 •

edited

Loading

qdegraaf commented Jul 21, 2023 •

edited

Loading