Skip to content

Commit

Permalink
Run dunder method rule on methods directly (#9815)
Browse files Browse the repository at this point in the history
This stood out in the flamegraph and I realized it requires us to
traverse over all statements in the class (unnecessarily).
  • Loading branch information
charliermarsh authored Feb 4, 2024
1 parent 5c99967 commit ad01216
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 33 deletions.
27 changes: 24 additions & 3 deletions crates/ruff_linter/src/checkers/ast/analyze/definitions.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
use ruff_python_ast::str::raw_contents_range;
use ruff_text_size::{Ranged, TextRange};

use ruff_python_semantic::{BindingKind, ContextualizedDefinition, Export};
use ruff_python_semantic::{
BindingKind, ContextualizedDefinition, Definition, Export, Member, MemberKind,
};

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::docstrings::Docstring;
use crate::fs::relativize_path;
use crate::rules::{flake8_annotations, flake8_pyi, pydocstyle};
use crate::rules::{flake8_annotations, flake8_pyi, pydocstyle, pylint};
use crate::{docstrings, warn_user};

/// Run lint rules over all [`Definition`] nodes in the [`SemanticModel`].
Expand All @@ -31,6 +33,7 @@ pub(crate) fn definitions(checker: &mut Checker) {
]);
let enforce_stubs = checker.source_type.is_stub() && checker.enabled(Rule::DocstringInStub);
let enforce_stubs_and_runtime = checker.enabled(Rule::IterMethodReturnIterable);
let enforce_dunder_method = checker.enabled(Rule::BadDunderMethodName);
let enforce_docstrings = checker.any_enabled(&[
Rule::BlankLineAfterLastSection,
Rule::BlankLineAfterSummary,
Expand Down Expand Up @@ -80,7 +83,12 @@ pub(crate) fn definitions(checker: &mut Checker) {
Rule::UndocumentedPublicPackage,
]);

if !enforce_annotations && !enforce_docstrings && !enforce_stubs && !enforce_stubs_and_runtime {
if !enforce_annotations
&& !enforce_docstrings
&& !enforce_stubs
&& !enforce_stubs_and_runtime
&& !enforce_dunder_method
{
return;
}

Expand Down Expand Up @@ -147,6 +155,19 @@ pub(crate) fn definitions(checker: &mut Checker) {
}
}

// pylint
if enforce_dunder_method {
if checker.enabled(Rule::BadDunderMethodName) {
if let Definition::Member(Member {
kind: MemberKind::Method(method),
..
}) = definition
{
pylint::rules::bad_dunder_method_name(checker, method);
}
}
}

// pydocstyle
if enforce_docstrings {
if pydocstyle::helpers::should_ignore_definition(
Expand Down
3 changes: 0 additions & 3 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,9 +513,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::SingleStringSlots) {
pylint::rules::single_string_slots(checker, class_def);
}
if checker.enabled(Rule::BadDunderMethodName) {
pylint::rules::bad_dunder_method_name(checker, body);
}
if checker.enabled(Rule::MetaClassABCMeta) {
refurb::rules::metaclass_abcmeta(checker, class_def);
}
Expand Down
54 changes: 27 additions & 27 deletions crates/ruff_linter/src/rules/pylint/rules/bad_dunder_method_name.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::identifier::Identifier;
use ruff_python_ast::Stmt;
use ruff_python_semantic::analyze::visibility;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -56,32 +56,32 @@ impl Violation for BadDunderMethodName {
}

/// PLW3201
pub(crate) fn bad_dunder_method_name(checker: &mut Checker, class_body: &[Stmt]) {
for method in class_body
.iter()
.filter_map(ruff_python_ast::Stmt::as_function_def_stmt)
.filter(|method| {
if is_known_dunder_method(&method.name)
|| checker
.settings
.pylint
.allow_dunder_method_names
.contains(method.name.as_str())
|| matches!(method.name.as_str(), "_")
{
return false;
}
method.name.starts_with('_') && method.name.ends_with('_')
})
pub(crate) fn bad_dunder_method_name(checker: &mut Checker, method: &ast::StmtFunctionDef) {
// If the name isn't a dunder, skip it.
if !method.name.starts_with('_') || !method.name.ends_with('_') {
return;
}

// If the name is explicitly allowed, skip it.
if is_known_dunder_method(&method.name)
|| checker
.settings
.pylint
.allow_dunder_method_names
.contains(method.name.as_str())
|| matches!(method.name.as_str(), "_")
{
if visibility::is_override(&method.decorator_list, checker.semantic()) {
continue;
}
checker.diagnostics.push(Diagnostic::new(
BadDunderMethodName {
name: method.name.to_string(),
},
method.identifier(),
));
return;
}

if visibility::is_override(&method.decorator_list, checker.semantic()) {
return;
}

checker.diagnostics.push(Diagnostic::new(
BadDunderMethodName {
name: method.name.to_string(),
},
method.identifier(),
));
}

0 comments on commit ad01216

Please sign in to comment.