From 9d48d7bbd1ca1cbd8ccdc9ba3cfb30fae02eff08 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 11 Jan 2023 19:02:20 -0500 Subject: [PATCH] Skip unused argument checks for magic methods (#1801) We still check `__init__`, `__call__`, and `__new__`. Closes #1796. --- .../fixtures/flake8_unused_arguments/ARG.py | 14 ++++++++ src/ast/cast.rs | 7 ++++ src/flake8_annotations/rules.rs | 4 +-- src/flake8_unused_arguments/rules.rs | 12 +++++++ ...nused_arguments__tests__ARG002_ARG.py.snap | 12 ++++++- src/pydocstyle/rules.rs | 24 ++++++++++---- src/visibility.rs | 32 ++++++++----------- 7 files changed, 77 insertions(+), 28 deletions(-) diff --git a/resources/test/fixtures/flake8_unused_arguments/ARG.py b/resources/test/fixtures/flake8_unused_arguments/ARG.py index aa5d42a60c..495fcc496f 100644 --- a/resources/test/fixtures/flake8_unused_arguments/ARG.py +++ b/resources/test/fixtures/flake8_unused_arguments/ARG.py @@ -181,3 +181,17 @@ def f(a: int, b: int) -> str: def f(a, b): return f"{a}{b}" + + +### +# Unused arguments on magic methods. +### +class C: + def __init__(self, x) -> None: + print("Hello, world!") + + def __str__(self) -> str: + return "Hello, world!" + + def __exit__(self, exc_type, exc_value, traceback) -> None: + print("Hello, world!") diff --git a/src/ast/cast.rs b/src/ast/cast.rs index d4bca521dc..2d1c210661 100644 --- a/src/ast/cast.rs +++ b/src/ast/cast.rs @@ -1,5 +1,12 @@ use rustpython_ast::{Expr, Stmt, StmtKind}; +pub fn name(stmt: &Stmt) -> &str { + match &stmt.node { + StmtKind::FunctionDef { name, .. } | StmtKind::AsyncFunctionDef { name, .. } => name, + _ => panic!("Expected StmtKind::FunctionDef | StmtKind::AsyncFunctionDef"), + } +} + pub fn decorator_list(stmt: &Stmt) -> &Vec { match &stmt.node { StmtKind::FunctionDef { decorator_list, .. } diff --git a/src/flake8_annotations/rules.rs b/src/flake8_annotations/rules.rs index e26a2a442d..78151340d1 100644 --- a/src/flake8_annotations/rules.rs +++ b/src/flake8_annotations/rules.rs @@ -319,7 +319,7 @@ pub fn definition(checker: &mut Checker, definition: &Definition, visibility: &V helpers::identifier_range(stmt, checker.locator), )); } - } else if visibility::is_init(stmt) { + } else if visibility::is_init(cast::name(stmt)) { // Allow omission of return annotation in `__init__` functions, as long as at // least one argument is typed. if checker.settings.enabled.contains(&RuleCode::ANN204) { @@ -341,7 +341,7 @@ pub fn definition(checker: &mut Checker, definition: &Definition, visibility: &V checker.diagnostics.push(diagnostic); } } - } else if visibility::is_magic(stmt) { + } else if visibility::is_magic(cast::name(stmt)) { if checker.settings.enabled.contains(&RuleCode::ANN204) { checker.diagnostics.push(Diagnostic::new( violations::MissingReturnTypeSpecialMethod(name.to_string()), diff --git a/src/flake8_unused_arguments/rules.rs b/src/flake8_unused_arguments/rules.rs index 42db9a45a3..56c1bf33d2 100644 --- a/src/flake8_unused_arguments/rules.rs +++ b/src/flake8_unused_arguments/rules.rs @@ -153,6 +153,10 @@ pub fn unused_arguments( .enabled .contains(Argumentable::Method.rule_code()) && !helpers::is_empty(body) + && (!visibility::is_magic(name) + || visibility::is_init(name) + || visibility::is_new(name) + || visibility::is_call(name)) && !visibility::is_abstract(checker, decorator_list) && !visibility::is_override(checker, decorator_list) && !visibility::is_overload(checker, decorator_list) @@ -178,6 +182,10 @@ pub fn unused_arguments( .enabled .contains(Argumentable::ClassMethod.rule_code()) && !helpers::is_empty(body) + && (!visibility::is_magic(name) + || visibility::is_init(name) + || visibility::is_new(name) + || visibility::is_call(name)) && !visibility::is_abstract(checker, decorator_list) && !visibility::is_override(checker, decorator_list) && !visibility::is_overload(checker, decorator_list) @@ -203,6 +211,10 @@ pub fn unused_arguments( .enabled .contains(Argumentable::StaticMethod.rule_code()) && !helpers::is_empty(body) + && (!visibility::is_magic(name) + || visibility::is_init(name) + || visibility::is_new(name) + || visibility::is_call(name)) && !visibility::is_abstract(checker, decorator_list) && !visibility::is_override(checker, decorator_list) && !visibility::is_overload(checker, decorator_list) diff --git a/src/flake8_unused_arguments/snapshots/ruff__flake8_unused_arguments__tests__ARG002_ARG.py.snap b/src/flake8_unused_arguments/snapshots/ruff__flake8_unused_arguments__tests__ARG002_ARG.py.snap index 0fae5a444a..3d13df950e 100644 --- a/src/flake8_unused_arguments/snapshots/ruff__flake8_unused_arguments__tests__ARG002_ARG.py.snap +++ b/src/flake8_unused_arguments/snapshots/ruff__flake8_unused_arguments__tests__ARG002_ARG.py.snap @@ -1,6 +1,6 @@ --- source: src/flake8_unused_arguments/mod.rs -expression: checks +expression: diagnostics --- - kind: UnusedMethodArgument: x @@ -32,4 +32,14 @@ expression: checks column: 16 fix: ~ parent: ~ +- kind: + UnusedMethodArgument: x + location: + row: 190 + column: 23 + end_location: + row: 190 + column: 24 + fix: ~ + parent: ~ diff --git a/src/pydocstyle/rules.rs b/src/pydocstyle/rules.rs index 3f6aa481a5..d75d3093f7 100644 --- a/src/pydocstyle/rules.rs +++ b/src/pydocstyle/rules.rs @@ -18,7 +18,9 @@ use crate::pydocstyle::helpers::{leading_quote, logical_line}; use crate::pydocstyle::settings::Convention; use crate::registry::{Diagnostic, RuleCode}; use crate::violations; -use crate::visibility::{is_init, is_magic, is_overload, is_override, is_staticmethod, Visibility}; +use crate::visibility::{ + is_call, is_init, is_magic, is_new, is_overload, is_override, is_staticmethod, Visibility, +}; /// D100, D101, D102, D103, D104, D105, D106, D107 pub fn not_missing( @@ -85,18 +87,26 @@ pub fn not_missing( || is_override(checker, cast::decorator_list(stmt)) { true - } else if is_magic(stmt) { - if checker.settings.enabled.contains(&RuleCode::D105) { + } else if is_init(cast::name(stmt)) { + if checker.settings.enabled.contains(&RuleCode::D107) { checker.diagnostics.push(Diagnostic::new( - violations::MagicMethod, + violations::PublicInit, identifier_range(stmt, checker.locator), )); } true - } else if is_init(stmt) { - if checker.settings.enabled.contains(&RuleCode::D107) { + } else if is_new(cast::name(stmt)) || is_call(cast::name(stmt)) { + if checker.settings.enabled.contains(&RuleCode::D102) { checker.diagnostics.push(Diagnostic::new( - violations::PublicInit, + violations::PublicMethod, + identifier_range(stmt, checker.locator), + )); + } + true + } else if is_magic(cast::name(stmt)) { + if checker.settings.enabled.contains(&RuleCode::D105) { + checker.diagnostics.push(Diagnostic::new( + violations::MagicMethod, identifier_range(stmt, checker.locator), )); } diff --git a/src/visibility.rs b/src/visibility.rs index 433058d355..95ac409a9e 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -82,27 +82,23 @@ pub fn is_abstract(checker: &Checker, decorator_list: &[Expr]) -> bool { } /// Returns `true` if a function is a "magic method". -pub fn is_magic(stmt: &Stmt) -> bool { - match &stmt.node { - StmtKind::FunctionDef { name, .. } | StmtKind::AsyncFunctionDef { name, .. } => { - name.starts_with("__") - && name.ends_with("__") - && name != "__init__" - && name != "__call__" - && name != "__new__" - } - _ => panic!("Found non-FunctionDef in is_magic"), - } +pub fn is_magic(name: &str) -> bool { + name.starts_with("__") && name.ends_with("__") } /// Returns `true` if a function is an `__init__`. -pub fn is_init(stmt: &Stmt) -> bool { - match &stmt.node { - StmtKind::FunctionDef { name, .. } | StmtKind::AsyncFunctionDef { name, .. } => { - name == "__init__" - } - _ => panic!("Found non-FunctionDef in is_init"), - } +pub fn is_init(name: &str) -> bool { + name == "__init__" +} + +/// Returns `true` if a function is an `__new__`. +pub fn is_new(name: &str) -> bool { + name == "__new__" +} + +/// Returns `true` if a function is an `__call__`. +pub fn is_call(name: &str) -> bool { + name == "__call__" } /// Returns `true` if a module name indicates public visibility.