Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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,60 @@ 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, 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(ast::ExprName::id) == target.as_name_expr().map(ast::ExprName::id)
}

/// ARG001, ARG002, ARG003, ARG004, ARG005
pub(crate) fn unused_arguments(
checker: &Checker,
Expand Down Expand Up @@ -345,6 +399,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 +419,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 +445,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 +471,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