Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format ExprYield/ExprYieldFrom #5921

Merged
merged 13 commits into from
Jul 21, 2023
Merged

Format ExprYield/ExprYieldFrom #5921

merged 13 commits into from
Jul 21, 2023

Conversation

qdegraaf
Copy link
Contributor

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

@qdegraaf qdegraaf marked this pull request as draft July 20, 2023 14:35
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

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
Copy link
Member

konstin commented Jul 20, 2023

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.

@konstin konstin added the formatter Related to the formatter label Jul 20, 2023
@qdegraaf
Copy link
Contributor Author

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)`.

if parent.is_stmt_return() || parent.is_expr_await() {
OptionalParentheses::Always
} else {
if parent.is_stmt_assign() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

konstin commented Jul 21, 2023

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)

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

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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())
]
)
}
}

@MichaReiser MichaReiser enabled auto-merge (squash) July 21, 2023 11:58
@MichaReiser MichaReiser merged commit 519dbdf into astral-sh:main Jul 21, 2023
@qdegraaf
Copy link
Contributor Author

qdegraaf commented Jul 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter: Yield / YieldFrom (good first issue)
3 participants