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.

Sign up for GitHub

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

Fix instability with await fluent style #8676

Merged
merged 2 commits into from
Nov 17, 2023
+42 −4
Merged

Fix instability with await fluent style #8676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2191dd8
Fix instability with await fluent style
konstin Nov 14, 2023
a9fbaaa
Add comments about parentheses types
konstin Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/await.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,12 @@
# comment
[foo]
)

# https://github.com/astral-sh/ruff/issues/8644
test_data = await (
Stream.from_async(async_data)
.flat_map_async()
.map()
.filter_async(is_valid_data)
.to_list()
)
2 changes: 1 addition & 1 deletion crates/ruff_python_formatter/src/expression/expr_await.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl FormatNodeRule<ExprAwait> for FormatExprAwait {
[
token("await"),
space(),
maybe_parenthesize_expression(value, item, Parenthesize::IfRequired)
maybe_parenthesize_expression(value, item, Parenthesize::IfBreaks)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be IfBreaksOrIfRequired?

Copy link
Member Author

Choose a reason for hiding this comment

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

That one would regress asyncio.gather

 # Long lines
 async def main():
-    await asyncio.gather(
-        asyncio.sleep(1),
-        asyncio.sleep(1),
-        asyncio.sleep(1),
-        asyncio.sleep(1),
-        asyncio.sleep(1),
-        asyncio.sleep(1),
-        asyncio.sleep(1),
+    await (
+        asyncio.gather(
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+            asyncio.sleep(1),
+        )
     )

(i'm happy too implement a different solution btw, this was just the solution that i found to work best by trial and error)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yeah, I agree that that's worse. I feel like I need to have a better understanding of why these Parenthesize values are leading to these changes before I'd be comfortable approving. Why does IfBreaks break that way? Will using IfBreaks ever lead to syntax errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's handled by OptionalParentheses::Multiline

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make some better examples why we get this behavior

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, I'm trying to make sure I play Micha and really understand the impact of formatting changes before approving.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the perspective of the await, it needs OptionalParentheses::Never because the child is already parenthesized, and we're preferring those parentheses over adding them around the await. For the await's child, we have four options:

  • Optional: Add parentheses if the child breaks, which we want e.g. for asyncio.gather. Keeps the child's parentheses from the source code, which we want to remove.
  • IfBreaks: Add parentheses if the child breaks, which we want e.g. for asyncio.gather. Discards child parentheses, which we want.
  • IfRequired: Discards child parentheses, which we want. Also discards them when the child breaks but has its own inner parentheses, which would lead to e.g. 
    await asyncio.gather(
    fut1,
    fut2,
)
    , which don't want.
  • IfBreaksOrIfRequired: Special case for return type annotations.

I added comments to the parentheses types to make them better understandable in the future

]
)
}
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_python_formatter/src/expression/parentheses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::comments::{
use crate::context::{NodeLevel, WithNodeLevel};
use crate::prelude::*;

/// From the perspective of the expression, under which circumstances does it need parentheses
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub(crate) enum OptionalParentheses {
/// Add parentheses if the expression expands over multiple lines
Expand Down Expand Up @@ -41,7 +42,8 @@ pub(crate) trait NeedsParentheses {
) -> OptionalParentheses;
}

/// Configures if the expression should be parenthesized.
/// From the perspective of the parent statement or expression, when should the child expression
/// get parentheses?
#[derive(Copy, Clone, Debug, PartialEq)]
pub(crate) enum Parenthesize {
/// Parenthesizes the expression if it doesn't fit on a line OR if the expression is parenthesized in the source code.
Expand Down
13 changes: 11 additions & 2 deletions ...n_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ async def main():
```diff
--- Black
+++ Ruff
@@ -21,7 +21,9 @@
@@ -21,11 +21,15 @@

# Check comments
async def main():
Expand All @@ -103,6 +103,13 @@ async def main():
+ )


async def main():
- await asyncio.sleep(1) # Hello
+ await (
+ asyncio.sleep(1) # Hello
+ )
Comment on lines +106 to +110
Copy link
Member Author

Choose a reason for hiding this comment

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

This is arguably correct for ruff

Copy link
Member

Choose a reason for hiding this comment

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

This seems like really weird formatting. Why is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we don't expand, we just keep

async def main():
    await asyncio.sleep(1)  # Hello
    await (
        asyncio.sleep(1)  # Hello
    )

as-is

Copy link
Member

Choose a reason for hiding this comment

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

Okay that's good to know. I guess that's okay. A little weird but fits with previous choices?

Copy link
Member

Choose a reason for hiding this comment

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

Why does this work with IfBreaks?

Copy link
Member

Choose a reason for hiding this comment

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

In yield we use Parenthesize::Optional...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that too for that same reason, but it introduced new black deviations:

 # Remove brackets for short coroutine/task
 async def main():
-    await asyncio.sleep(1)
+    await (asyncio.sleep(1))
 
 
 async def main():
-    await asyncio.sleep(1)
+    await (asyncio.sleep(1))
 
 
 async def main():
-    await asyncio.sleep(1)
+    await (asyncio.sleep(1))



async def main():
```

Expand Down Expand Up @@ -138,7 +145,9 @@ async def main():


async def main():
await asyncio.sleep(1) # Hello
await (
asyncio.sleep(1) # Hello
)


async def main():
Expand Down
18 changes: 18 additions & 0 deletions crates/ruff_python_formatter/tests/snapshots/format@expression__await.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ await (
# comment
[foo]
)

# https://github.com/astral-sh/ruff/issues/8644
test_data = await (
Stream.from_async(async_data)
.flat_map_async()
.map()
.filter_async(is_valid_data)
.to_list()
)
```

## Output
Expand Down Expand Up @@ -122,6 +131,15 @@ await (
# comment
[foo]
)

# https://github.com/astral-sh/ruff/issues/8644
test_data = await (
Stream.from_async(async_data)
.flat_map_async()
.map()
.filter_async(is_valid_data)
.to_list()
)
```


Expand Down
Loading