Skip to content

Commit

Permalink
RUF023: Don't sort __match_args__, only __slots__ (#9724)
Browse files Browse the repository at this point in the history
Fixes #9723. I'm pretty embarrassed I forgot that order was important
here :(
  • Loading branch information
AlexWaygood authored Jan 30, 2024
1 parent 541aef4 commit 6bb1264
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 123 deletions.
33 changes: 15 additions & 18 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,22 @@

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():
__slots__ = {"x": "docs for x", "m": "docs for m", "a": "docs for a"}
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
Expand All @@ -37,7 +37,7 @@ class Klass3:
# a comment regarding 'a0':
"a0"
)
__match_args__ = [
__slots__ = [
"d",
"c", # a comment regarding 'c'
"b",
Expand All @@ -61,7 +61,7 @@ class Klass4:
) # comment6
# comment7

__match_args__ = [ # comment0
__slots__ = [ # comment0
# comment1
# comment2
"dx", "cx", "bx", "ax" # comment3
Expand Down Expand Up @@ -139,7 +139,7 @@ class SlotUser:
'distance': 'measured in kilometers'}

class Klass5:
__match_args__ = (
__slots__ = (
"look",
(
"a_veeeeeeeeeeeeeeeeeeery_long_parenthesized_item"
Expand Down Expand Up @@ -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")
Expand All @@ -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
Expand All @@ -245,10 +245,10 @@ class Klass6:
__slots__ = [
()
]
__match_args__ = (
__slots__ = (
()
)
__match_args__ = (
__slots__ = (
[]
)
__slots__ = (
Expand All @@ -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"}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
88 changes: 25 additions & 63 deletions crates/ruff_linter/src/rules/ruff/rules/sort_dunder_slots.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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
Expand All @@ -40,51 +37,25 @@ use itertools::izip;
#[violation]
pub struct UnsortedDunderSlots {
class_name: String,
class_variable: SpecialClassDunder,
}

impl Violation for UnsortedDunderSlots {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[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<String> {
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(
Expand All @@ -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) {
Expand All @@ -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,
Expand All @@ -118,21 +88,19 @@ 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
else {
return;
};

let Some(display) = StringLiteralDisplay::new(node, dunder_kind) else {
let Some(display) = StringLiteralDisplay::new(node) else {
return;
};

Expand All @@ -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,
);
Expand Down Expand Up @@ -179,47 +146,42 @@ impl Ranged for StringLiteralDisplay<'_> {
}

impl<'a> StringLiteralDisplay<'a> {
fn new(node: &'a ast::Expr, dunder_kind: SpecialClassDunder) -> Option<Self> {
let result = match (dunder_kind, node) {
(_, ast::Expr::List(ast::ExprList { elts, range, .. })) => {
fn new(node: &'a ast::Expr) -> Option<Self> {
let result = match node {
ast::Expr::List(ast::ExprList { elts, range, .. }) => {
let display_kind = DisplayKind::Sequence(SequenceKind::List);
Self {
elts: Cow::Borrowed(elts),
range: *range,
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),
range: *range,
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),
range: *range,
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;
Expand Down
Loading

0 comments on commit 6bb1264

Please sign in to comment.