diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py index 0e4d877d4b485..cfe42a12d3836 100644 --- 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 index 7d32438022985..2ed6f4c9cf7e3 100644 --- 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( } /// 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 index db323efe66afb..ddaf48098f757 100644 --- 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 index b7ab81f3d315b..4d40bfafd94d0 100644 --- 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 index bc91fb867cef4..d5f7860983c0c 100644 --- 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 index 275dd20dbaa08..16c5c72d08705 100644 --- 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 index 7e6539ec04c0b..b0d1645a5d07f 100644 --- 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)