Skip to content

Commit

Permalink
Add base-class inheritance detection to flake8-django rules
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Dec 15, 2023
1 parent 82731b8 commit 1ccfc51
Show file tree
Hide file tree
Showing 11 changed files with 189 additions and 199 deletions.
18 changes: 18 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_django/DJ012.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

20 changes: 3 additions & 17 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -48,21 +47,12 @@ impl Violation for DjangoAllWithModelForm {
}

/// DJ007
pub(crate) fn all_with_model_form(
checker: &Checker,
arguments: Option<&Arguments>,
body: &[Stmt],
) -> Option<Diagnostic> {
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;
};
Expand All @@ -83,18 +73,23 @@ 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;
}
}
_ => (),
};
}
}
}
None
}
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -46,21 +45,12 @@ impl Violation for DjangoExcludeWithModelForm {
}

/// DJ006
pub(crate) fn exclude_with_model_form(
checker: &Checker,
arguments: Option<&Arguments>,
body: &[Stmt],
) -> Option<Diagnostic> {
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;
};
Expand All @@ -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
}
12 changes: 6 additions & 6 deletions crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs
Original file line number Diff line number Diff line change
@@ -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"]
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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;
};
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -140,53 +183,3 @@ fn get_element_type(element: &Stmt, semantic: &SemanticModel) -> Option<ContentT
_ => 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);
}
}
}
Loading

0 comments on commit 1ccfc51

Please sign in to comment.