mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-03 18:28:24 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[flake8-pyi] Avoid syntax error in the case of starred and keyword arguments (PYI059) (#18611)

Browse source 
## Summary

Fixes https://github.com/astral-sh/ruff/issues/18602 by:
1. Avoiding a fix when `*args` are present
2. Inserting the `Generic` base class right before the first keyword
argument, if one is present

In an intermediate commit, I also had special handling to avoid a fix in
the `**kwargs` case, but this is treated (roughly) as a normal keyword,
and I believe handling it properly falls out of the other keyword fix.

I also updated the `add_argument` utility function to insert new
arguments right before the keyword argument list instead of at the very
end of the argument list. This changed a couple of snapshots unrelated
to `PYI059`, but there shouldn't be any functional changes to other
rules because all other calls to `add_argument` were adding a keyword
argument anyway.

## Test Plan

Existing PYI059 cases, plus new tests based on the issue

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This commit is contained in:
Brent Westbrook 2025-06-10 12:27:06 -04:00 • committed by GitHub
parent 161446a47a
commit 6051a118d1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 122 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

12
crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py vendored
View file

@ -52,3 +52,15 @@ class MyList(Sized, Generic[T]): # Generic already in last place
class SomeGeneric(Generic[T]): # Only one generic
pass
# syntax errors with starred and keyword arguments from
# https://github.com/astral-sh/ruff/issues/18602
class C1(Generic[T], str, **{"metaclass": type}): # PYI059
...
class C2(Generic[T], str, metaclass=type): # PYI059
...
class C3(Generic[T], metaclass=type, *[str]): # PYI059 but no fix
...

16
crates/ruff_linter/src/fix/edits.rs
View file

@ -2,9 +2,9 @@
use anyhow::{Context, Result};
use ruff_python_ast::AnyNodeRef;
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Expr, ExprList, Parameters, Stmt};
use ruff_python_ast::{AnyNodeRef, ArgOrKeyword};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_trivia::textwrap::dedent_to;
@ -257,19 +257,23 @@ pub(crate) fn remove_argument<T: Ranged>(
}
/// Generic function to add arguments or keyword arguments to function calls.
///
/// The new argument will be inserted before the first existing keyword argument in `arguments`, if
/// there are any present. Otherwise, the new argument is added to the end of the argument list.
pub(crate) fn add_argument(
argument: &str,
arguments: &Arguments,
comment_ranges: &CommentRanges,
source: &str,
) -> Edit {
if let Some(last) = arguments.arguments_source_order().last() {
if let Some(ast::Keyword { range, value, .. }) = arguments.keywords.first() {
let keyword = parenthesized_range(value.into(), arguments.into(), comment_ranges, source)
.unwrap_or(*range);
Edit::insertion(format!("{argument}, "), keyword.start())
} else if let Some(last) = arguments.arguments_source_order().last() {
// Case 1: existing arguments, so append after the last argument.
let last = parenthesized_range(
match last {
ArgOrKeyword::Arg(arg) => arg.into(),
ArgOrKeyword::Keyword(keyword) => (&keyword.value).into(),
},
last.value().into(),
arguments.into(),
comment_ranges,
source,

9
crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B028_B028.py.snap
View file

@ -37,11 +37,10 @@ B028.py:9:1: B028 [*] No explicit `stacklevel` keyword argument found
7 7 |
8 8 | warnings.warn("test", DeprecationWarning)
9 |-warnings.warn("test", DeprecationWarning, source=None)
10 9 | warnings.warn("test", DeprecationWarning, source=None, stacklevel=2)
10 |+warnings.warn("test", DeprecationWarning, source=None, stacklevel=2)
9 |+warnings.warn("test", DeprecationWarning, stacklevel=2, source=None)
10 10 | warnings.warn("test", DeprecationWarning, source=None, stacklevel=2)
11 11 | warnings.warn("test", DeprecationWarning, stacklevel=1)
12 12 | warnings.warn("test", DeprecationWarning, 1)
13 13 | warnings.warn("test", category=DeprecationWarning, stacklevel=1)
B028.py:22:1: B028 [*] No explicit `stacklevel` keyword argument found
|
@ -59,7 +58,7 @@ B028.py:22:1: B028 [*] No explicit `stacklevel` keyword argument found
24 24 | DeprecationWarning,
25 25 | # some comments here
26 |- source = None # no trailing comma
26 |+ source = None, stacklevel=2 # no trailing comma
26 |+ stacklevel=2, source = None # no trailing comma
27 27 | )
28 28 |
29 29 | # https://github.com/astral-sh/ruff/issues/18011
@ -80,7 +79,7 @@ B028.py:32:1: B028 [*] No explicit `stacklevel` keyword argument found
30 30 | warnings.warn("test", skip_file_prefixes=(os.path.dirname(__file__),))
31 31 | # trigger diagnostic if `skip_file_prefixes` is present and set to the default value
32 |-warnings.warn("test", skip_file_prefixes=())
32 |+warnings.warn("test", skip_file_prefixes=(), stacklevel=2)
32 |+warnings.warn("test", stacklevel=2, skip_file_prefixes=())
33 33 |
34 34 | _my_prefixes = ("this","that")
35 35 | warnings.warn("test", skip_file_prefixes = _my_prefixes)

38
crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs
View file

@ -13,8 +13,11 @@ use crate::{Fix, FixAvailability, Violation};
/// ## Why is this bad?
/// If `Generic[]` is not the final class in the bases tuple, unexpected
/// behaviour can occur at runtime (See [this CPython issue][1] for an example).
/// The rule is also applied to stub files, but, unlike at runtime,
/// in stubs it is purely enforced for stylistic consistency.
///
/// The rule is also applied to stub files, where it won't cause issues at
/// runtime. This is because type checkers may not be able to infer an
/// accurate [MRO] for the class, which could lead to unexpected or
/// inaccurate results when they analyze your code.
///
/// For example:
/// ```python
@ -43,10 +46,23 @@ use crate::{Fix, FixAvailability, Violation};
/// ):
/// ...
/// ```
///
/// ## Fix safety
///
/// This rule's fix is always unsafe because reordering base classes can change
/// the behavior of the code by modifying the class's MRO. The fix will also
/// delete trailing comments after the `Generic` base class in multi-line base
/// class lists, if any are present.
///
/// ## Fix availability
///
/// This rule's fix is only available when there are no `*args` present in the base class list.
///
/// ## References
/// - [`typing.Generic` documentation](https://docs.python.org/3/library/typing.html#typing.Generic)
///
/// [1]: https://github.com/python/cpython/issues/106102
/// [MRO]: https://docs.python.org/3/glossary.html#term-method-resolution-order
#[derive(ViolationMetadata)]
pub(crate) struct GenericNotLastBaseClass;
@ -94,6 +110,22 @@ pub(crate) fn generic_not_last_base_class(checker: &Checker, class_def: &ast::St
let mut diagnostic = checker.report_diagnostic(GenericNotLastBaseClass, bases.range());
// Avoid suggesting a fix if any of the arguments is starred. This avoids tricky syntax errors
// in cases like
//
// ```python
// class C3(Generic[T], metaclass=type, *[str]): ...
// ```
//
// where we would naively try to put `Generic[T]` after `*[str]`, which is also after a keyword
// argument, causing the error.
if bases
.arguments_source_order()
.any(|arg| arg.value().is_starred_expr())
{
return;
}
// No fix if multiple `Generic[]`s are seen in the class bases.
if generic_base_iter.next().is_none() {
diagnostic.try_set_fix(|| generate_fix(generic_base, bases, checker));
@ -116,5 +148,5 @@ fn generate_fix(
source,
);
Ok(Fix::safe_edits(deletion, [insertion]))
Ok(Fix::unsafe_edits(deletion, [insertion]))
}

58
crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap
View file

@ -12,7 +12,7 @@ PYI059.py:8:17: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
5 5 | K = TypeVar('K')
6 6 | V = TypeVar('V')
7 7 |
@ -37,7 +37,7 @@ PYI059.py:15:16: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
13 13 | self._items.append(item)
14 14 |
15 15 | class MyMapping( # PYI059
@ -59,7 +59,7 @@ PYI059.py:26:10: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
23 23 | # Inheriting from just `Generic` is a TypeError, but it's probably fine
24 24 | # to flag this issue in this case as well, since after fixing the error
25 25 | # the Generic's position issue persists.
@ -88,7 +88,7 @@ PYI059.py:30:10: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
30 30 | class Foo( # comment about the bracket
31 31 | # Part 1 of multiline comment 1
32 32 | # Part 2 of multiline comment 1
@ -111,3 +111,53 @@ PYI059.py:45:8: PYI059 `Generic[]` should always be the last base class
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059
|
= help: Move `Generic[]` to the end
PYI059.py:59:9: PYI059 [*] `Generic[]` should always be the last base class
|
57 | # syntax errors with starred and keyword arguments from
58 | # https://github.com/astral-sh/ruff/issues/18602
59 | class C1(Generic[T], str, **{"metaclass": type}): # PYI059
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059
60 | ...
|
= help: Move `Generic[]` to the end
ℹ Unsafe fix
56 56 |
57 57 | # syntax errors with starred and keyword arguments from
58 58 | # https://github.com/astral-sh/ruff/issues/18602
59 |-class C1(Generic[T], str, **{"metaclass": type}): # PYI059
59 |+class C1(str, Generic[T], **{"metaclass": type}): # PYI059
60 60 | ...
61 61 |
62 62 | class C2(Generic[T], str, metaclass=type): # PYI059
PYI059.py:62:9: PYI059 [*] `Generic[]` should always be the last base class
|
60 | ...
61 |
62 | class C2(Generic[T], str, metaclass=type): # PYI059
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059
63 | ...
|
= help: Move `Generic[]` to the end
ℹ Unsafe fix
59 59 | class C1(Generic[T], str, **{"metaclass": type}): # PYI059
60 60 | ...
61 61 |
62 |-class C2(Generic[T], str, metaclass=type): # PYI059
62 |+class C2(str, Generic[T], metaclass=type): # PYI059
63 63 | ...
64 64 |
65 65 | class C3(Generic[T], metaclass=type, *[str]): # PYI059 but no fix
PYI059.py:65:9: PYI059 `Generic[]` should always be the last base class
|
63 | ...
64 |
65 | class C3(Generic[T], metaclass=type, *[str]): # PYI059 but no fix
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059
66 | ...
|
= help: Move `Generic[]` to the end

8
crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap
View file

@ -12,7 +12,7 @@ PYI059.pyi:8:17: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
5 5 | K = TypeVar('K')
6 6 | V = TypeVar('V')
7 7 |
@ -37,7 +37,7 @@ PYI059.pyi:12:16: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
10 10 | def push(self, item: T) -> None: ...
11 11 |
12 12 | class MyMapping( # PYI059
@ -58,7 +58,7 @@ PYI059.pyi:22:10: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
19 19 | # Inheriting from just `Generic` is a TypeError, but it's probably fine
20 20 | # to flag this issue in this case as well, since after fixing the error
21 21 | # the Generic's position issue persists.
@ -87,7 +87,7 @@ PYI059.pyi:25:10: PYI059 [*] `Generic[]` should always be the last base class
|
= help: Move `Generic[]` to the end
ℹ Safe fix
ℹ Unsafe fix
25 25 | class Foo( # comment about the bracket
26 26 | # Part 1 of multiline comment 1
27 27 | # Part 2 of multiline comment 1

6
crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap
View file

@ -37,7 +37,7 @@ subprocess_run_without_check.py:5:1: PLW1510 [*] `subprocess.run` without explic
3 3 | # Errors.
4 4 | subprocess.run("ls")
5 |-subprocess.run("ls", shell=True)
5 |+subprocess.run("ls", shell=True, check=False)
5 |+subprocess.run("ls", check=False, shell=True)
6 6 | subprocess.run(
7 7 | ["ls"],
8 8 | shell=False,
@ -58,7 +58,7 @@ subprocess_run_without_check.py:6:1: PLW1510 [*] `subprocess.run` without explic
6 6 | subprocess.run(
7 7 | ["ls"],
8 |- shell=False,
8 |+ shell=False, check=False,
8 |+ check=False, shell=False,
9 9 | )
10 10 | subprocess.run(["ls"], **kwargs)
11 11 |
@ -79,7 +79,7 @@ subprocess_run_without_check.py:10:1: PLW1510 [*] `subprocess.run` without expli
8 8 | shell=False,
9 9 | )
10 |-subprocess.run(["ls"], **kwargs)
10 |+subprocess.run(["ls"], **kwargs, check=False)
10 |+subprocess.run(["ls"], check=False, **kwargs)
11 11 |
12 12 | # Non-errors.
13 13 | subprocess.run("ls", check=True)