Skip to content

Commit

Permalink
Rename rule; add fix; add docs
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Mar 1, 2024
1 parent 6fb1272 commit 6702c4d
Show file tree
Hide file tree
Showing 9 changed files with 206 additions and 98 deletions.
20 changes: 0 additions & 20 deletions crates/ruff_linter/resources/test/fixtures/pylint/E1519.py

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
from functools import singledispatch, singledispatchmethod


@singledispatch
def convert_position(position):
pass


class Board:
@singledispatch # [singledispatch-method]
@classmethod
def convert_position(cls, position):
pass

@singledispatch # [singledispatch-method]
def move(self, position):
pass

@singledispatchmethod
def place(self, position):
pass

@singledispatch
@staticmethod
def do(position):
pass

# False negative (flagged by Pylint).
@convert_position.register
@classmethod
def _(cls, position: str) -> tuple:
position_a, position_b = position.split(",")
return (int(position_a), int(position_b))

# False negative (flagged by Pylint).
@convert_position.register
@classmethod
def _(cls, position: tuple) -> str:
return f"{position[0]},{position[1]}"
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
Rule::UnusedPrivateTypedDict,
Rule::UnusedStaticMethodArgument,
Rule::UnusedVariable,
Rule::SingleDispatchMethod,
Rule::SingledispatchMethod,
]) {
return;
}
Expand Down Expand Up @@ -391,8 +391,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
pylint::rules::too_many_locals(checker, scope, &mut diagnostics);
}

if checker.enabled(Rule::SingleDispatchMethod) {
pylint::rules::single_dispatch_method(checker, scope, &mut diagnostics);
if checker.enabled(Rule::SingledispatchMethod) {
pylint::rules::singledispatch_method(checker, scope, &mut diagnostics);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "E1307") => (RuleGroup::Stable, rules::pylint::rules::BadStringFormatType),
(Pylint, "E1310") => (RuleGroup::Stable, rules::pylint::rules::BadStrStripCall),
(Pylint, "E1507") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarValue),
(Pylint, "E1519") => (RuleGroup::Preview, rules::pylint::rules::SingleDispatchMethod),
(Pylint, "E1519") => (RuleGroup::Preview, rules::pylint::rules::SingledispatchMethod),
(Pylint, "E1700") => (RuleGroup::Stable, rules::pylint::rules::YieldFromInAsyncFunction),
(Pylint, "E2502") => (RuleGroup::Stable, rules::pylint::rules::BidirectionalUnicode),
(Pylint, "E2510") => (RuleGroup::Stable, rules::pylint::rules::InvalidCharacterBackspace),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ mod tests {
use crate::settings::LinterSettings;
use crate::test::test_path;

#[test_case(Rule::SingleDispatchMethod, Path::new("E1519.py"))]
#[test_case(Rule::SingledispatchMethod, Path::new("singledispatch_method.py"))]
#[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))]
#[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))]
#[test_case(Rule::BadOpenMode, Path::new("bad_open_mode.py"))]
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ pub(crate) use repeated_isinstance_calls::*;
pub(crate) use repeated_keyword_argument::*;
pub(crate) use return_in_init::*;
pub(crate) use self_assigning_variable::*;
pub(crate) use single_dispatch_method::*;
pub(crate) use single_string_slots::*;
pub(crate) use singledispatch_method::*;
pub(crate) use subprocess_popen_preexec_fn::*;
pub(crate) use subprocess_run_without_check::*;
pub(crate) use super_without_brackets::*;
Expand Down Expand Up @@ -140,8 +140,8 @@ mod repeated_isinstance_calls;
mod repeated_keyword_argument;
mod return_in_init;
mod self_assigning_variable;
mod single_dispatch_method;
mod single_string_slots;
mod singledispatch_method;
mod subprocess_popen_preexec_fn;
mod subprocess_run_without_check;
mod super_without_brackets;
Expand Down

This file was deleted.

117 changes: 117 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/singledispatch_method.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_semantic::analyze::function_type;
use ruff_python_semantic::Scope;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;

/// ## What it does
/// Checks for `@singledispatch` decorators on class and instance methods.
///
/// ## Why is this bad?
/// The `@singledispatch` decorator is intended for use with functions, not methods.
///
/// Instead, use the `@singledispatchmethod` decorator, or migrate the method to a
/// standalone function or `@staticmethod`.
///
/// ## Example
/// ```python
/// from functools import singledispatch
///
/// class Class:
/// @singledispatch
/// def method(self, arg):
/// ...
/// ```
///
/// Use instead:
/// ```python
/// from functools import singledispatchmethod
///
/// class Class:
/// @singledispatchmethod
/// def method(self, arg):
/// ...
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as migrating from `@singledispatch` to
/// `@singledispatchmethod` may change the behavior of the code.
#[violation]
pub struct SingledispatchMethod;

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

#[derive_message_formats]
fn message(&self) -> String {
format!("`@singledispatch` decorator should not be used on methods")
}

fn fix_title(&self) -> Option<String> {
Some("Replace with `@singledispatchmethod`".to_string())
}
}

/// E1519
pub(crate) fn singledispatch_method(
checker: &Checker,
scope: &Scope,
diagnostics: &mut Vec<Diagnostic>,
) {
let Some(func) = scope.kind.as_function() else {
return;
};

let ast::StmtFunctionDef {
name,
decorator_list,
..
} = func;

let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else {
return;
};

let type_ = function_type::classify(
name,
decorator_list,
parent,
checker.semantic(),
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
);
if !matches!(
type_,
function_type::FunctionType::Method | function_type::FunctionType::ClassMethod
) {
return;
}

for decorator in decorator_list {
if checker
.semantic()
.resolve_call_path(&decorator.expression)
.is_some_and(|call_path| {
matches!(call_path.as_slice(), ["functools", "singledispatch"])
})
{
let mut diagnostic = Diagnostic::new(SingledispatchMethod, decorator.range());
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import("functools", "singledispatchmethod"),
decorator.start(),
checker.semantic(),
)?;
Ok(Fix::unsafe_edits(
Edit::range_replacement(binding, decorator.expression.range()),
[import_edit],
))
});
diagnostics.push(diagnostic);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
singledispatch_method.py:10:5: PLE1519 [*] `@singledispatch` decorator should not be used on methods
|
9 | class Board:
10 | @singledispatch # [singledispatch-method]
| ^^^^^^^^^^^^^^^ PLE1519
11 | @classmethod
12 | def convert_position(cls, position):
|
= help: Replace with `@singledispatchmethod`

Unsafe fix
7 7 |
8 8 |
9 9 | class Board:
10 |- @singledispatch # [singledispatch-method]
10 |+ @singledispatchmethod # [singledispatch-method]
11 11 | @classmethod
12 12 | def convert_position(cls, position):
13 13 | pass

singledispatch_method.py:15:5: PLE1519 [*] `@singledispatch` decorator should not be used on methods
|
13 | pass
14 |
15 | @singledispatch # [singledispatch-method]
| ^^^^^^^^^^^^^^^ PLE1519
16 | def move(self, position):
17 | pass
|
= help: Replace with `@singledispatchmethod`

Unsafe fix
12 12 | def convert_position(cls, position):
13 13 | pass
14 14 |
15 |- @singledispatch # [singledispatch-method]
15 |+ @singledispatchmethod # [singledispatch-method]
16 16 | def move(self, position):
17 17 | pass
18 18 |

0 comments on commit 6702c4d

Please sign in to comment.