From 1ccfc517d0f56bbf2ae6a2efc7d3bc1f5321c00b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 15 Dec 2023 12:52:58 -0500 Subject: [PATCH] Add base-class inheritance detection to flake8-django rules --- .../test/fixtures/flake8_django/DJ012.py | 18 ++++ .../src/checkers/ast/analyze/statement.rs | 20 +--- .../rules/all_with_model_form.rs | 31 +++--- .../rules/exclude_with_model_form.rs | 26 ++--- .../src/rules/flake8_django/rules/helpers.rs | 12 +-- .../rules/model_without_dunder_str.rs | 61 ++++-------- .../rules/unordered_body_content_in_model.rs | 97 +++++++++---------- ..._flake8_django__tests__DJ012_DJ012.py.snap | 8 ++ .../src/rules/flake8_type_checking/helpers.rs | 57 ++--------- .../ruff_python_semantic/src/analyze/class.rs | 57 +++++++++++ .../ruff_python_semantic/src/analyze/mod.rs | 1 + 11 files changed, 189 insertions(+), 199 deletions(-) create mode 100644 crates/ruff_python_semantic/src/analyze/class.rs diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ012.py b/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ012.py index a0f8d9da221a0b..20d3e4078ba965 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ012.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ012.py @@ -127,3 +127,21 @@ def get_absolute_url(self): pass middle_name = models.CharField(max_length=32) + + +class BaseModel(models.Model): + pass + + +class StrBeforeFieldInheritedModel(BaseModel): + """Model with `__str__` before fields.""" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + return "foobar" + + first_name = models.CharField(max_length=32) + diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 5fb77629a28c8d..aee17b44368d4f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -397,27 +397,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_django::rules::nullable_model_string_field(checker, body); } if checker.enabled(Rule::DjangoExcludeWithModelForm) { - if let Some(diagnostic) = flake8_django::rules::exclude_with_model_form( - checker, - arguments.as_deref(), - body, - ) { - checker.diagnostics.push(diagnostic); - } + flake8_django::rules::exclude_with_model_form(checker, class_def); } if checker.enabled(Rule::DjangoAllWithModelForm) { - if let Some(diagnostic) = - flake8_django::rules::all_with_model_form(checker, arguments.as_deref(), body) - { - checker.diagnostics.push(diagnostic); - } + flake8_django::rules::all_with_model_form(checker, class_def) } if checker.enabled(Rule::DjangoUnorderedBodyContentInModel) { - flake8_django::rules::unordered_body_content_in_model( - checker, - arguments.as_deref(), - body, - ); + flake8_django::rules::unordered_body_content_in_model(checker, class_def); } if !checker.source_type.is_stub() { if checker.enabled(Rule::DjangoModelWithoutDunderStr) { diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/all_with_model_form.rs b/crates/ruff_linter/src/rules/flake8_django/rules/all_with_model_form.rs index 8e97c68c2ca0f5..8083575e2a59c6 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/all_with_model_form.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/all_with_model_form.rs @@ -1,7 +1,6 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -48,21 +47,12 @@ impl Violation for DjangoAllWithModelForm { } /// DJ007 -pub(crate) fn all_with_model_form( - checker: &Checker, - arguments: Option<&Arguments>, - body: &[Stmt], -) -> Option { - if !arguments.is_some_and(|arguments| { - arguments - .args - .iter() - .any(|base| is_model_form(base, checker.semantic())) - }) { - return None; +pub(crate) fn all_with_model_form(checker: &mut Checker, class_def: &ast::StmtClassDef) { + if !is_model_form(class_def, checker.semantic()) { + return; } - for element in body { + for element in &class_def.body { let Stmt::ClassDef(ast::StmtClassDef { name, body, .. }) = element else { continue; }; @@ -83,12 +73,18 @@ pub(crate) fn all_with_model_form( match value.as_ref() { Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => { if value == "__all__" { - return Some(Diagnostic::new(DjangoAllWithModelForm, element.range())); + checker + .diagnostics + .push(Diagnostic::new(DjangoAllWithModelForm, element.range())); + return; } } Expr::BytesLiteral(ast::ExprBytesLiteral { value, .. }) => { if value == "__all__".as_bytes() { - return Some(Diagnostic::new(DjangoAllWithModelForm, element.range())); + checker + .diagnostics + .push(Diagnostic::new(DjangoAllWithModelForm, element.range())); + return; } } _ => (), @@ -96,5 +92,4 @@ pub(crate) fn all_with_model_form( } } } - None } diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/exclude_with_model_form.rs b/crates/ruff_linter/src/rules/flake8_django/rules/exclude_with_model_form.rs index 41661892bb9d6c..d1211c566210a6 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/exclude_with_model_form.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/exclude_with_model_form.rs @@ -1,7 +1,6 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -46,21 +45,12 @@ impl Violation for DjangoExcludeWithModelForm { } /// DJ006 -pub(crate) fn exclude_with_model_form( - checker: &Checker, - arguments: Option<&Arguments>, - body: &[Stmt], -) -> Option { - if !arguments.is_some_and(|arguments| { - arguments - .args - .iter() - .any(|base| is_model_form(base, checker.semantic())) - }) { - return None; +pub(crate) fn exclude_with_model_form(checker: &mut Checker, class_def: &ast::StmtClassDef) { + if !is_model_form(class_def, checker.semantic()) { + return; } - for element in body { + for element in &class_def.body { let Stmt::ClassDef(ast::StmtClassDef { name, body, .. }) = element else { continue; }; @@ -76,10 +66,12 @@ pub(crate) fn exclude_with_model_form( continue; }; if id == "exclude" { - return Some(Diagnostic::new(DjangoExcludeWithModelForm, target.range())); + checker + .diagnostics + .push(Diagnostic::new(DjangoExcludeWithModelForm, target.range())); + return; } } } } - None } diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs b/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs index c857bec150c117..0318de183970ae 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs @@ -1,17 +1,17 @@ -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr}; -use ruff_python_semantic::SemanticModel; +use ruff_python_semantic::{analyze, SemanticModel}; /// Return `true` if a Python class appears to be a Django model, based on its base classes. -pub(super) fn is_model(base: &Expr, semantic: &SemanticModel) -> bool { - semantic.resolve_call_path(base).is_some_and(|call_path| { +pub(super) fn is_model(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + analyze::class::any_over_body(class_def, semantic, &|call_path| { matches!(call_path.as_slice(), ["django", "db", "models", "Model"]) }) } /// Return `true` if a Python class appears to be a Django model form, based on its base classes. -pub(super) fn is_model_form(base: &Expr, semantic: &SemanticModel) -> bool { - semantic.resolve_call_path(base).is_some_and(|call_path| { +pub(super) fn is_model_form(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + analyze::class::any_over_body(class_def, semantic, &|call_path| { matches!( call_path.as_slice(), ["django", "forms", "ModelForm"] | ["django", "forms", "models", "ModelForm"] diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/model_without_dunder_str.rs b/crates/ruff_linter/src/rules/flake8_django/rules/model_without_dunder_str.rs index 9228d04753e4e9..0baaeafb347f61 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/model_without_dunder_str.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/model_without_dunder_str.rs @@ -1,10 +1,9 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::is_const_true; +use ruff_python_ast::identifier::Identifier; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_semantic::SemanticModel; -use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -52,57 +51,39 @@ impl Violation for DjangoModelWithoutDunderStr { } /// DJ008 -pub(crate) fn model_without_dunder_str( - checker: &mut Checker, - ast::StmtClassDef { - name, - arguments, - body, - .. - }: &ast::StmtClassDef, -) { - if !is_non_abstract_model(arguments.as_deref(), body, checker.semantic()) { +pub(crate) fn model_without_dunder_str(checker: &mut Checker, class_def: &ast::StmtClassDef) { + if !is_non_abstract_model(class_def, checker.semantic()) { return; } - if has_dunder_method(body) { + if has_dunder_method(class_def) { return; } - checker - .diagnostics - .push(Diagnostic::new(DjangoModelWithoutDunderStr, name.range())); + checker.diagnostics.push(Diagnostic::new( + DjangoModelWithoutDunderStr, + class_def.identifier(), + )); } -fn has_dunder_method(body: &[Stmt]) -> bool { - body.iter().any(|val| match val { - Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => { - if name == "__str__" { - return true; - } - false - } +/// Returns `true` if the class has `__str__` method. +fn has_dunder_method(class_def: &ast::StmtClassDef) -> bool { + class_def.body.iter().any(|val| match val { + Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => name == "__str__", _ => false, }) } -fn is_non_abstract_model( - arguments: Option<&Arguments>, - body: &[Stmt], - semantic: &SemanticModel, -) -> bool { - let Some(Arguments { args: bases, .. }) = arguments else { - return false; - }; - - if is_model_abstract(body) { - return false; +/// Returns `true` if the class is a non-abstract Django model. +fn is_non_abstract_model(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + if class_def.bases().is_empty() || is_model_abstract(class_def) { + false + } else { + helpers::is_model(class_def, semantic) } - - bases.iter().any(|base| helpers::is_model(base, semantic)) } /// Check if class is abstract, in terms of Django model inheritance. -fn is_model_abstract(body: &[Stmt]) -> bool { - for element in body { +fn is_model_abstract(class_def: &ast::StmtClassDef) -> bool { + for element in &class_def.body { let Stmt::ClassDef(ast::StmtClassDef { name, body, .. }) = element else { continue; }; diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/unordered_body_content_in_model.rs b/crates/ruff_linter/src/rules/flake8_django/rules/unordered_body_content_in_model.rs index 01a63d4e342f9a..635527dcaf0147 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/unordered_body_content_in_model.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/unordered_body_content_in_model.rs @@ -1,9 +1,8 @@ use std::fmt; -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; @@ -79,6 +78,50 @@ impl Violation for DjangoUnorderedBodyContentInModel { } } +/// DJ012 +pub(crate) fn unordered_body_content_in_model( + checker: &mut Checker, + class_def: &ast::StmtClassDef, +) { + if !helpers::is_model(class_def, checker.semantic()) { + return; + } + + // Track all the element types we've seen so far. + let mut element_types = Vec::new(); + let mut prev_element_type = None; + for element in &class_def.body { + let Some(element_type) = get_element_type(element, checker.semantic()) else { + continue; + }; + + // Skip consecutive elements of the same type. It's less noisy to only report + // violations at type boundaries (e.g., avoid raising a violation for _every_ + // field declaration that's out of order). + if prev_element_type == Some(element_type) { + continue; + } + + prev_element_type = Some(element_type); + + if let Some(&prev_element_type) = element_types + .iter() + .find(|&&prev_element_type| prev_element_type > element_type) + { + let diagnostic = Diagnostic::new( + DjangoUnorderedBodyContentInModel { + element_type, + prev_element_type, + }, + element.range(), + ); + checker.diagnostics.push(diagnostic); + } else { + element_types.push(element_type); + } + } +} + #[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq)] enum ContentType { FieldDeclaration, @@ -140,53 +183,3 @@ fn get_element_type(element: &Stmt, semantic: &SemanticModel) -> Option None, } } - -/// DJ012 -pub(crate) fn unordered_body_content_in_model( - checker: &mut Checker, - arguments: Option<&Arguments>, - body: &[Stmt], -) { - if !arguments.is_some_and(|arguments| { - arguments - .args - .iter() - .any(|base| helpers::is_model(base, checker.semantic())) - }) { - return; - } - - // Track all the element types we've seen so far. - let mut element_types = Vec::new(); - let mut prev_element_type = None; - for element in body { - let Some(element_type) = get_element_type(element, checker.semantic()) else { - continue; - }; - - // Skip consecutive elements of the same type. It's less noisy to only report - // violations at type boundaries (e.g., avoid raising a violation for _every_ - // field declaration that's out of order). - if prev_element_type == Some(element_type) { - continue; - } - - prev_element_type = Some(element_type); - - if let Some(&prev_element_type) = element_types - .iter() - .find(|&&prev_element_type| prev_element_type > element_type) - { - let diagnostic = Diagnostic::new( - DjangoUnorderedBodyContentInModel { - element_type, - prev_element_type, - }, - element.range(), - ); - checker.diagnostics.push(diagnostic); - } else { - element_types.push(element_type); - } - } -} diff --git a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ012_DJ012.py.snap b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ012_DJ012.py.snap index 5f15655232937c..79e5d7e39539ed 100644 --- a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ012_DJ012.py.snap +++ b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ012_DJ012.py.snap @@ -54,4 +54,12 @@ DJ012.py:129:5: DJ012 Order of model's inner classes, methods, and fields does n | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ DJ012 | +DJ012.py:146:5: DJ012 Order of model's inner classes, methods, and fields does not follow the Django Style Guide: field declaration should come before `Meta` class + | +144 | return "foobar" +145 | +146 | first_name = models.CharField(max_length=32) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ DJ012 + | + diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index 2eaab9dd27788b..1f548896057b98 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -1,13 +1,12 @@ use anyhow::Result; -use rustc_hash::FxHashSet; use ruff_diagnostics::Edit; use ruff_python_ast::call_path::from_qualified_name; -use ruff_python_ast::helpers::{map_callable, map_subscript}; +use ruff_python_ast::helpers::map_callable; use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::{Generator, Stylist}; use ruff_python_semantic::{ - Binding, BindingId, BindingKind, NodeId, ResolvedReference, SemanticModel, + analyze, Binding, BindingKind, NodeId, ResolvedReference, SemanticModel, }; use ruff_source_file::Locator; use ruff_text_size::Ranged; @@ -59,57 +58,17 @@ pub(crate) fn runtime_required_class( false } +/// Return `true` if a class is a subclass of a runtime-required base class. fn runtime_required_base_class( class_def: &ast::StmtClassDef, base_classes: &[String], semantic: &SemanticModel, ) -> bool { - fn inner( - class_def: &ast::StmtClassDef, - base_classes: &[String], - semantic: &SemanticModel, - seen: &mut FxHashSet, - ) -> bool { - class_def.bases().iter().any(|expr| { - // If the base class is itself runtime-required, then this is too. - // Ex) `class Foo(BaseModel): ...` - if semantic - .resolve_call_path(map_subscript(expr)) - .is_some_and(|call_path| { - base_classes - .iter() - .any(|base_class| from_qualified_name(base_class) == call_path) - }) - { - return true; - } - - // If the base class extends a runtime-required class, then this does too. - // Ex) `class Bar(BaseModel): ...; class Foo(Bar): ...` - if let Some(id) = semantic.lookup_attribute(map_subscript(expr)) { - if seen.insert(id) { - let binding = semantic.binding(id); - if let Some(base_class) = binding - .kind - .as_class_definition() - .map(|id| &semantic.scopes[*id]) - .and_then(|scope| scope.kind.as_class()) - { - if inner(base_class, base_classes, semantic, seen) { - return true; - } - } - } - } - false - }) - } - - if base_classes.is_empty() { - return false; - } - - inner(class_def, base_classes, semantic, &mut FxHashSet::default()) + analyze::class::any_over_body(class_def, semantic, &|call_path| { + base_classes + .iter() + .any(|base_class| from_qualified_name(base_class) == call_path) + }) } fn runtime_required_decorators( diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs new file mode 100644 index 00000000000000..3efc01ff33754a --- /dev/null +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -0,0 +1,57 @@ +use rustc_hash::FxHashSet; + +use ruff_python_ast as ast; +use ruff_python_ast::call_path::CallPath; +use ruff_python_ast::helpers::map_subscript; + +use crate::{BindingId, SemanticModel}; + +/// Return `true` if any base class of a class definition matches a predicate. +pub fn any_over_body( + class_def: &ast::StmtClassDef, + semantic: &SemanticModel, + func: &dyn Fn(CallPath) -> bool, +) -> bool { + fn inner( + class_def: &ast::StmtClassDef, + semantic: &SemanticModel, + func: &dyn Fn(CallPath) -> bool, + seen: &mut FxHashSet, + ) -> bool { + class_def.bases().iter().any(|expr| { + // If the base class itself matches the pattern, then this does too. + // Ex) `class Foo(BaseModel): ...` + if semantic + .resolve_call_path(map_subscript(expr)) + .is_some_and(func) + { + return true; + } + + // If the base class extends a class that matches the pattern, then this does too. + // Ex) `class Bar(BaseModel): ...; class Foo(Bar): ...` + if let Some(id) = semantic.lookup_attribute(map_subscript(expr)) { + if seen.insert(id) { + let binding = semantic.binding(id); + if let Some(base_class) = binding + .kind + .as_class_definition() + .map(|id| &semantic.scopes[*id]) + .and_then(|scope| scope.kind.as_class()) + { + if inner(base_class, semantic, func, seen) { + return true; + } + } + } + } + false + }) + } + + if class_def.bases().is_empty() { + return false; + } + + inner(class_def, semantic, func, &mut FxHashSet::default()) +} diff --git a/crates/ruff_python_semantic/src/analyze/mod.rs b/crates/ruff_python_semantic/src/analyze/mod.rs index f0fb3844e30c6d..0376f63c39f432 100644 --- a/crates/ruff_python_semantic/src/analyze/mod.rs +++ b/crates/ruff_python_semantic/src/analyze/mod.rs @@ -1,3 +1,4 @@ +pub mod class; pub mod function_type; pub mod imports; pub mod logging;