Skip to content

Commit

Permalink
Add a rule/autofix to sort __slots__ and __match_args__ (#9564)
Browse files Browse the repository at this point in the history
## Summary

This PR introduces a new rule to sort `__slots__` and `__match_args__`
according to a [natural sort](https://en.wikipedia.org/wiki/Natural_sort_order), as was
requested in #1198 (comment).

The implementation here generalises some of the machinery introduced in
3aae16f
so that different kinds of sorts can be applied to lists of string
literals. (We use an "isort-style" sort for `__all__`, but that isn't
really appropriate for `__slots__` and `__match_args__`, where nearly
all items will be snake_case.) Several sections of code have been moved
from `sort_dunder_all.rs` to a new module, `sorting_helpers.rs`, which
`sort_dunder_all.rs` and `sort_dunder_slots.rs` both make use of.

`__match_args__` is very similar to `__all__`, in that it can only be a
tuple or a list. `__slots__` differs from the other two, however, in
that it can be any iterable of strings. If slots is a dictionary, the
values are used by the builtin `help()` function as per-attribute
docstrings that show up in the output of `help()`. (There's no
particular use-case for making `__slots__` a set, but it's perfectly
legal at runtime, so there's no reason for us not to handle it in this
rule.)

Note that we don't do an autofix for multiline `__slots__` if `__slots__` is a dictionary: that's out of scope. Everything else, we can nearly always fix, however.

## Test Plan

`cargo test` / `cargo insta review`.

I also ran this rule on CPython, and the diff looked pretty good

---

Co-authored-by: Micha Reiser <micha@reiser.io>
  • Loading branch information
AlexWaygood and MichaReiser committed Jan 22, 2024
1 parent a3d667e commit f5061db
Show file tree
Hide file tree
Showing 11 changed files with 2,093 additions and 813 deletions.
2 changes: 2 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF022.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,5 @@ class IntroducesNonModuleScope:
__all__ = [
"foo", (), "bar"
]

__all__ = "foo", "an" "implicitly_concatenated_second_item", not_a_string_literal
234 changes: 234 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
#########################
# Single-line definitions
#########################

class Klass:
__slots__ = ["d", "c", "b", "a"] # a comment that is untouched
__match_args__ = ("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",)

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__ = ("parenthesized_item"), "in", ("an_unparenthesized_tuple")
# we use natural sort,
# not alphabetical sort or "isort-style" sort
__slots__ = {"aadvark237", "aadvark10092", "aadvark174", "aadvark532"}

############################
# Neat multiline definitions
############################

class Klass3:
__slots__ = (
"d0",
"c0", # a comment regarding 'c0'
"b0",
# a comment regarding 'a0':
"a0"
)
__match_args__ = [
"d",
"c", # a comment regarding 'c'
"b",
# a comment regarding 'a':
"a"
]

##########################################
# Messier multiline __all__ definitions...
##########################################

class Klass4:
# comment0
__slots__ = ("d", "a", # comment1
# comment2
"f", "b",
"strangely", # comment3
# comment4
"formatted",
# comment5
) # comment6
# comment7

__match_args__ = [ # comment0
# comment1
# comment2
"dx", "cx", "bx", "ax" # comment3
# comment4
# comment5
# comment6
] # comment7

# from cpython/Lib/pathlib/__init__.py
class PurePath:
__slots__ = (
# The `_raw_paths` slot stores unnormalized string paths. This is set
# in the `__init__()` method.
'_raw_paths',

# The `_drv`, `_root` and `_tail_cached` slots store parsed and
# normalized parts of the path. They are set when any of the `drive`,
# `root` or `_tail` properties are accessed for the first time. The
# three-part division corresponds to the result of
# `os.path.splitroot()`, except that the tail is further split on path
# separators (i.e. it is a list of strings), and that the root and
# tail are normalized.
'_drv', '_root', '_tail_cached',

# The `_str` slot stores the string representation of the path,
# computed from the drive, root and tail when `__str__()` is called
# for the first time. It's used to implement `_str_normcase`
'_str',

# The `_str_normcase_cached` slot stores the string path with
# normalized case. It is set when the `_str_normcase` property is
# accessed for the first time. It's used to implement `__eq__()`
# `__hash__()`, and `_parts_normcase`
'_str_normcase_cached',

# The `_parts_normcase_cached` slot stores the case-normalized
# string path after splitting on path separators. It's set when the
# `_parts_normcase` property is accessed for the first time. It's used
# to implement comparison methods like `__lt__()`.
'_parts_normcase_cached',

# The `_hash` slot stores the hash of the case-normalized string
# path. It's set when `__hash__()` is called for the first time.
'_hash',
)

# From cpython/Lib/pickletools.py
class ArgumentDescriptor(object):
__slots__ = (
# name of descriptor record, also a module global name; a string
'name',

# length of argument, in bytes; an int; UP_TO_NEWLINE and
# TAKEN_FROM_ARGUMENT{1,4,8} are negative values for variable-length
# cases
'n',

# a function taking a file-like object, reading this kind of argument
# from the object at the current position, advancing the current
# position by n bytes, and returning the value of the argument
'reader',

# human-readable docs for this arg descriptor; a string
'doc',
)

####################################
# Should be flagged, but not fixed
####################################

# from cpython/Lib/test/test_inspect.py.
# Multiline dicts are out of scope.
class SlotUser:
__slots__ = {'power': 'measured in kilowatts',
'distance': 'measured in kilometers'}

class Klass5:
__match_args__ = (
"look",
(
"a_veeeeeeeeeeeeeeeeeeery_long_parenthesized_item"
),
)
__slots__ = (
"b",
((
"c"
)),
"a"
)
__slots__ = ("don't" "care" "about", "__slots__" "with", "concatenated" "strings")

###################################
# These should all not get flagged:
###################################

class Klass6:
__slots__ = ()
__match_args__ = []
__slots__ = ("single_item",)
__match_args__ = (
"single_item_multiline",
)
__slots__ = {"single_item",}
__slots__ = {"single_item_no_trailing_comma": "docs for that"}
__match_args__ = [
"single_item_multiline_no_trailing_comma"
]
__slots__ = ("not_a_tuple_just_a_string")
__slots__ = ["a", "b", "c", "d"]
__slots__ += ["e", "f", "g"]
__slots__ = ("a", "b", "c", "d")

if bool():
__slots__ += ("e", "f", "g")
else:
__slots__ += ["alpha", "omega"]

__slots__ = {"not": "sorted", "but": "includes", **a_kwarg_splat}

__slots__ = ("b", "a", "e", "d")
__slots__ = ["b", "a", "e", "d"]
__match_args__ = ["foo", "bar", "antipasti"]

class Klass6:
__slots__ = (9, 8, 7)
__match_args__ = ( # This is just an empty tuple,
# but,
# it's very well
) # documented

# We don't deduplicate elements;
# this just ensures that duplicate elements aren't unnecessarily
# reordered by an autofix:
__slots__ = (
"duplicate_element", # comment1
"duplicate_element", # comment3
"duplicate_element", # comment2
"duplicate_element", # comment0
)

__slots__ = "foo", "an" "implicitly_concatenated_second_item", not_a_string_literal

__slots__ =[
[]
]
__slots__ = [
()
]
__match_args__ = (
()
)
__match_args__ = (
[]
)
__slots__ = (
(),
)
__slots__ = (
[],
)
__match_args__ = (
"foo", [], "bar"
)
__match_args__ = [
"foo", (), "bar"
]

__match_args__ = {"a", "set", "for", "__match_args__", "is invalid"}
__match_args__ = {"this": "is", "also": "invalid"}
6 changes: 6 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.settings.rules.enabled(Rule::UnsortedDunderAll) {
ruff::rules::sort_dunder_all_assign(checker, assign);
}
if checker.enabled(Rule::UnsortedDunderSlots) {
ruff::rules::sort_dunder_slots_assign(checker, assign);
}
if checker.source_type.is_stub() {
if checker.any_enabled(&[
Rule::UnprefixedTypeParam,
Expand Down Expand Up @@ -1523,6 +1526,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.settings.rules.enabled(Rule::UnsortedDunderAll) {
ruff::rules::sort_dunder_all_ann_assign(checker, assign_stmt);
}
if checker.enabled(Rule::UnsortedDunderSlots) {
ruff::rules::sort_dunder_slots_ann_assign(checker, assign_stmt);
}
if checker.source_type.is_stub() {
if let Some(value) = value {
if checker.enabled(Rule::AssignmentDefaultInStub) {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "020") => (RuleGroup::Preview, rules::ruff::rules::NeverUnion),
(Ruff, "021") => (RuleGroup::Preview, rules::ruff::rules::ParenthesizeChainedOperators),
(Ruff, "022") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderAll),
(Ruff, "023") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderSlots),
(Ruff, "024") => (RuleGroup::Preview, rules::ruff::rules::MutableFromkeysValue),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ mod tests {
#[test_case(Rule::NeverUnion, Path::new("RUF020.py"))]
#[test_case(Rule::ParenthesizeChainedOperators, Path::new("RUF021.py"))]
#[test_case(Rule::UnsortedDunderAll, Path::new("RUF022.py"))]
#[test_case(Rule::UnsortedDunderSlots, Path::new("RUF023.py"))]
#[test_case(Rule::MutableFromkeysValue, Path::new("RUF024.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub(crate) use pairwise_over_zipped::*;
pub(crate) use parenthesize_logical_operators::*;
pub(crate) use quadratic_list_summation::*;
pub(crate) use sort_dunder_all::*;
pub(crate) use sort_dunder_slots::*;
pub(crate) use static_key_dict_comprehension::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
Expand All @@ -37,7 +38,9 @@ mod mutable_fromkeys_value;
mod never_union;
mod pairwise_over_zipped;
mod parenthesize_logical_operators;
mod sequence_sorting;
mod sort_dunder_all;
mod sort_dunder_slots;
mod static_key_dict_comprehension;
mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
Expand Down

0 comments on commit f5061db

Please sign in to comment.