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

[pylint] Implement PLE0302 unexpected-special-method-signature #4075

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
class TestClass:
def __bool__(self):
...

def __bool__(self, x): # too many mandatory args
...

def __bool__(self, x=1): # additional optional args OK
...

def __bool__(self, *args): # varargs OK
...

def __bool__(): # ignored; should be caughty by E0211/N805
...

@staticmethod
def __bool__():
...

@staticmethod
def __bool__(x): # too many mandatory args
...

@staticmethod
def __bool__(x=1): # additional optional args OK
...

def __eq__(self, other): # multiple args
...

def __eq__(self, other=1): # expected arg is optional
...

def __eq__(self): # too few mandatory args
...

def __eq__(self, other, other_other): # too many mandatory args
...

def __round__(self): # allow zero additional args.
...

def __round__(self, x): # allow one additional args.
...

def __round__(self, x, y): # disallow 2 args
...

def __round__(self, x, y, z=2): # disallow 3 args even when one is optional
...
15 changes: 15 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,21 @@ where
);
}

if self
.settings
.rules
.enabled(Rule::UnexpectedSpecialMethodSignature)
{
pylint::rules::unexpected_special_method_signature(
self,
stmt,
name,
decorator_list,
args,
self.locator,
);
}

self.check_builtin_shadowing(name, stmt, true);

// Visit the decorators and arguments, but avoid the body, which will be
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Pylint, "W0711") => Rule::BinaryOpException,
(Pylint, "W1508") => Rule::InvalidEnvvarDefault,
(Pylint, "W2901") => Rule::RedefinedLoopName,
(Pylint, "E0302") => Rule::UnexpectedSpecialMethodSignature,

// flake8-builtins
(Flake8Builtins, "001") => Rule::BuiltinVariableShadowing,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ ruff_macros::register_rules!(
rules::pylint::rules::RedefinedLoopName,
rules::pylint::rules::LoggingTooFewArgs,
rules::pylint::rules::LoggingTooManyArgs,
rules::pylint::rules::UnexpectedSpecialMethodSignature,
// flake8-builtins
rules::flake8_builtins::rules::BuiltinVariableShadowing,
rules::flake8_builtins::rules::BuiltinArgumentShadowing,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ mod tests {
#[test_case(Rule::TooManyBranches, Path::new("too_many_branches.py"); "PLR0912")]
#[test_case(Rule::TooManyReturnStatements, Path::new("too_many_return_statements.py"); "PLR0911")]
#[test_case(Rule::TooManyStatements, Path::new("too_many_statements.py"); "PLR0915")]
#[test_case(Rule::UnexpectedSpecialMethodSignature, Path::new("unexpected_special_method_signature.py"); "PLE0302")]
#[test_case(Rule::UnnecessaryDirectLambdaCall, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")]
#[test_case(Rule::LoadBeforeGlobalDeclaration, Path::new("load_before_global_declaration.py"); "PLE0118")]
#[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"); "PLW0120")]
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ pub use too_many_arguments::{too_many_arguments, TooManyArguments};
pub use too_many_branches::{too_many_branches, TooManyBranches};
pub use too_many_return_statements::{too_many_return_statements, TooManyReturnStatements};
pub use too_many_statements::{too_many_statements, TooManyStatements};
pub use unexpected_special_method_signature::{
unexpected_special_method_signature, UnexpectedSpecialMethodSignature,
};
pub use unnecessary_direct_lambda_call::{
unnecessary_direct_lambda_call, UnnecessaryDirectLambdaCall,
};
Expand Down Expand Up @@ -73,6 +76,7 @@ mod too_many_arguments;
mod too_many_branches;
mod too_many_return_statements;
mod too_many_statements;
mod unexpected_special_method_signature;
mod unnecessary_direct_lambda_call;
mod useless_else_on_loop;
mod useless_import_alias;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
use std::ops::RangeInclusive;

use rustpython_parser::ast::{Arguments, Expr, Stmt};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::identifier_range;
use ruff_python_ast::source_code::Locator;
use ruff_python_semantic::analyze::visibility::is_staticmethod;
use ruff_python_semantic::scope::ScopeKind;

use crate::checkers::ast::Checker;

#[violation]
pub struct UnexpectedSpecialMethodSignature {
pub method_name: String,
pub expected_params: std::ops::RangeInclusive<usize>,
pub actual_params: usize,
}

impl Violation for UnexpectedSpecialMethodSignature {
#[derive_message_formats]
fn message(&self) -> String {
let verb = if self.actual_params > 1 {
"were"
} else {
"was"
};
if self.expected_params.end() - self.expected_params.start() > 1 {
format!(
"The special method '{}' expects between {} and {} param(s), {} {} given",
self.method_name,
self.expected_params.start(),
self.expected_params.end(),
self.actual_params,
verb,
)
} else {
format!(
"The special method '{}' expects {} param(s), {} {} given",
mccullocht marked this conversation as resolved.
Show resolved Hide resolved
self.method_name,
self.expected_params.start(),
self.actual_params,
verb,
)
}
}
}

fn expected_params(name: &str, is_staticmethod: bool) -> Option<RangeInclusive<usize>> {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than modeling each case as RangeInclusive, and using self.expected_params.end() - self.expected_params.start() > 1-like checks to determine whether a branch should be treated as a range or not, what if we introduced an enum here to represent the two cases? E.g.:

enum ExpectedParameters {
  Fixed(usize),
  // Or RangeInclusive<usize>
  Range(usize, usize),
}

(Names etc. are of course flexible, just illustrating the concept.)

I think that would make some of the case-handling below a bit clearer by way of being more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I used the new enum in the violation so it has to be public :/. If you have a preference expected_params can be rendered into a string for formatting before creating the violation, which would allow ExpectedParams to be private again.

Copy link
Member

Choose a reason for hiding this comment

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

All good, we follow this pattern after and the enums end up public. No worries.

match name {
"__del__" | "__repr__" | "__str__" | "__bytes__" | "__hash__" | "__bool__" | "__dir__"
| "__len__" | "__length_hint__" | "__iter__" | "__reversed__" | "__neg__" | "__pos__"
| "__abs__" | "__invert__" | "__complex__" | "__int__" | "__float__" | "__index__"
| "__trunc__" | "__floor__" | "__ceil__" | "__enter__" | "__aenter__"
| "__getnewargs_ex__" | "__getnewargs__" | "__getstate__" | "__reduce__" | "__copy__"
| "__unicode__" | "__nonzero__" | "__await__" | "__aiter__" | "__anext__"
| "__fspath__" | "__subclasses__" => Some(0..=0),
"__format__" | "__lt__" | "__le__" | "__eq__" | "__ne__" | "__gt__" | "__ge__"
| "__getattr__" | "__getattribute__" | "__delattr__" | "__delete__"
| "__instancecheck__" | "__subclasscheck__" | "__getitem__" | "__missing__"
| "__delitem__" | "__contains__" | "__add__" | "__sub__" | "__mul__" | "__truediv__"
| "__floordiv__" | "__rfloordiv__" | "__mod__" | "__divmod__" | "__lshift__"
| "__rshift__" | "__and__" | "__xor__" | "__or__" | "__radd__" | "__rsub__"
| "__rmul__" | "__rtruediv__" | "__rmod__" | "__rdivmod__" | "__rpow__" | "__rlshift__"
| "__rrshift__" | "__rand__" | "__rxor__" | "__ror__" | "__iadd__" | "__isub__"
| "__imul__" | "__itruediv__" | "__ifloordiv__" | "__imod__" | "__ilshift__"
| "__irshift__" | "__iand__" | "__ixor__" | "__ior__" | "__ipow__" | "__setstate__"
| "__reduce_ex__" | "__deepcopy__" | "__cmp__" | "__matmul__" | "__rmatmul__"
| "__imatmul__" | "__div__" => Some(1..=1),
"__setattr__" | "__get__" | "__set__" | "__setitem__" | "__set_name__" => Some(2..=2),
"__exit__" | "__aexit__" => Some(3..=3),
"__round__" => Some(0..=1),
"__pow__" => Some(1..=2),
_ => return None,
}
.map(|r| {
if is_staticmethod {
r
} else {
RangeInclusive::new(r.start() + 1, r.end() + 1)
Copy link
Member

Choose a reason for hiding this comment

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

(This could even be an associated function on ExpectedParameters, to increment the count.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ExpectedParams::from_method() takes an is_staticmethod bool and applies this internally on construction, but I got rid of the map() call.

}
})
}

/// PLE0302
pub fn unexpected_special_method_signature(
checker: &mut Checker,
stmt: &Stmt,
name: &str,
decorator_list: &[Expr],
args: &Arguments,
locator: &Locator,
) {
if !matches!(checker.ctx.scope().kind, ScopeKind::Class(_)) {
return;
}

// Method has no parameter, will be caught by no-method-argument (E0211/N805).
if args.args.is_empty() && args.vararg.is_none() {
return;
}

let actual_params = args.args.len();
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to take into account positional-only and/or keyword-only arguments? (Elsewhere, we have logic like let defaults_start = args.posonlyargs.len() + args.args.len() - args.defaults.len(); and let defaults_start = args.kwonlyargs.len() - args.kw_defaults.len(); to infer some similar information from function signatures.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I patterned off of the pylint check which did not take posonly/kwonly args into account, but I can investigate further if you'd like. I don't think we need to consider kw-only arguments as it doesn't seem that any special methods have them according to the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we want to extend the check with a custom message when posonly/kwonly args are included, since that itself deviates from the expected signature, albeit in a different way?

let optional_params = args.defaults.len();
let mandatory_params = actual_params - optional_params;

if let Some(expected_params) =
mccullocht marked this conversation as resolved.
Show resolved Hide resolved
expected_params(name, is_staticmethod(&checker.ctx, decorator_list))
{
let emit = if expected_params.end() - expected_params.start() > 1 {
!expected_params.contains(&actual_params)
} else if *expected_params.start() < mandatory_params {
true
} else if *expected_params.start() > mandatory_params {
args.vararg.is_none() && optional_params < (*expected_params.start() - mandatory_params)
} else {
false
};

if emit {
checker.diagnostics.push(Diagnostic::new(
UnexpectedSpecialMethodSignature {
method_name: name.to_owned(),
expected_params,
actual_params,
},
identifier_range(stmt, locator),
));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
---
unexpected_special_method_signature.py:5:9: PLE0302 The special method '__bool__' expects 1 param(s), 2 were given
|
5 | ...
6 |
7 | def __bool__(self, x): # too many mandatory args
| ^^^^^^^^ PLE0302
8 | ...
|

unexpected_special_method_signature.py:22:9: PLE0302 The special method '__bool__' expects 0 param(s), 1 was given
|
22 | @staticmethod
23 | def __bool__(x): # too many mandatory args
| ^^^^^^^^ PLE0302
24 | ...
|

unexpected_special_method_signature.py:35:9: PLE0302 The special method '__eq__' expects 2 param(s), 1 was given
|
35 | ...
36 |
37 | def __eq__(self): # too few mandatory args
| ^^^^^^ PLE0302
38 | ...
|

unexpected_special_method_signature.py:38:9: PLE0302 The special method '__eq__' expects 2 param(s), 3 were given
|
38 | ...
39 |
40 | def __eq__(self, other, other_other): # too many mandatory args
| ^^^^^^ PLE0302
41 | ...
|

unexpected_special_method_signature.py:44:9: PLE0302 The special method '__round__' expects 1 param(s), 2 were given
|
44 | ...
45 |
46 | def __round__(self, x): # allow one additional args.
| ^^^^^^^^^ PLE0302
47 | ...
|

unexpected_special_method_signature.py:47:9: PLE0302 The special method '__round__' expects 1 param(s), 3 were given
|
47 | ...
48 |
49 | def __round__(self, x, y): # disallow 2 args
| ^^^^^^^^^ PLE0302
50 | ...
|

unexpected_special_method_signature.py:50:9: PLE0302 The special method '__round__' expects 1 param(s), 4 were given
|
50 | ...
51 |
52 | def __round__(self, x, y, z=2): # disallow 3 args even when one is optional
| ^^^^^^^^^ PLE0302
53 | ...
|


3 changes: 3 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.