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

Add base-class inheritance detection to flake8-django rules #9151

Merged
merged 1 commit into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading