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

ntBre · AlexWaygood · web-flow · commit 6051a118d163 · 2025-06-10T12:27:06.000-04:00
## Summary Fixes #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>
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py
@@ -52,3 +52,15 @@ def __init__(self) -> None:
 
 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
+    ...
diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs
@@ -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,
diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B028_B028.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B028_B028.py.snap
@@ -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)
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs
@@ -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]))
 }
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap
@@ -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
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap
@@ -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
diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1510_subprocess_run_without_check.py.snap
@@ -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)
