From 6bb126415d650f4842c3e83dfc752fee689afc79 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jan 2024 22:44:49 +0000 Subject: [PATCH] RUF023: Don't sort `__match_args__`, only `__slots__` (#9724) Fixes #9723. I'm pretty embarrassed I forgot that order was important here :( --- .../resources/test/fixtures/ruff/RUF023.py | 33 ++++--- .../src/rules/ruff/rules/sequence_sorting.rs | 2 +- .../src/rules/ruff/rules/sort_dunder_slots.rs | 88 ++++++------------- ..._rules__ruff__tests__RUF023_RUF023.py.snap | 82 ++++++++--------- 4 files changed, 82 insertions(+), 123 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py index 3a9bd1310ef45..f2c4383f1d32c 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py @@ -4,14 +4,14 @@ class Klass: __slots__ = ["d", "c", "b", "a"] # a comment that is untouched - __match_args__ = ("d", "c", "b", "a") + __slots__ = ("d", "c", "b", "a") # Quoting style is retained, # but unnecessary parens are not __slots__: set = {'b', "c", ((('a')))} # Trailing commas are also not retained for single-line definitions # (but they are in multiline definitions) - __match_args__: tuple = ("b", "c", "a",) + __slots__: tuple = ("b", "c", "a",) class Klass2: if bool(): @@ -19,7 +19,7 @@ class Klass2: else: __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) - __match_args__: list[str] = ["the", "three", "little", "pigs"] + __slots__: list[str] = ["the", "three", "little", "pigs"] __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") # we use natural sort, # not alphabetical sort or "isort-style" sort @@ -37,7 +37,7 @@ class Klass3: # a comment regarding 'a0': "a0" ) - __match_args__ = [ + __slots__ = [ "d", "c", # a comment regarding 'c' "b", @@ -61,7 +61,7 @@ class Klass4: ) # comment6 # comment7 - __match_args__ = [ # comment0 + __slots__ = [ # comment0 # comment1 # comment2 "dx", "cx", "bx", "ax" # comment3 @@ -139,7 +139,7 @@ class SlotUser: 'distance': 'measured in kilometers'} class Klass5: - __match_args__ = ( + __slots__ = ( "look", ( "a_veeeeeeeeeeeeeeeeeeery_long_parenthesized_item" @@ -194,14 +194,14 @@ class BezierBuilder4: class Klass6: __slots__ = () - __match_args__ = [] + __slots__ = [] __slots__ = ("single_item",) - __match_args__ = ( + __slots__ = ( "single_item_multiline", ) __slots__ = {"single_item",} __slots__ = {"single_item_no_trailing_comma": "docs for that"} - __match_args__ = [ + __slots__ = [ "single_item_multiline_no_trailing_comma" ] __slots__ = ("not_a_tuple_just_a_string") @@ -218,11 +218,11 @@ class Klass6: __slots__ = ("b", "a", "e", "d") __slots__ = ["b", "a", "e", "d"] -__match_args__ = ["foo", "bar", "antipasti"] +__slots__ = ["foo", "bar", "antipasti"] class Klass6: __slots__ = (9, 8, 7) - __match_args__ = ( # This is just an empty tuple, + __slots__ = ( # This is just an empty tuple, # but, # it's very well ) # documented @@ -245,10 +245,10 @@ class Klass6: __slots__ = [ () ] - __match_args__ = ( + __slots__ = ( () ) - __match_args__ = ( + __slots__ = ( [] ) __slots__ = ( @@ -257,12 +257,9 @@ class Klass6: __slots__ = ( [], ) - __match_args__ = ( + __slots__ = ( "foo", [], "bar" ) - __match_args__ = [ + __slots__ = [ "foo", (), "bar" ] - - __match_args__ = {"a", "set", "for", "__match_args__", "is invalid"} - __match_args__ = {"this": "is", "also": "invalid"} diff --git a/crates/ruff_linter/src/rules/ruff/rules/sequence_sorting.rs b/crates/ruff_linter/src/rules/ruff/rules/sequence_sorting.rs index e27cca3562ed8..da082966d1d21 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/sequence_sorting.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/sequence_sorting.rs @@ -2,7 +2,7 @@ /// /// Examples where these are useful: /// - Sorting `__all__` in the global scope, -/// - Sorting `__slots__` or `__match_args__` in a class scope +/// - Sorting `__slots__` in a class scope use std::borrow::Cow; use std::cmp::Ordering; diff --git a/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_slots.rs b/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_slots.rs index bac21f6befe88..83248ac9dc0f3 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_slots.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_slots.rs @@ -1,5 +1,4 @@ use std::borrow::Cow; -use std::fmt::Display; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -17,14 +16,12 @@ use crate::rules::ruff::rules::sequence_sorting::{ use itertools::izip; /// ## What it does -/// Checks for `__slots__` and `__match_args__` -/// definitions that are not ordered according to a +/// Checks for `__slots__` definitions that are not ordered according to a /// [natural sort](https://en.wikipedia.org/wiki/Natural_sort_order). /// /// ## Why is this bad? -/// Consistency is good. Use a common convention for -/// these special variables to make your code more -/// readable and idiomatic. +/// Consistency is good. Use a common convention for this special variable +/// to make your code more readable and idiomatic. /// /// ## Example /// ```python @@ -40,7 +37,6 @@ use itertools::izip; #[violation] pub struct UnsortedDunderSlots { class_name: String, - class_variable: SpecialClassDunder, } impl Violation for UnsortedDunderSlots { @@ -48,43 +44,18 @@ impl Violation for UnsortedDunderSlots { #[derive_message_formats] fn message(&self) -> String { - let UnsortedDunderSlots { - class_name, - class_variable, - } = self; - format!("`{class_name}.{class_variable}` is not sorted") + format!("`{}.__slots__` is not sorted", self.class_name) } fn fix_title(&self) -> Option { - let UnsortedDunderSlots { - class_name, - class_variable, - } = self; Some(format!( - "Apply a natural sort to `{class_name}.{class_variable}`" + "Apply a natural sort to `{}.__slots__`", + self.class_name )) } } -/// Enumeration of the two special class dunders -/// that we're interested in for this rule: `__match_args__` and `__slots__` -#[derive(Debug, Eq, PartialEq, Copy, Clone)] -enum SpecialClassDunder { - Slots, - MatchArgs, -} - -impl Display for SpecialClassDunder { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let string = match self { - Self::MatchArgs => "__match_args__", - Self::Slots => "__slots__", - }; - write!(f, "{string}") - } -} - -/// Sort a `__slots__`/`__match_args__` definition +/// Sort a `__slots__` definition /// represented by a `StmtAssign` AST node. /// For example: `__slots__ = ["b", "c", "a"]`. pub(crate) fn sort_dunder_slots_assign( @@ -96,7 +67,7 @@ pub(crate) fn sort_dunder_slots_assign( } } -/// Sort a `__slots__`/`__match_args__` definition +/// Sort a `__slots__` definition /// represented by a `StmtAnnAssign` AST node. /// For example: `__slots__: list[str] = ["b", "c", "a"]`. pub(crate) fn sort_dunder_slots_ann_assign(checker: &mut Checker, node: &ast::StmtAnnAssign) { @@ -107,8 +78,7 @@ pub(crate) fn sort_dunder_slots_ann_assign(checker: &mut Checker, node: &ast::St const SORTING_STYLE: SortingStyle = SortingStyle::Natural; -/// Sort a tuple, list, dict or set that defines `__slots__` -/// or `__match_args__` in a class scope. +/// Sort a tuple, list, dict or set that defines `__slots__` in a class scope. /// /// This routine checks whether the display is sorted, and emits a /// violation if it is not sorted. If the tuple/list/set was not sorted, @@ -118,13 +88,11 @@ fn sort_dunder_slots(checker: &mut Checker, target: &ast::Expr, node: &ast::Expr return; }; - let dunder_kind = match id.as_str() { - "__slots__" => SpecialClassDunder::Slots, - "__match_args__" => SpecialClassDunder::MatchArgs, - _ => return, - }; + if id != "__slots__" { + return; + } - // We're only interested in `__slots__`/`__match_args__` in the class scope + // We're only interested in `__slots__` in the class scope let ScopeKind::Class(ast::StmtClassDef { name: class_name, .. }) = checker.semantic().current_scope().kind @@ -132,7 +100,7 @@ fn sort_dunder_slots(checker: &mut Checker, target: &ast::Expr, node: &ast::Expr return; }; - let Some(display) = StringLiteralDisplay::new(node, dunder_kind) else { + let Some(display) = StringLiteralDisplay::new(node) else { return; }; @@ -144,7 +112,6 @@ fn sort_dunder_slots(checker: &mut Checker, target: &ast::Expr, node: &ast::Expr let mut diagnostic = Diagnostic::new( UnsortedDunderSlots { class_name: class_name.to_string(), - class_variable: dunder_kind, }, display.range, ); @@ -179,9 +146,9 @@ impl Ranged for StringLiteralDisplay<'_> { } impl<'a> StringLiteralDisplay<'a> { - fn new(node: &'a ast::Expr, dunder_kind: SpecialClassDunder) -> Option { - let result = match (dunder_kind, node) { - (_, ast::Expr::List(ast::ExprList { elts, range, .. })) => { + fn new(node: &'a ast::Expr) -> Option { + let result = match node { + ast::Expr::List(ast::ExprList { elts, range, .. }) => { let display_kind = DisplayKind::Sequence(SequenceKind::List); Self { elts: Cow::Borrowed(elts), @@ -189,7 +156,7 @@ impl<'a> StringLiteralDisplay<'a> { display_kind, } } - (_, ast::Expr::Tuple(tuple_node @ ast::ExprTuple { elts, range, .. })) => { + ast::Expr::Tuple(tuple_node @ ast::ExprTuple { elts, range, .. }) => { let display_kind = DisplayKind::Sequence(SequenceKind::Tuple(tuple_node)); Self { elts: Cow::Borrowed(elts), @@ -197,7 +164,7 @@ impl<'a> StringLiteralDisplay<'a> { display_kind, } } - (SpecialClassDunder::Slots, ast::Expr::Set(ast::ExprSet { elts, range })) => { + ast::Expr::Set(ast::ExprSet { elts, range }) => { let display_kind = DisplayKind::Sequence(SequenceKind::Set); Self { elts: Cow::Borrowed(elts), @@ -205,21 +172,16 @@ impl<'a> StringLiteralDisplay<'a> { display_kind, } } - ( - SpecialClassDunder::Slots, - ast::Expr::Dict(ast::ExprDict { - keys, - values, - range, - }), - ) => { + ast::Expr::Dict(ast::ExprDict { + keys, + values, + range, + }) => { let mut narrowed_keys = Vec::with_capacity(values.len()); for key in keys { if let Some(key) = key { // This is somewhat unfortunate, - // *but* only `__slots__` can be a dict out of - // `__all__`, `__slots__` and `__match_args__`, - // and even for `__slots__`, using a dict is very rare + // *but* using a dict for __slots__ is very rare narrowed_keys.push(key.to_owned()); } else { return None; diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap index 484358cb1904a..3b903394795af 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF023_RUF023.py.snap @@ -6,7 +6,7 @@ RUF023.py:6:17: RUF023 [*] `Klass.__slots__` is not sorted 5 | class Klass: 6 | __slots__ = ["d", "c", "b", "a"] # a comment that is untouched | ^^^^^^^^^^^^^^^^^^^^ RUF023 -7 | __match_args__ = ("d", "c", "b", "a") +7 | __slots__ = ("d", "c", "b", "a") | = help: Apply a natural sort to `Klass.__slots__` @@ -16,27 +16,27 @@ RUF023.py:6:17: RUF023 [*] `Klass.__slots__` is not sorted 5 5 | class Klass: 6 |- __slots__ = ["d", "c", "b", "a"] # a comment that is untouched 6 |+ __slots__ = ["a", "b", "c", "d"] # a comment that is untouched -7 7 | __match_args__ = ("d", "c", "b", "a") +7 7 | __slots__ = ("d", "c", "b", "a") 8 8 | 9 9 | # Quoting style is retained, -RUF023.py:7:22: RUF023 [*] `Klass.__match_args__` is not sorted +RUF023.py:7:17: RUF023 [*] `Klass.__slots__` is not sorted | 5 | class Klass: 6 | __slots__ = ["d", "c", "b", "a"] # a comment that is untouched -7 | __match_args__ = ("d", "c", "b", "a") - | ^^^^^^^^^^^^^^^^^^^^ RUF023 +7 | __slots__ = ("d", "c", "b", "a") + | ^^^^^^^^^^^^^^^^^^^^ RUF023 8 | 9 | # Quoting style is retained, | - = help: Apply a natural sort to `Klass.__match_args__` + = help: Apply a natural sort to `Klass.__slots__` ℹ Safe fix 4 4 | 5 5 | class Klass: 6 6 | __slots__ = ["d", "c", "b", "a"] # a comment that is untouched -7 |- __match_args__ = ("d", "c", "b", "a") - 7 |+ __match_args__ = ("a", "b", "c", "d") +7 |- __slots__ = ("d", "c", "b", "a") + 7 |+ __slots__ = ("a", "b", "c", "d") 8 8 | 9 9 | # Quoting style is retained, 10 10 | # but unnecessary parens are not @@ -60,25 +60,25 @@ RUF023.py:11:22: RUF023 [*] `Klass.__slots__` is not sorted 11 |+ __slots__: set = {'a', 'b', "c"} 12 12 | # Trailing commas are also not retained for single-line definitions 13 13 | # (but they are in multiline definitions) -14 14 | __match_args__: tuple = ("b", "c", "a",) +14 14 | __slots__: tuple = ("b", "c", "a",) -RUF023.py:14:29: RUF023 [*] `Klass.__match_args__` is not sorted +RUF023.py:14:24: RUF023 [*] `Klass.__slots__` is not sorted | 12 | # Trailing commas are also not retained for single-line definitions 13 | # (but they are in multiline definitions) -14 | __match_args__: tuple = ("b", "c", "a",) - | ^^^^^^^^^^^^^^^^ RUF023 +14 | __slots__: tuple = ("b", "c", "a",) + | ^^^^^^^^^^^^^^^^ RUF023 15 | 16 | class Klass2: | - = help: Apply a natural sort to `Klass.__match_args__` + = help: Apply a natural sort to `Klass.__slots__` ℹ Safe fix 11 11 | __slots__: set = {'b', "c", ((('a')))} 12 12 | # Trailing commas are also not retained for single-line definitions 13 13 | # (but they are in multiline definitions) -14 |- __match_args__: tuple = ("b", "c", "a",) - 14 |+ __match_args__: tuple = ("a", "b", "c") +14 |- __slots__: tuple = ("b", "c", "a",) + 14 |+ __slots__: tuple = ("a", "b", "c") 15 15 | 16 16 | class Klass2: 17 17 | if bool(): @@ -111,7 +111,7 @@ RUF023.py:20:21: RUF023 [*] `Klass2.__slots__` is not sorted 20 | __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) | ^^^^^^^^^^^^^^^^^^^^^^ RUF023 21 | -22 | __match_args__: list[str] = ["the", "three", "little", "pigs"] +22 | __slots__: list[str] = ["the", "three", "little", "pigs"] | = help: Apply a natural sort to `Klass2.__slots__` @@ -122,33 +122,33 @@ RUF023.py:20:21: RUF023 [*] `Klass2.__slots__` is not sorted 20 |- __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) 20 |+ __slots__ = "foo1", "foo2", "foo3" # NB: an implicit tuple (without parens) 21 21 | -22 22 | __match_args__: list[str] = ["the", "three", "little", "pigs"] +22 22 | __slots__: list[str] = ["the", "three", "little", "pigs"] 23 23 | __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") -RUF023.py:22:33: RUF023 [*] `Klass2.__match_args__` is not sorted +RUF023.py:22:28: RUF023 [*] `Klass2.__slots__` is not sorted | 20 | __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) 21 | -22 | __match_args__: list[str] = ["the", "three", "little", "pigs"] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 +22 | __slots__: list[str] = ["the", "three", "little", "pigs"] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 23 | __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") 24 | # we use natural sort, | - = help: Apply a natural sort to `Klass2.__match_args__` + = help: Apply a natural sort to `Klass2.__slots__` ℹ Safe fix 19 19 | else: 20 20 | __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) 21 21 | -22 |- __match_args__: list[str] = ["the", "three", "little", "pigs"] - 22 |+ __match_args__: list[str] = ["little", "pigs", "the", "three"] +22 |- __slots__: list[str] = ["the", "three", "little", "pigs"] + 22 |+ __slots__: list[str] = ["little", "pigs", "the", "three"] 23 23 | __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") 24 24 | # we use natural sort, 25 25 | # not alphabetical sort or "isort-style" sort RUF023.py:23:17: RUF023 [*] `Klass2.__slots__` is not sorted | -22 | __match_args__: list[str] = ["the", "three", "little", "pigs"] +22 | __slots__: list[str] = ["the", "three", "little", "pigs"] 23 | __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF023 24 | # we use natural sort, @@ -159,7 +159,7 @@ RUF023.py:23:17: RUF023 [*] `Klass2.__slots__` is not sorted ℹ Safe fix 20 20 | __slots__ = "foo3", "foo2", "foo1" # NB: an implicit tuple (without parens) 21 21 | -22 22 | __match_args__: list[str] = ["the", "three", "little", "pigs"] +22 22 | __slots__: list[str] = ["the", "three", "little", "pigs"] 23 |- __slots__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple") 23 |+ __slots__ = "an_unparenthesized_tuple", "in", "parenthesized_item" 24 24 | # we use natural sort, @@ -199,7 +199,7 @@ RUF023.py:33:17: RUF023 [*] `Klass3.__slots__` is not sorted 38 | | "a0" 39 | | ) | |_____^ RUF023 -40 | __match_args__ = [ +40 | __slots__ = [ 41 | "d", | = help: Apply a natural sort to `Klass3.__slots__` @@ -218,15 +218,15 @@ RUF023.py:33:17: RUF023 [*] `Klass3.__slots__` is not sorted 38 |- "a0" 38 |+ "d0" 39 39 | ) -40 40 | __match_args__ = [ +40 40 | __slots__ = [ 41 41 | "d", -RUF023.py:40:22: RUF023 [*] `Klass3.__match_args__` is not sorted +RUF023.py:40:17: RUF023 [*] `Klass3.__slots__` is not sorted | 38 | "a0" 39 | ) -40 | __match_args__ = [ - | ______________________^ +40 | __slots__ = [ + | _________________^ 41 | | "d", 42 | | "c", # a comment regarding 'c' 43 | | "b", @@ -237,12 +237,12 @@ RUF023.py:40:22: RUF023 [*] `Klass3.__match_args__` is not sorted 47 | 48 | ################################## | - = help: Apply a natural sort to `Klass3.__match_args__` + = help: Apply a natural sort to `Klass3.__slots__` ℹ Safe fix 38 38 | "a0" 39 39 | ) -40 40 | __match_args__ = [ +40 40 | __slots__ = [ 41 |- "d", 41 |+ # a comment regarding 'a': 42 |+ "a", @@ -296,12 +296,12 @@ RUF023.py:54:17: RUF023 [*] `Klass4.__slots__` is not sorted 61 64 | ) # comment6 62 65 | # comment7 -RUF023.py:64:22: RUF023 [*] `Klass4.__match_args__` is not sorted +RUF023.py:64:17: RUF023 [*] `Klass4.__slots__` is not sorted | 62 | # comment7 63 | -64 | __match_args__ = [ # comment0 - | ______________________^ +64 | __slots__ = [ # comment0 + | _________________^ 65 | | # comment1 66 | | # comment2 67 | | "dx", "cx", "bx", "ax" # comment3 @@ -313,12 +313,12 @@ RUF023.py:64:22: RUF023 [*] `Klass4.__match_args__` is not sorted 72 | 73 | # from cpython/Lib/pathlib/__init__.py | - = help: Apply a natural sort to `Klass4.__match_args__` + = help: Apply a natural sort to `Klass4.__slots__` ℹ Safe fix 62 62 | # comment7 63 63 | -64 64 | __match_args__ = [ # comment0 +64 64 | __slots__ = [ # comment0 65 |+ "ax", 66 |+ "bx", 67 |+ "cx", @@ -499,11 +499,11 @@ RUF023.py:138:17: RUF023 `SlotUser.__slots__` is not sorted | = help: Apply a natural sort to `SlotUser.__slots__` -RUF023.py:142:22: RUF023 `Klass5.__match_args__` is not sorted +RUF023.py:142:17: RUF023 `Klass5.__slots__` is not sorted | 141 | class Klass5: -142 | __match_args__ = ( - | ______________________^ +142 | __slots__ = ( + | _________________^ 143 | | "look", 144 | | ( 145 | | "a_veeeeeeeeeeeeeeeeeeery_long_parenthesized_item" @@ -513,7 +513,7 @@ RUF023.py:142:22: RUF023 `Klass5.__match_args__` is not sorted 148 | __slots__ = ( 149 | "b", | - = help: Apply a natural sort to `Klass5.__match_args__` + = help: Apply a natural sort to `Klass5.__slots__` RUF023.py:148:17: RUF023 `Klass5.__slots__` is not sorted |