diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ008.py b/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ008.py index 27473e9b75e40..ccf4bc36cd0c4 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ008.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_django/DJ008.py @@ -165,3 +165,19 @@ def my_brand_new_property(self): def my_beautiful_method(self): return 2 + + +# Subclass with its own __str__ +class SubclassTestModel1(TestModel1): + def __str__(self): + return self.new_field + + +# Subclass with inherited __str__ +class SubclassTestModel2(TestModel4): + pass + + +# Subclass without __str__ +class SubclassTestModel3(TestModel1): + pass 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 0318de183970a..073371c1bc420 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/helpers.rs @@ -4,14 +4,14 @@ 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(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { - analyze::class::any_over_body(class_def, semantic, &|call_path| { + analyze::class::any_call_path(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(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { - analyze::class::any_over_body(class_def, semantic, &|call_path| { + analyze::class::any_call_path(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 0baaeafb347f6..68e717f3c9901 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 @@ -3,7 +3,7 @@ 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_python_semantic::{analyze, SemanticModel}; use crate::checkers::ast::Checker; @@ -55,9 +55,11 @@ pub(crate) fn model_without_dunder_str(checker: &mut Checker, class_def: &ast::S if !is_non_abstract_model(class_def, checker.semantic()) { return; } - if has_dunder_method(class_def) { + + if has_dunder_method(class_def, checker.semantic()) { return; } + checker.diagnostics.push(Diagnostic::new( DjangoModelWithoutDunderStr, class_def.identifier(), @@ -65,10 +67,12 @@ pub(crate) fn model_without_dunder_str(checker: &mut Checker, class_def: &ast::S } /// 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 has_dunder_method(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + analyze::class::any_super_class(class_def, semantic, &|class_def| { + class_def.body.iter().any(|val| match val { + Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => name == "__str__", + _ => false, + }) }) } diff --git a/crates/ruff_linter/src/rules/flake8_django/rules/nullable_model_string_field.rs b/crates/ruff_linter/src/rules/flake8_django/rules/nullable_model_string_field.rs index 42034271e8f91..c1fcaac144593 100644 --- a/crates/ruff_linter/src/rules/flake8_django/rules/nullable_model_string_field.rs +++ b/crates/ruff_linter/src/rules/flake8_django/rules/nullable_model_string_field.rs @@ -1,8 +1,9 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use ruff_python_ast::{self as ast, 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_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -48,7 +49,7 @@ impl Violation for DjangoNullableModelStringField { #[derive_message_formats] fn message(&self) -> String { let DjangoNullableModelStringField { field_name } = self; - format!("Avoid using `null=True` on string-based fields such as {field_name}") + format!("Avoid using `null=True` on string-based fields such as `{field_name}`") } } @@ -58,7 +59,7 @@ pub(crate) fn nullable_model_string_field(checker: &mut Checker, body: &[Stmt]) let Stmt::Assign(ast::StmtAssign { value, .. }) = statement else { continue; }; - if let Some(field_name) = is_nullable_field(checker, value) { + if let Some(field_name) = is_nullable_field(value, checker.semantic()) { checker.diagnostics.push(Diagnostic::new( DjangoNullableModelStringField { field_name: field_name.to_string(), @@ -69,22 +70,12 @@ pub(crate) fn nullable_model_string_field(checker: &mut Checker, body: &[Stmt]) } } -fn is_nullable_field<'a>(checker: &'a Checker, value: &'a Expr) -> Option<&'a str> { - let Expr::Call(ast::ExprCall { - func, - arguments: Arguments { keywords, .. }, - .. - }) = value - else { - return None; - }; - - let Some(valid_field_name) = helpers::get_model_field_name(func, checker.semantic()) else { - return None; - }; +fn is_nullable_field<'a>(value: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a str> { + let call = value.as_call_expr()?; + let field_name = helpers::get_model_field_name(&call.func, semantic)?; if !matches!( - valid_field_name, + field_name, "CharField" | "TextField" | "SlugField" | "EmailField" | "FilePathField" | "URLField" ) { return None; @@ -93,7 +84,7 @@ fn is_nullable_field<'a>(checker: &'a Checker, value: &'a Expr) -> Option<&'a st let mut null_key = false; let mut blank_key = false; let mut unique_key = false; - for keyword in keywords { + for keyword in &call.arguments.keywords { let Some(argument) = &keyword.arg else { continue; }; @@ -113,5 +104,6 @@ fn is_nullable_field<'a>(checker: &'a Checker, value: &'a Expr) -> Option<&'a st if !null_key { return None; } - Some(valid_field_name) + + Some(field_name) } diff --git a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ001_DJ001.py.snap b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ001_DJ001.py.snap index d3c4c698a045d..54812a0f1d7f3 100644 --- a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ001_DJ001.py.snap +++ b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ001_DJ001.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_django/mod.rs --- -DJ001.py:7:17: DJ001 Avoid using `null=True` on string-based fields such as CharField +DJ001.py:7:17: DJ001 Avoid using `null=True` on string-based fields such as `CharField` | 6 | class IncorrectModel(models.Model): 7 | charfield = models.CharField(max_length=255, null=True) @@ -10,7 +10,7 @@ DJ001.py:7:17: DJ001 Avoid using `null=True` on string-based fields such as Char 9 | slugfield = models.SlugField(max_length=255, null=True) | -DJ001.py:8:17: DJ001 Avoid using `null=True` on string-based fields such as TextField +DJ001.py:8:17: DJ001 Avoid using `null=True` on string-based fields such as `TextField` | 6 | class IncorrectModel(models.Model): 7 | charfield = models.CharField(max_length=255, null=True) @@ -20,7 +20,7 @@ DJ001.py:8:17: DJ001 Avoid using `null=True` on string-based fields such as Text 10 | emailfield = models.EmailField(max_length=255, null=True) | -DJ001.py:9:17: DJ001 Avoid using `null=True` on string-based fields such as SlugField +DJ001.py:9:17: DJ001 Avoid using `null=True` on string-based fields such as `SlugField` | 7 | charfield = models.CharField(max_length=255, null=True) 8 | textfield = models.TextField(max_length=255, null=True) @@ -30,7 +30,7 @@ DJ001.py:9:17: DJ001 Avoid using `null=True` on string-based fields such as Slug 11 | filepathfield = models.FilePathField(max_length=255, null=True) | -DJ001.py:10:18: DJ001 Avoid using `null=True` on string-based fields such as EmailField +DJ001.py:10:18: DJ001 Avoid using `null=True` on string-based fields such as `EmailField` | 8 | textfield = models.TextField(max_length=255, null=True) 9 | slugfield = models.SlugField(max_length=255, null=True) @@ -40,7 +40,7 @@ DJ001.py:10:18: DJ001 Avoid using `null=True` on string-based fields such as Ema 12 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:11:21: DJ001 Avoid using `null=True` on string-based fields such as FilePathField +DJ001.py:11:21: DJ001 Avoid using `null=True` on string-based fields such as `FilePathField` | 9 | slugfield = models.SlugField(max_length=255, null=True) 10 | emailfield = models.EmailField(max_length=255, null=True) @@ -49,7 +49,7 @@ DJ001.py:11:21: DJ001 Avoid using `null=True` on string-based fields such as Fil 12 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:12:16: DJ001 Avoid using `null=True` on string-based fields such as URLField +DJ001.py:12:16: DJ001 Avoid using `null=True` on string-based fields such as `URLField` | 10 | emailfield = models.EmailField(max_length=255, null=True) 11 | filepathfield = models.FilePathField(max_length=255, null=True) @@ -57,7 +57,7 @@ DJ001.py:12:16: DJ001 Avoid using `null=True` on string-based fields such as URL | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ DJ001 | -DJ001.py:16:17: DJ001 Avoid using `null=True` on string-based fields such as CharField +DJ001.py:16:17: DJ001 Avoid using `null=True` on string-based fields such as `CharField` | 15 | class IncorrectModelWithAlias(DjangoModel): 16 | charfield = DjangoModel.CharField(max_length=255, null=True) @@ -66,7 +66,7 @@ DJ001.py:16:17: DJ001 Avoid using `null=True` on string-based fields such as Cha 18 | slugfield = models.SlugField(max_length=255, null=True) | -DJ001.py:17:17: DJ001 Avoid using `null=True` on string-based fields such as CharField +DJ001.py:17:17: DJ001 Avoid using `null=True` on string-based fields such as `CharField` | 15 | class IncorrectModelWithAlias(DjangoModel): 16 | charfield = DjangoModel.CharField(max_length=255, null=True) @@ -76,7 +76,7 @@ DJ001.py:17:17: DJ001 Avoid using `null=True` on string-based fields such as Cha 19 | emailfield = models.EmailField(max_length=255, null=True) | -DJ001.py:18:17: DJ001 Avoid using `null=True` on string-based fields such as SlugField +DJ001.py:18:17: DJ001 Avoid using `null=True` on string-based fields such as `SlugField` | 16 | charfield = DjangoModel.CharField(max_length=255, null=True) 17 | textfield = SmthCharField(max_length=255, null=True) @@ -86,7 +86,7 @@ DJ001.py:18:17: DJ001 Avoid using `null=True` on string-based fields such as Slu 20 | filepathfield = models.FilePathField(max_length=255, null=True) | -DJ001.py:19:18: DJ001 Avoid using `null=True` on string-based fields such as EmailField +DJ001.py:19:18: DJ001 Avoid using `null=True` on string-based fields such as `EmailField` | 17 | textfield = SmthCharField(max_length=255, null=True) 18 | slugfield = models.SlugField(max_length=255, null=True) @@ -96,7 +96,7 @@ DJ001.py:19:18: DJ001 Avoid using `null=True` on string-based fields such as Ema 21 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:20:21: DJ001 Avoid using `null=True` on string-based fields such as FilePathField +DJ001.py:20:21: DJ001 Avoid using `null=True` on string-based fields such as `FilePathField` | 18 | slugfield = models.SlugField(max_length=255, null=True) 19 | emailfield = models.EmailField(max_length=255, null=True) @@ -105,7 +105,7 @@ DJ001.py:20:21: DJ001 Avoid using `null=True` on string-based fields such as Fil 21 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:21:16: DJ001 Avoid using `null=True` on string-based fields such as URLField +DJ001.py:21:16: DJ001 Avoid using `null=True` on string-based fields such as `URLField` | 19 | emailfield = models.EmailField(max_length=255, null=True) 20 | filepathfield = models.FilePathField(max_length=255, null=True) @@ -113,7 +113,7 @@ DJ001.py:21:16: DJ001 Avoid using `null=True` on string-based fields such as URL | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ DJ001 | -DJ001.py:25:17: DJ001 Avoid using `null=True` on string-based fields such as CharField +DJ001.py:25:17: DJ001 Avoid using `null=True` on string-based fields such as `CharField` | 24 | class IncorrectModelWithoutSuperclass: 25 | charfield = DjangoModel.CharField(max_length=255, null=True) @@ -122,7 +122,7 @@ DJ001.py:25:17: DJ001 Avoid using `null=True` on string-based fields such as Cha 27 | slugfield = models.SlugField(max_length=255, null=True) | -DJ001.py:26:17: DJ001 Avoid using `null=True` on string-based fields such as CharField +DJ001.py:26:17: DJ001 Avoid using `null=True` on string-based fields such as `CharField` | 24 | class IncorrectModelWithoutSuperclass: 25 | charfield = DjangoModel.CharField(max_length=255, null=True) @@ -132,7 +132,7 @@ DJ001.py:26:17: DJ001 Avoid using `null=True` on string-based fields such as Cha 28 | emailfield = models.EmailField(max_length=255, null=True) | -DJ001.py:27:17: DJ001 Avoid using `null=True` on string-based fields such as SlugField +DJ001.py:27:17: DJ001 Avoid using `null=True` on string-based fields such as `SlugField` | 25 | charfield = DjangoModel.CharField(max_length=255, null=True) 26 | textfield = SmthCharField(max_length=255, null=True) @@ -142,7 +142,7 @@ DJ001.py:27:17: DJ001 Avoid using `null=True` on string-based fields such as Slu 29 | filepathfield = models.FilePathField(max_length=255, null=True) | -DJ001.py:28:18: DJ001 Avoid using `null=True` on string-based fields such as EmailField +DJ001.py:28:18: DJ001 Avoid using `null=True` on string-based fields such as `EmailField` | 26 | textfield = SmthCharField(max_length=255, null=True) 27 | slugfield = models.SlugField(max_length=255, null=True) @@ -152,7 +152,7 @@ DJ001.py:28:18: DJ001 Avoid using `null=True` on string-based fields such as Ema 30 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:29:21: DJ001 Avoid using `null=True` on string-based fields such as FilePathField +DJ001.py:29:21: DJ001 Avoid using `null=True` on string-based fields such as `FilePathField` | 27 | slugfield = models.SlugField(max_length=255, null=True) 28 | emailfield = models.EmailField(max_length=255, null=True) @@ -161,7 +161,7 @@ DJ001.py:29:21: DJ001 Avoid using `null=True` on string-based fields such as Fil 30 | urlfield = models.URLField(max_length=255, null=True) | -DJ001.py:30:16: DJ001 Avoid using `null=True` on string-based fields such as URLField +DJ001.py:30:16: DJ001 Avoid using `null=True` on string-based fields such as `URLField` | 28 | emailfield = models.EmailField(max_length=255, null=True) 29 | filepathfield = models.FilePathField(max_length=255, null=True) diff --git a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ008_DJ008.py.snap b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ008_DJ008.py.snap index 159ee3a9d2d32..a913353ad76b0 100644 --- a/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ008_DJ008.py.snap +++ b/crates/ruff_linter/src/rules/flake8_django/snapshots/ruff_linter__rules__flake8_django__tests__DJ008_DJ008.py.snap @@ -23,4 +23,12 @@ DJ008.py:36:7: DJ008 Model does not define `__str__` method 37 | new_field = models.CharField(max_length=10) | +DJ008.py:182:7: DJ008 Model does not define `__str__` method + | +181 | # Subclass without __str__ +182 | class SubclassTestModel3(TestModel1): + | ^^^^^^^^^^^^^^^^^^ DJ008 +183 | pass + | + 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 5cee3fcd5807a..a7abd8571e468 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -77,7 +77,7 @@ fn runtime_required_base_class( base_classes: &[String], semantic: &SemanticModel, ) -> bool { - analyze::class::any_over_body(class_def, semantic, &|call_path| { + analyze::class::any_call_path(class_def, semantic, &|call_path| { base_classes .iter() .any(|base_class| from_qualified_name(base_class) == call_path) diff --git a/crates/ruff_linter/src/rules/ruff/rules/helpers.rs b/crates/ruff_linter/src/rules/ruff/rules/helpers.rs index e7b4bfabc54a3..1e2f2c828e2e8 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/helpers.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/helpers.rs @@ -56,7 +56,7 @@ pub(super) fn has_default_copy_semantics( class_def: &ast::StmtClassDef, semantic: &SemanticModel, ) -> bool { - analyze::class::any_over_body(class_def, semantic, &|call_path| { + analyze::class::any_call_path(class_def, semantic, &|call_path| { matches!( call_path.as_slice(), ["pydantic", "BaseModel" | "BaseSettings"] diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs index 3efc01ff33754..bbc3a1e2bd3c6 100644 --- a/crates/ruff_python_semantic/src/analyze/class.rs +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -6,8 +6,8 @@ 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( +/// Return `true` if any base class matches a [`CallPath`] predicate. +pub fn any_call_path( class_def: &ast::StmtClassDef, semantic: &SemanticModel, func: &dyn Fn(CallPath) -> bool, @@ -55,3 +55,45 @@ pub fn any_over_body( inner(class_def, semantic, func, &mut FxHashSet::default()) } + +/// Return `true` if any base class matches an [`ast::StmtClassDef`] predicate. +pub fn any_super_class( + class_def: &ast::StmtClassDef, + semantic: &SemanticModel, + func: &dyn Fn(&ast::StmtClassDef) -> bool, +) -> bool { + fn inner( + class_def: &ast::StmtClassDef, + semantic: &SemanticModel, + func: &dyn Fn(&ast::StmtClassDef) -> bool, + seen: &mut FxHashSet, + ) -> bool { + // If the function itself matches the pattern, then this does too. + if func(class_def) { + return true; + } + + // Otherwise, check every base class. + class_def.bases().iter().any(|expr| { + // If the base class extends a class that matches the pattern, then this does too. + 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 + }) + } + + inner(class_def, semantic, func, &mut FxHashSet::default()) +}