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 @@ -107,6 +107,10 @@ def f(self, x):
def f(self, x):
raise NotImplemented("...")

def f(self, x):
msg = "..."
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,71 @@ 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 matches!(value.as_ref(), ast::Expr::StringLiteral(_)) =>
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
{
rest
}
_ => &function_def.body,
};

let [first_stmt, second_stmt] = statements else {
return false;
};

let Stmt::Assign(ast::StmtAssign { value, .. }) = first_stmt 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 Stmt::Raise(StmtRaise {
exc: Some(exception),
..
}) = second_stmt
else {
return false;
};
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

let ast::Expr::Call(ast::ExprCall {
func, arguments, ..
}) = exception.as_ref()
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
}) = exception.as_ref()
}) = &**exception

Copy link
Contributor Author

@Watercycle Watercycle Oct 16, 2024

Choose a reason for hiding this comment

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

I see mixed usages of &** and as_ref throughout the code base. Out of curiosity, when is one used over the other? The general heuristic I've seen is * and & are recommended for simple stuff; and, once you start de-referencing and re-borrowing, that as_ref is recommended if just to avoid needing to think about whatever container the value* is in.

Copy link
Member

Choose a reason for hiding this comment

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

We used to use as_ref but started to prefer dereferencing (except for Option where you still need as_ref and as_deref) because it's possible that as_ref becomes ambiguous if a new AsRef implementation is added to a type. For example, String has many as_ref methods:

  = note: multiple `impl`s satisfying `String: AsRef<_>` found in the following crates: `alloc`, `std`:
          - impl AsRef<OsStr> for String;
          - impl AsRef<Path> for String;
          - impl AsRef<[u8]> for String;
          - impl AsRef<str> for String;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting, that makes sense! Thank you!

else {
return false;
};

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

if arguments.len() != 1 {
return false;
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

true
}

/// ARG001, ARG002, ARG003, ARG004, ARG005
pub(crate) fn unused_arguments(
checker: &Checker,
Expand Down Expand Up @@ -345,6 +410,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 +430,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 +456,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 +482,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,11 @@ ARG.py:43:16: ARG002 Unused method argument: `x`
44 | print("Hello, world!")
|

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


Loading