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

Make ARG002 compatible with EM101 when raising NotImplementedError #13714

Merged
merged 11 commits into from
Oct 18, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ def f(cls, x):
def f(x):
print("Hello, world!")

def f(self, x):
msg[0] = "..."
raise NotImplementedError(msg)

def f(self, x):
msg = "..."
raise NotImplementedError(foo)

def f(self, x):
msg = "..."
raise NotImplementedError("must use msg")

###
# Unused arguments attached to empty functions (OK).
###
Expand Down Expand Up @@ -107,6 +119,15 @@ def f(self, x):
def f(self, x):
raise NotImplemented("...")

def f(self, x):
msg = "..."
raise NotImplementedError(msg)

def f(self, x, y):
"""Docstring."""
msg = f"{x}..."
raise NotImplementedError(msg)

###
# Unused functions attached to abstract methods (OK).
###
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use regex::Regex;
use ruff_python_ast as ast;
use ruff_python_ast::{Parameter, Parameters};
use ruff_python_ast::{Parameter, Parameters, Stmt, StmtExpr, StmtFunctionDef, StmtRaise};

use ruff_diagnostics::DiagnosticKind;
use ruff_diagnostics::{Diagnostic, Violation};
Expand Down Expand Up @@ -311,6 +311,63 @@ fn call<'a>(
}));
}

/// Returns `true` if a function appears to be a base class stub. In other
/// words, if it matches the following syntax:
///
/// ```text
/// variable = <string | f-string>
/// raise NotImplementedError(variable)
/// ```
///
/// See also [`is_stub`]. We're a bit more generous in what is considered a
/// stub in this rule to avoid clashing with [`EM101`].
///
/// [`is_stub`]: function_type::is_stub
/// [`EM101`]: https://docs.astral.sh/ruff/rules/raw-string-in-exception/
fn is_not_implemented_stub_with_variable(function_def: &StmtFunctionDef) -> bool {
// Ignore doc-strings.
let statements = match function_def.body.as_slice() {
[Stmt::Expr(StmtExpr { value, .. }), rest @ ..] if value.is_string_literal_expr() => rest,
_ => &function_def.body,
};

let [Stmt::Assign(ast::StmtAssign { targets, value, .. }), Stmt::Raise(StmtRaise {
exc: Some(exception),
..
})] = statements
else {
return false;
};

if !matches!(
value.as_ref(),
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
ast::Expr::StringLiteral(_) | ast::Expr::FString(_)
) {
return false;
}

let ast::Expr::Call(ast::ExprCall {
func, arguments, ..
}) = &**exception
else {
return false;
};

if !matches!(&**func, ast::Expr::Name(name) if name.id == "NotImplementedError") {
return false;
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

let [argument] = &*arguments.args else {
return false;
};

let [target] = targets.as_slice() else {
return false;
};

argument.as_name_expr().map(|name| name.id()) == target.as_name_expr().map(|name| name.id())
}

/// ARG001, ARG002, ARG003, ARG004, ARG005
pub(crate) fn unused_arguments(
checker: &Checker,
Expand Down Expand Up @@ -345,6 +402,7 @@ pub(crate) fn unused_arguments(
function_type::FunctionType::Function => {
if checker.enabled(Argumentable::Function.rule_code())
&& !function_type::is_stub(function_def, checker.semantic())
&& !is_not_implemented_stub_with_variable(function_def)
&& !visibility::is_overload(decorator_list, checker.semantic())
{
function(
Expand All @@ -364,6 +422,7 @@ pub(crate) fn unused_arguments(
function_type::FunctionType::Method => {
if checker.enabled(Argumentable::Method.rule_code())
&& !function_type::is_stub(function_def, checker.semantic())
&& !is_not_implemented_stub_with_variable(function_def)
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
Expand All @@ -389,6 +448,7 @@ pub(crate) fn unused_arguments(
function_type::FunctionType::ClassMethod => {
if checker.enabled(Argumentable::ClassMethod.rule_code())
&& !function_type::is_stub(function_def, checker.semantic())
&& !is_not_implemented_stub_with_variable(function_def)
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
Expand All @@ -414,6 +474,7 @@ pub(crate) fn unused_arguments(
function_type::FunctionType::StaticMethod => {
if checker.enabled(Argumentable::StaticMethod.rule_code())
&& !function_type::is_stub(function_def, checker.semantic())
&& !is_not_implemented_stub_with_variable(function_def)
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,41 @@ ARG.py:43:16: ARG002 Unused method argument: `x`
44 | print("Hello, world!")
|

ARG.py:192:24: ARG002 Unused method argument: `x`
ARG.py:58:17: ARG002 Unused method argument: `x`
|
56 | print("Hello, world!")
57 |
58 | def f(self, x):
| ^ ARG002
59 | msg[0] = "..."
60 | raise NotImplementedError(msg)
|

ARG.py:62:17: ARG002 Unused method argument: `x`
|
60 | raise NotImplementedError(msg)
61 |
62 | def f(self, x):
| ^ ARG002
63 | msg = "..."
64 | raise NotImplementedError(foo)
|

ARG.py:66:17: ARG002 Unused method argument: `x`
|
64 | raise NotImplementedError(foo)
65 |
66 | def f(self, x):
| ^ ARG002
67 | msg = "..."
68 | raise NotImplementedError("must use msg")
|

ARG.py:213:24: ARG002 Unused method argument: `x`
|
190 | ###
191 | class C:
192 | def __init__(self, x) -> None:
211 | ###
212 | class C:
213 | def __init__(self, x) -> None:
| ^ ARG002
193 | print("Hello, world!")
214 | print("Hello, world!")
|


Loading