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

[flake8-annotations] Correct syntax for typing.Union in suggested return type fixes for ANN20x rules #16025

Merged
merged 3 commits into from
Feb 7, 2025
Merged

[flake8-annotations] Correct syntax for typing.Union in suggested return type fixes for ANN20x rules #16025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7724ca9
commas not pipes in typing.Union
dylwil3 Feb 7, 2025
e2d717d
update snapshot
dylwil3 Feb 7, 2025
5bf82d0
add warning comment about empty unions
dylwil3 Feb 7, 2025
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
21 changes: 10 additions & 11 deletions ...tions/snapshots/ruff_linter__rules__flake8_annotations__tests__auto_return_type_py38.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/flake8_annotations/mod.rs
snapshot_kind: text
---
auto_return_type.py:1:5: ANN201 [*] Missing return type annotation for public function `func`
|
Expand Down Expand Up @@ -97,7 +96,7 @@ auto_return_type.py:27:5: ANN201 [*] Missing return type annotation for public f
| ^^^^ ANN201
28 | return 1 or 2.5 if x > 0 else 1.5 or "str"
|
= help: Add return type annotation: `Union[str | float]`
= help: Add return type annotation: `Union[str, float]`

ℹ Unsafe fix
1 |+from typing import Union
Expand All @@ -109,7 +108,7 @@ auto_return_type.py:27:5: ANN201 [*] Missing return type annotation for public f
25 26 |
26 27 |
27 |-def func(x: int):
28 |+def func(x: int) -> Union[str | float]:
28 |+def func(x: int) -> Union[str, float]:
28 29 | return 1 or 2.5 if x > 0 else 1.5 or "str"
29 30 |
30 31 |
Expand All @@ -120,7 +119,7 @@ auto_return_type.py:31:5: ANN201 [*] Missing return type annotation for public f
| ^^^^ ANN201
32 | return 1 + 2.5 if x > 0 else 1.5 or "str"
|
= help: Add return type annotation: `Union[str | float]`
= help: Add return type annotation: `Union[str, float]`

ℹ Unsafe fix
1 |+from typing import Union
Expand All @@ -132,7 +131,7 @@ auto_return_type.py:31:5: ANN201 [*] Missing return type annotation for public f
29 30 |
30 31 |
31 |-def func(x: int):
32 |+def func(x: int) -> Union[str | float]:
32 |+def func(x: int) -> Union[str, float]:
32 33 | return 1 + 2.5 if x > 0 else 1.5 or "str"
33 34 |
34 35 |
Expand Down Expand Up @@ -204,7 +203,7 @@ auto_return_type.py:59:5: ANN201 [*] Missing return type annotation for public f
60 | if not x:
61 | return 1
|
= help: Add return type annotation: `Union[str | int | None]`
= help: Add return type annotation: `Union[str, int, None]`

ℹ Unsafe fix
1 |+from typing import Union
Expand All @@ -216,7 +215,7 @@ auto_return_type.py:59:5: ANN201 [*] Missing return type annotation for public f
57 58 |
58 59 |
59 |-def func(x: int):
60 |+def func(x: int) -> Union[str | int | None]:
60 |+def func(x: int) -> Union[str, int, None]:
60 61 | if not x:
61 62 | return 1
62 63 | elif x > 5:
Expand Down Expand Up @@ -294,7 +293,7 @@ auto_return_type.py:82:5: ANN201 [*] Missing return type annotation for public f
83 | match x:
84 | case [1, 2, 3]:
|
= help: Add return type annotation: `Union[str | int | None]`
= help: Add return type annotation: `Union[str, int, None]`

ℹ Unsafe fix
1 |+from typing import Union
Expand All @@ -306,7 +305,7 @@ auto_return_type.py:82:5: ANN201 [*] Missing return type annotation for public f
80 81 |
81 82 |
82 |-def func(x: int):
83 |+def func(x: int) -> Union[str | int | None]:
83 |+def func(x: int) -> Union[str, int, None]:
83 84 | match x:
84 85 | case [1, 2, 3]:
85 86 | return 1
Expand Down Expand Up @@ -853,7 +852,7 @@ auto_return_type.py:299:5: ANN201 [*] Missing return type annotation for public
300 | match x:
301 | case [1, 2, 3]:
|
= help: Add return type annotation: `Union[str | int]`
= help: Add return type annotation: `Union[str, int]`

ℹ Unsafe fix
214 214 | return 1
Expand All @@ -869,7 +868,7 @@ auto_return_type.py:299:5: ANN201 [*] Missing return type annotation for public
297 297 |
298 298 |
299 |-def func(x: int):
299 |+def func(x: int) -> Union[str | int]:
299 |+def func(x: int) -> Union[str, int]:
300 300 | match x:
301 301 | case [1, 2, 3]:
302 302 | return 1
31 changes: 10 additions & 21 deletions crates/ruff_python_ast/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1437,33 +1437,22 @@ pub fn typing_optional(elt: Expr, binding: Name) -> Expr {
}

/// Format the expressions as a `typing.Union`-style union.
///
/// Note: It is a syntax error to have `Union[]` so the caller
/// should ensure that the `elts` argument is nonempty.
pub fn typing_union(elts: &[Expr], binding: Name) -> Expr {
fn tuple(elts: &[Expr], binding: Name) -> Expr {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with the code so this is probably a dumb question but the the old implementation applied this function recursively. Is it correct that this is no longer needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was also confused by the code that was there before... At the very least this was a problem:

            [rest @ .., elt] => Expr::BinOp(ast::ExprBinOp {
                left: Box::new(tuple(rest, binding)),
                op: Operator::BitOr,
                right: Box::new(elt.clone()),
                range: TextRange::default(),
            }),

I will try to find an MRE for the difference involving the recursion... but I can't figure out why it would be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I think the recursion is there just because they were trying to get something like:

Union[int | str | float]

which is built out of binary operations, so they had to do them one at a time.

But since we're doing:

Union[int, str, float]

we can just plop down all the elements at once.

The other confusing thing is the treatment of the empty case. An empty Union is either a SyntaxError (if you do Union[]) or a TypeError (if you do Union[()]). So that's the other difference: the modified function will produce a SyntaxError where the previous produced a TypeError. But really the function should not be called at all when you have empty elts.

I could:

  • make that the responsibility of the caller
  • change the return signature to Result
  • add a debug_assert
  • something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@charliermarsh if you happen to remember you're reasoning from #8885 and see that I'm missing something, please let me know 😄

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it's hard to recall without digging deeper. But, yes, I'm guessing it's something to do with the recursive nature of the | operator. Notice that this is very similar to the pep_604_union version above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries!

match elts {
[] => Expr::Tuple(ast::ExprTuple {
elts: vec![],
ctx: ExprContext::Load,
range: TextRange::default(),
parenthesized: true,
}),
[Expr::Tuple(ast::ExprTuple { elts, .. })] => typing_union(elts, binding),
[elt] => elt.clone(),
[rest @ .., elt] => Expr::BinOp(ast::ExprBinOp {
left: Box::new(tuple(rest, binding)),
op: Operator::BitOr,
right: Box::new(elt.clone()),
range: TextRange::default(),
}),
}
}

Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: binding.clone(),
id: binding,
range: TextRange::default(),
ctx: ExprContext::Load,
})),
slice: Box::new(Expr::Tuple(ast::ExprTuple {
range: TextRange::default(),
elts: elts.to_vec(),
ctx: ExprContext::Load,
parenthesized: false,
})),
slice: Box::new(tuple(elts, binding)),
ctx: ExprContext::Load,
range: TextRange::default(),
})
Expand Down
Loading