Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUF023: Don't sort __match_args__, only __slots__ #9724

Merged
merged 1 commit into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading