Skip to content

[refurb] Mark FURB180 fix unsafe when class has bases #18149

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 3, 2025
+17 −4
Merged

[refurb] Mark FURB180 fix unsafe when class has bases #18149

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b335c8
Mark FURB180 fix unsafe when class has bases
robsdedude May 17, 2025
6e74d75
Merge branch 'main' into fix/13307-unsafe-fix
ntBre May 29, 2025
06e03fd
Minor refactoring: code simplification
robsdedude May 30, 2025
b05972d
Merge branch 'main' into fix/13307-unsafe-fix
robsdedude May 30, 2025
67f2d2b
Formatting
robsdedude May 30, 2025
f8f1aec
cargo clippy --fix
ntBre Jun 3, 2025
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
23 changes: 21 additions & 2 deletions crates/ruff_linter/src/rules/refurb/rules/metaclass_abcmeta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ use crate::{AlwaysFixableViolation, Edit, Fix};
/// pass
/// ```
///
/// ## Fix safety
/// The rule's fix is unsafe if the class has base classes. This is because the base classes might
/// be validating the class's other base classes (e.g., `typing.Protocol` does this) or otherwise
/// alter runtime behavior if more base classes are added.
///
/// ## References
/// - [Python documentation: `abc.ABC`](https://docs.python.org/3/library/abc.html#abc.ABC)
/// - [Python documentation: `abc.ABCMeta`](https://docs.python.org/3/library/abc.html#abc.ABCMeta)
Expand Down Expand Up @@ -69,6 +74,7 @@ pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) {
return;
}

let has_bases = !class_def.bases().is_empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we could use something like this here:

Suggested change
let has_bases = !class_def.bases().is_empty();
let applicability = if class_def.bases().is_empty() { Applicability::Safe } else { Applicability::Unsafe };

And then instead of the make_fix method, we can just use Fix::applicable_edits where we previously used Fix::safe_edits. You could then move your comments from make_fix here too, or possibly delete them too since I think you covered it nicely in the Fix safety section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. Nit happily applied.

let mut diagnostic = checker.report_diagnostic(MetaClassABCMeta, keyword.range);

diagnostic.try_set_fix(|| {
Expand All @@ -80,7 +86,8 @@ pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) {
Ok(if position > 0 {
// When the `abc.ABCMeta` is not the first keyword, put `abc.ABC` before the first
// keyword.
Fix::safe_edits(
make_fix(
has_bases,
// Delete from the previous argument, to the end of the `metaclass` argument.
Edit::range_deletion(TextRange::new(
class_def.keywords()[position - 1].end(),
Expand All @@ -93,10 +100,22 @@ pub(crate) fn metaclass_abcmeta(checker: &Checker, class_def: &StmtClassDef) {
],
)
} else {
Fix::safe_edits(
make_fix(
has_bases,
Edit::range_replacement(binding, keyword.range),
[import_edit],
)
})
});
}

fn make_fix(has_bases: bool, edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Fix {
if has_bases {
// If the class has bases, the fix is not safe as the base class might be
// validating the class's other base classes (e.g., `typing.Protocol` does this).
Fix::unsafe_edits(edit, rest)
} else {
// If the class doesn't have bases, the fix is safe.
Fix::safe_edits(edit, rest)
}
}
4 changes: 2 additions & 2 deletions ...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB180_FURB180.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ FURB180.py:26:18: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract
|
= help: Replace with `abc.ABC`

ℹ Safe fix
ℹ Unsafe fix
23 23 | pass
24 24 |
25 25 |
Expand All @@ -68,7 +68,7 @@ FURB180.py:31:34: FURB180 [*] Use of `metaclass=abc.ABCMeta` to define abstract
|
= help: Replace with `abc.ABC`

ℹ Safe fix
ℹ Unsafe fix
28 28 | def foo(self): pass
29 29 |
30 30 |
Expand Down
Loading