From e415ad399c8aa08ce9515811d6a74a9a1b242893 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 5 Aug 2024 17:42:34 +0100 Subject: [PATCH 1/2] [`ruff`] Mark `RUF023` fix as unsafe if `__slots__` is not a set and the binding is used elsewhere --- .../resources/test/fixtures/ruff/RUF023.py | 8 + .../src/checkers/ast/analyze/bindings.rs | 8 +- .../src/checkers/ast/analyze/statement.rs | 6 - .../src/rules/ruff/rules/sort_dunder_slots.rs | 154 ++++++++++-------- ..._rules__ruff__tests__RUF023_RUF023.py.snap | 26 ++- 5 files changed, 119 insertions(+), 83 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py index c77446056c69f..d9de20a0b4525 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF023.py @@ -192,6 +192,14 @@ class BezierBuilder4: "baz", "bingo" } + +class VeryDRY: + # This should get flagged, *but* the fix is unsafe, + # since the `__slots__` binding is used by the `__match_args__` definition + __slots__ = ("foo", "bar") + __match_args__ = __slots__ + + ################################### # These should all not get flagged: ################################### diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 0fbc85f5552fa..6400f5a52081f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -3,7 +3,7 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint}; +use crate::rules::{flake8_import_conventions, flake8_pyi, pyflakes, pylint, ruff}; /// Run lint rules over the [`Binding`]s. pub(crate) fn bindings(checker: &mut Checker) { @@ -13,6 +13,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::NonAsciiName, Rule::UnaliasedCollectionsAbcSetImport, Rule::UnconventionalImportAlias, + Rule::UnsortedDunderSlots, Rule::UnusedVariable, ]) { return; @@ -71,5 +72,10 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::UnsortedDunderSlots) { + if let Some(diagnostic) = ruff::rules::sort_dunder_slots(checker, binding) { + checker.diagnostics.push(diagnostic); + } + } } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 69dff843c6512..385eb53bd4661 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1591,9 +1591,6 @@ 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, @@ -1676,9 +1673,6 @@ 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) { 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 dc25033087631..1e6da67a8b0f5 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,9 +1,9 @@ use std::borrow::Cow; -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; -use ruff_python_semantic::ScopeKind; +use ruff_python_semantic::Binding; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -36,7 +36,7 @@ use itertools::izip; /// ``` #[violation] pub struct UnsortedDunderSlots { - class_name: String, + class_name: ast::name::Name, } impl Violation for UnsortedDunderSlots { @@ -55,27 +55,6 @@ impl Violation for UnsortedDunderSlots { } } -/// Sort a `__slots__` definition -/// represented by a `StmtAssign` AST node. -/// For example: `__slots__ = ["b", "c", "a"]`. -pub(crate) fn sort_dunder_slots_assign( - checker: &mut Checker, - ast::StmtAssign { value, targets, .. }: &ast::StmtAssign, -) { - if let [expr] = targets.as_slice() { - sort_dunder_slots(checker, expr, value); - } -} - -/// 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) { - if let Some(value) = &node.value { - sort_dunder_slots(checker, &node.target, value); - } -} - const SORTING_STYLE: SortingStyle = SortingStyle::Natural; /// Sort a tuple, list, dict or set that defines `__slots__` in a class scope. @@ -83,46 +62,59 @@ const SORTING_STYLE: SortingStyle = SortingStyle::Natural; /// 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, /// it attempts to set a `Fix` on the violation. -fn sort_dunder_slots(checker: &mut Checker, target: &ast::Expr, node: &ast::Expr) { - let ast::Expr::Name(ast::ExprName { id, .. }) = target else { - return; +pub(crate) fn sort_dunder_slots(checker: &Checker, binding: &Binding) -> Option { + let semantic = checker.semantic(); + + let (target, value) = match binding.statement(semantic)? { + ast::Stmt::Assign(ast::StmtAssign { targets, value, .. }) => match targets.as_slice() { + [target] => (target, &**value), + _ => return None, + }, + ast::Stmt::AnnAssign(ast::StmtAnnAssign { target, value, .. }) => { + (&**target, value.as_deref()?) + } + _ => return None, }; + let ast::ExprName { id, .. } = target.as_name_expr()?; + if id != "__slots__" { - return; + return None; } // 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 enclosing_class = semantic.scopes[binding.scope].kind.as_class()?; - let Some(display) = StringLiteralDisplay::new(node) else { - return; - }; + // and it has to be an assignment to a "display literal" (a literal dict/set/tuple/list) + let display = StringLiteralDisplay::new(value)?; let sort_classification = SortClassification::of_elements(&display.elts, SORTING_STYLE); if sort_classification.is_not_a_list_of_string_literals() || sort_classification.is_sorted() { - return; + return None; } let mut diagnostic = Diagnostic::new( UnsortedDunderSlots { - class_name: class_name.to_string(), + class_name: enclosing_class.name.id.clone(), }, display.range, ); if let SortClassification::UnsortedAndMaybeFixable { items } = sort_classification { - if let Some(fix) = display.generate_fix(&items, checker) { - diagnostic.set_fix(fix); + if let Some(sorted_source_code) = display.generate_sorted_source_code(&items, checker) { + let edit = Edit::range_replacement(sorted_source_code, display.range()); + + let applicability = if display.kind.is_set_literal() || !binding.is_used() { + Applicability::Safe + } else { + Applicability::Unsafe + }; + + diagnostic.set_fix(Fix::applicable_edit(edit, applicability)); } } - checker.diagnostics.push(diagnostic); + Some(diagnostic) } /// Struct representing a [display](https://docs.python.org/3/reference/expressions.html#displays-for-lists-sets-and-dictionaries) @@ -136,7 +128,7 @@ struct StringLiteralDisplay<'a> { /// The source-code range of the display as a whole range: TextRange, /// What kind of a display is it? A dict, set, list or tuple? - display_kind: DisplayKind<'a>, + kind: DisplayKind<'a>, } impl Ranged for StringLiteralDisplay<'_> { @@ -149,29 +141,29 @@ impl<'a> StringLiteralDisplay<'a> { 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); + let kind = DisplayKind::Sequence(SequenceKind::List); Self { elts: Cow::Borrowed(elts), range: *range, - display_kind, + kind, } } ast::Expr::Tuple(tuple_node @ ast::ExprTuple { elts, range, .. }) => { - let display_kind = DisplayKind::Sequence(SequenceKind::Tuple { + let kind = DisplayKind::Sequence(SequenceKind::Tuple { parenthesized: tuple_node.parenthesized, }); Self { elts: Cow::Borrowed(elts), range: *range, - display_kind, + kind, } } ast::Expr::Set(ast::ExprSet { elts, range }) => { - let display_kind = DisplayKind::Sequence(SequenceKind::Set); + let kind = DisplayKind::Sequence(SequenceKind::Set); Self { elts: Cow::Borrowed(elts), range: *range, - display_kind, + kind, } } ast::Expr::Dict(dict @ ast::ExprDict { items, range }) => { @@ -193,7 +185,7 @@ impl<'a> StringLiteralDisplay<'a> { Self { elts: Cow::Owned(narrowed_keys), range: *range, - display_kind: DisplayKind::Dict { items }, + kind: DisplayKind::Dict { items }, } } _ => return None, @@ -201,11 +193,17 @@ impl<'a> StringLiteralDisplay<'a> { Some(result) } - fn generate_fix(&self, elements: &[&str], checker: &Checker) -> Option { + fn generate_sorted_source_code(&self, elements: &[&str], checker: &Checker) -> Option { let locator = checker.locator(); - let is_multiline = locator.contains_line_break(self.range()); - let sorted_source_code = match (&self.display_kind, is_multiline) { - (DisplayKind::Sequence(sequence_kind), true) => { + + let multiline_classification = if locator.contains_line_break(self.range()) { + MultilineClassification::Multiline + } else { + MultilineClassification::SingleLine + }; + + match (&self.kind, multiline_classification) { + (DisplayKind::Sequence(sequence_kind), MultilineClassification::Multiline) => { let analyzed_sequence = MultilineStringSequenceValue::from_source_range( self.range(), *sequence_kind, @@ -214,37 +212,51 @@ impl<'a> StringLiteralDisplay<'a> { elements, )?; assert_eq!(analyzed_sequence.len(), self.elts.len()); - analyzed_sequence.into_sorted_source_code(SORTING_STYLE, locator, checker.stylist()) + Some(analyzed_sequence.into_sorted_source_code( + SORTING_STYLE, + locator, + checker.stylist(), + )) } // Sorting multiline dicts is unsupported - (DisplayKind::Dict { .. }, true) => return None, - (DisplayKind::Sequence(sequence_kind), false) => sort_single_line_elements_sequence( - *sequence_kind, - &self.elts, - elements, - locator, - SORTING_STYLE, - ), - (DisplayKind::Dict { items }, false) => { - sort_single_line_elements_dict(&self.elts, elements, items, locator) + (DisplayKind::Dict { .. }, MultilineClassification::Multiline) => None, + (DisplayKind::Sequence(sequence_kind), MultilineClassification::SingleLine) => { + Some(sort_single_line_elements_sequence( + *sequence_kind, + &self.elts, + elements, + locator, + SORTING_STYLE, + )) } - }; - Some(Fix::safe_edit(Edit::range_replacement( - sorted_source_code, - self.range, - ))) + (DisplayKind::Dict { items }, MultilineClassification::SingleLine) => Some( + sort_single_line_elements_dict(&self.elts, elements, items, locator), + ), + } } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum MultilineClassification { + SingleLine, + Multiline, +} + /// An enumeration of the various kinds of /// [display literals](https://docs.python.org/3/reference/expressions.html#displays-for-lists-sets-and-dictionaries) /// Python provides for builtin containers. -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] enum DisplayKind<'a> { Sequence(SequenceKind), Dict { items: &'a [ast::DictItem] }, } +impl<'a> DisplayKind<'a> { + const fn is_set_literal(self) -> bool { + matches!(self, Self::Sequence(SequenceKind::Set)) + } +} + /// A newtype that zips together three iterables: /// /// 1. The string values of a dict literal's keys; 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 2ec896e3081c3..7365bb1259708 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 @@ -672,8 +672,6 @@ RUF023.py:191:17: RUF023 [*] `BezierBuilder4.__slots__` is not sorted 192 | | "baz", "bingo" 193 | | } | |__________________^ RUF023 -194 | -195 | ################################### | = help: Apply a natural sort to `BezierBuilder4.__slots__` @@ -691,7 +689,25 @@ RUF023.py:191:17: RUF023 [*] `BezierBuilder4.__slots__` is not sorted 195 |+ "foo" 196 |+ } 194 197 | -195 198 | ################################### -196 199 | # These should all not get flagged: - +195 198 | +196 199 | class VeryDRY: +RUF023.py:199:17: RUF023 [*] `VeryDRY.__slots__` is not sorted + | +197 | # This should get flagged, *but* the fix is unsafe, +198 | # since the `__slots__` binding is used by the `__match_args__` definition +199 | __slots__ = ("foo", "bar") + | ^^^^^^^^^^^^^^ RUF023 +200 | __match_args__ = __slots__ + | + = help: Apply a natural sort to `VeryDRY.__slots__` + +ℹ Unsafe fix +196 196 | class VeryDRY: +197 197 | # This should get flagged, *but* the fix is unsafe, +198 198 | # since the `__slots__` binding is used by the `__match_args__` definition +199 |- __slots__ = ("foo", "bar") + 199 |+ __slots__ = ("bar", "foo") +200 200 | __match_args__ = __slots__ +201 201 | +202 202 | From 77bf58087c522575ba6fd345e3f0d0b2726645c4 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 5 Aug 2024 22:02:56 +0100 Subject: [PATCH 2/2] Add more docs on fix safety --- .../src/rules/ruff/rules/sort_dunder_all.rs | 9 +++++++-- .../src/rules/ruff/rules/sort_dunder_slots.rs | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_all.rs b/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_all.rs index 2e52956ed45c8..6b0ebbaaf4fc5 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_all.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/sort_dunder_all.rs @@ -54,8 +54,13 @@ use crate::rules::ruff::rules::sequence_sorting::{ /// /// ## Fix safety /// This rule's fix is marked as always being safe, in that -/// it should never alter the semantics of any Python code. -/// However, note that for multiline `__all__` definitions +/// it should very rarely alter the semantics of any Python code. +/// However, note that (although it's rare) the value of `__all__` +/// could be read by code elsewhere that depends on the exact +/// iteration order of the items in `__all__`, in which case this +/// rule's fix could theoretically cause breakage. +/// +/// Note also that for multiline `__all__` definitions /// that include comments on their own line, it can be hard /// to tell where the comments should be moved to when sorting /// the contents of `__all__`. While this rule's fix will 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 1e6da67a8b0f5..dbc40493b8520 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 @@ -34,6 +34,26 @@ use itertools::izip; /// class Dog: /// __slots__ = "breed", "name" /// ``` +/// +/// ## Fix safety +/// This rule's fix is marked as unsafe whenever Ruff can detect that code +/// elsewhere in the same file reads the `__slots__` variable in some way. +/// This is because the order of the items in `__slots__` may have semantic +/// significance if the `__slots__` of a class is being iterated over, or +/// being assigned to another value. +/// +/// In the vast majority of other cases, this rule's fix is unlikely to +/// cause breakage; as such, Ruff will otherwise mark this rule's fix as +/// safe. However, note that (although it's rare) the value of `__slots__` +/// could still be read by code outside of the module in which the +/// `__slots__` definition occurs, in which case this rule's fix could +/// theoretically cause breakage. +/// +/// Additionally, note that for multiline `__slots__` definitions that +/// include comments on their own line, it can be hard to tell where the +/// comments should be moved to when sorting the contents of `__slots__`. +/// While this rule's fix will never delete a comment, it might *sometimes* +/// move a comment to an unexpected location. #[violation] pub struct UnsortedDunderSlots { class_name: ast::name::Name,