From bc79f540e491075863a5a1c49a61317da6e34363 Mon Sep 17 00:00:00 2001 From: Luc Khai Hai Date: Mon, 27 Feb 2023 07:29:42 +0900 Subject: [PATCH] [flake8-django] DJ003, DJ006, DJ007 (#3236) Implements [flake8-django](https://github.com/rocioar/flake8-django) rules: - DJ03 - DJ06 - DJ07 --- .../test/fixtures/flake8_django/DJ003.py | 21 ++++ .../test/fixtures/flake8_django/DJ006.py | 11 +++ .../test/fixtures/flake8_django/DJ007.py | 16 ++++ crates/ruff/src/checkers/ast.rs | 20 ++++ crates/ruff/src/codes.rs | 3 + crates/ruff/src/registry.rs | 3 + crates/ruff/src/rules/flake8_django/mod.rs | 3 + .../rules/all_with_model_form.rs | 96 +++++++++++++++++++ .../rules/exclude_with_model_form.rs | 79 +++++++++++++++ .../src/rules/flake8_django/rules/helpers.rs | 11 ++- .../rules/locals_in_render_function.rs | 88 +++++++++++++++++ .../ruff/src/rules/flake8_django/rules/mod.rs | 6 ++ ..._flake8_django__tests__DJ003_DJ003.py.snap | 24 +++++ ..._flake8_django__tests__DJ006_DJ006.py.snap | 14 +++ ..._flake8_django__tests__DJ007_DJ007.py.snap | 24 +++++ .../rules/missing_whitespace_after_keyword.rs | 11 +-- ruff.schema.json | 3 + 17 files changed, 426 insertions(+), 7 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_django/DJ003.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_django/DJ006.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_django/DJ007.py create mode 100644 crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs create mode 100644 crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs create mode 100644 crates/ruff/src/rules/flake8_django/rules/locals_in_render_function.rs create mode 100644 crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ003_DJ003.py.snap create mode 100644 crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ006_DJ006.py.snap create mode 100644 crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ007_DJ007.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_django/DJ003.py b/crates/ruff/resources/test/fixtures/flake8_django/DJ003.py new file mode 100644 index 0000000000000..7a7dd0b1741aa --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_django/DJ003.py @@ -0,0 +1,21 @@ +from django.shortcuts import render + + +def test_view1(request): + return render(request, "index.html", locals()) + + +def test_view2(request): + return render(request, "index.html", context=locals()) + + +def test_view3(request): + return render(request, "index.html") + + +def test_view4(request): + return render(request, "index.html", {}) + + +def test_view5(request): + return render(request, "index.html", context={}) diff --git a/crates/ruff/resources/test/fixtures/flake8_django/DJ006.py b/crates/ruff/resources/test/fixtures/flake8_django/DJ006.py new file mode 100644 index 0000000000000..943ec02deee85 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_django/DJ006.py @@ -0,0 +1,11 @@ +from django import forms + + +class TestModelForm1(forms.ModelForm): + class Meta: + exclude = ["bar"] + + +class TestModelForm2(forms.ModelForm): + class Meta: + fields = ["foo"] diff --git a/crates/ruff/resources/test/fixtures/flake8_django/DJ007.py b/crates/ruff/resources/test/fixtures/flake8_django/DJ007.py new file mode 100644 index 0000000000000..642b13272e3ce --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_django/DJ007.py @@ -0,0 +1,16 @@ +from django import forms + + +class TestModelForm1(forms.ModelForm): + class Meta: + fields = "__all__" + + +class TestModelForm2(forms.ModelForm): + class Meta: + fields = b"__all__" + + +class TestModelForm3(forms.ModelForm): + class Meta: + fields = ["foo"] diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 30bda4f089456..8c90bcf942f29 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -812,6 +812,21 @@ where self, body, )); } + + if self.settings.rules.enabled(&Rule::ExcludeWithModelForm) { + if let Some(diagnostic) = + flake8_django::rules::exclude_with_model_form(self, bases, body) + { + self.diagnostics.push(diagnostic); + } + } + if self.settings.rules.enabled(&Rule::AllWithModelForm) { + if let Some(diagnostic) = + flake8_django::rules::all_with_model_form(self, bases, body) + { + self.diagnostics.push(diagnostic); + } + } if self.settings.rules.enabled(&Rule::ModelWithoutDunderStr) { if let Some(diagnostic) = flake8_django::rules::model_without_dunder_str(self, bases, body, stmt) @@ -2941,6 +2956,11 @@ where { pylint::rules::logging_call(self, func, args, keywords); } + + // flake8-django + if self.settings.rules.enabled(&Rule::LocalsInRenderFunction) { + flake8_django::rules::locals_in_render_function(self, func, args, keywords); + } } ExprKind::Dict { keys, values } => { if self diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 8df9ac8c517cb..4c13815afabaf 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -621,6 +621,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { // flake8-django (Flake8Django, "001") => Rule::NullableModelStringField, + (Flake8Django, "003") => Rule::LocalsInRenderFunction, + (Flake8Django, "006") => Rule::ExcludeWithModelForm, + (Flake8Django, "007") => Rule::AllWithModelForm, (Flake8Django, "008") => Rule::ModelWithoutDunderStr, (Flake8Django, "013") => Rule::NonLeadingReceiverDecorator, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 0b80018588ee2..45f98dbd340da 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -581,6 +581,9 @@ ruff_macros::register_rules!( rules::ruff::rules::UnusedNOQA, // flake8-django rules::flake8_django::rules::NullableModelStringField, + rules::flake8_django::rules::LocalsInRenderFunction, + rules::flake8_django::rules::ExcludeWithModelForm, + rules::flake8_django::rules::AllWithModelForm, rules::flake8_django::rules::ModelWithoutDunderStr, rules::flake8_django::rules::NonLeadingReceiverDecorator, ); diff --git a/crates/ruff/src/rules/flake8_django/mod.rs b/crates/ruff/src/rules/flake8_django/mod.rs index b9840e665217a..99ad2807fc3e2 100644 --- a/crates/ruff/src/rules/flake8_django/mod.rs +++ b/crates/ruff/src/rules/flake8_django/mod.rs @@ -14,6 +14,9 @@ mod tests { use crate::test::test_path; #[test_case(Rule::NullableModelStringField, Path::new("DJ001.py"); "DJ001")] + #[test_case(Rule::LocalsInRenderFunction, Path::new("DJ003.py"); "DJ003")] + #[test_case(Rule::ExcludeWithModelForm, Path::new("DJ006.py"); "DJ006")] + #[test_case(Rule::AllWithModelForm, Path::new("DJ007.py"); "DJ007")] #[test_case(Rule::ModelWithoutDunderStr, Path::new("DJ008.py"); "DJ008")] #[test_case(Rule::NonLeadingReceiverDecorator, Path::new("DJ013.py"); "DJ013")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { diff --git a/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs b/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs new file mode 100644 index 0000000000000..185dcafa704e5 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs @@ -0,0 +1,96 @@ +use rustpython_parser::ast::{Constant, Expr, ExprKind, Stmt, StmtKind}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::rules::flake8_django::rules::helpers::is_model_form; +use crate::violation::Violation; +use crate::{checkers::ast::Checker, registry::Diagnostic, Range}; + +define_violation!( + /// ## What it does + /// Checks for the use of `fields = "__all__"` in Django `ModelForm` + /// classes. + /// + /// ## Why is this bad? + /// If a `ModelForm` includes the `fields = "__all__"` attribute, any new + /// field that is added to the model will automatically be exposed for + /// modification. + /// + /// ## Example + /// ```python + /// from django.forms import ModelForm + /// + /// class PostForm(ModelForm): + /// class Meta: + /// model = Post + /// fields = "__all__" + /// ``` + /// + /// Use instead: + /// ```python + /// from django.forms import ModelForm + /// + /// class PostForm(ModelForm): + /// class Meta: + /// model = Post + /// fields = ["title", "content"] + /// ``` + pub struct AllWithModelForm; +); +impl Violation for AllWithModelForm { + #[derive_message_formats] + fn message(&self) -> String { + format!("Do not use `__all__` with `ModelForm`, use `fields` instead") + } +} + +/// DJ007 +pub fn all_with_model_form(checker: &Checker, bases: &[Expr], body: &[Stmt]) -> Option { + if !bases.iter().any(|base| is_model_form(checker, base)) { + return None; + } + for element in body.iter() { + let StmtKind::ClassDef { name, body, .. } = &element.node else { + continue; + }; + if name != "Meta" { + continue; + } + for element in body.iter() { + let StmtKind::Assign { targets, value, .. } = &element.node else { + continue; + }; + for target in targets.iter() { + let ExprKind::Name { id, .. } = &target.node else { + continue; + }; + if id != "fields" { + continue; + } + let ExprKind::Constant { value, .. } = &value.node else { + continue; + }; + match &value { + Constant::Str(s) => { + if s == "__all__" { + return Some(Diagnostic::new( + AllWithModelForm, + Range::from_located(element), + )); + } + } + Constant::Bytes(b) => { + if b == "__all__".as_bytes() { + return Some(Diagnostic::new( + AllWithModelForm, + Range::from_located(element), + )); + } + } + _ => (), + }; + } + } + } + None +} diff --git a/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs b/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs new file mode 100644 index 0000000000000..f64f3df49b0da --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs @@ -0,0 +1,79 @@ +use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::rules::flake8_django::rules::helpers::is_model_form; +use crate::violation::Violation; +use crate::{checkers::ast::Checker, registry::Diagnostic, Range}; + +define_violation!( + /// ## What it does + /// Checks for the use of `exclude` in Django `ModelForm` classes. + /// + /// ## Why is this bad? + /// If a `ModelForm` includes the `exclude` attribute, any new field that + /// is added to the model will automatically be exposed for modification. + /// + /// ## Example + /// ```python + /// from django.forms import ModelForm + /// + /// class PostForm(ModelForm): + /// class Meta: + /// model = Post + /// exclude = ["author"] + /// ``` + /// + /// Use instead: + /// ```python + /// from django.forms import ModelForm + /// + /// class PostForm(ModelForm): + /// class Meta: + /// model = Post + /// fields = ["title", "content"] + /// ``` + pub struct ExcludeWithModelForm; +); +impl Violation for ExcludeWithModelForm { + #[derive_message_formats] + fn message(&self) -> String { + format!("Do not use `exclude` with `ModelForm`, use `fields` instead") + } +} + +/// DJ006 +pub fn exclude_with_model_form( + checker: &Checker, + bases: &[Expr], + body: &[Stmt], +) -> Option { + if !bases.iter().any(|base| is_model_form(checker, base)) { + return None; + } + for element in body.iter() { + let StmtKind::ClassDef { name, body, .. } = &element.node else { + continue; + }; + if name != "Meta" { + continue; + } + for element in body.iter() { + let StmtKind::Assign { targets, .. } = &element.node else { + continue; + }; + for target in targets.iter() { + let ExprKind::Name { id, .. } = &target.node else { + continue; + }; + if id == "exclude" { + return Some(Diagnostic::new( + ExcludeWithModelForm, + Range::from_located(target), + )); + } + } + } + } + None +} diff --git a/crates/ruff/src/rules/flake8_django/rules/helpers.rs b/crates/ruff/src/rules/flake8_django/rules/helpers.rs index d308d7ebf61be..da5f3e989bcb5 100644 --- a/crates/ruff/src/rules/flake8_django/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_django/rules/helpers.rs @@ -2,13 +2,22 @@ use rustpython_parser::ast::Expr; use crate::checkers::ast::Checker; -/// Return `true` if a Python class appears to be a Django model based on a base class. +/// Return `true` if a Python class appears to be a Django model, based on its base classes. pub fn is_model(checker: &Checker, base: &Expr) -> bool { checker.resolve_call_path(base).map_or(false, |call_path| { 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 fn is_model_form(checker: &Checker, base: &Expr) -> bool { + checker.resolve_call_path(base).map_or(false, |call_path| { + call_path.as_slice() == ["django", "forms", "ModelForm"] + || call_path.as_slice() == ["django", "forms", "models", "ModelForm"] + }) +} + +/// Return the name of the field type, if the expression is constructor for a Django model field. pub fn get_model_field_name<'a>(checker: &'a Checker, expr: &'a Expr) -> Option<&'a str> { checker.resolve_call_path(expr).and_then(|call_path| { let call_path = call_path.as_slice(); diff --git a/crates/ruff/src/rules/flake8_django/rules/locals_in_render_function.rs b/crates/ruff/src/rules/flake8_django/rules/locals_in_render_function.rs new file mode 100644 index 0000000000000..afb341c73e25c --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/rules/locals_in_render_function.rs @@ -0,0 +1,88 @@ +use rustpython_parser::ast::{Expr, ExprKind, Keyword}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::{checkers::ast::Checker, registry::Diagnostic, violation::Violation, Range}; + +define_violation!( + /// ## What it does + /// Checks for the use of `locals()` in `render` functions. + /// + /// ## Why is this bad? + /// Using `locals()` can expose internal variables or other unintentional + /// data to the rendered template. + /// + /// ## Example + /// ```python + /// from django.shortcuts import render + /// + /// def index(request): + /// posts = Post.objects.all() + /// return render(request, "app/index.html", locals()) + /// ``` + /// + /// Use instead: + /// ```python + /// from django.shortcuts import render + /// + /// def index(request): + /// posts = Post.objects.all() + /// context = {"posts": posts} + /// return render(request, "app/index.html", context) + /// ``` + pub struct LocalsInRenderFunction; +); +impl Violation for LocalsInRenderFunction { + #[derive_message_formats] + fn message(&self) -> String { + format!("Avoid passing `locals()` as context to a `render` function") + } +} + +/// DJ003 +pub fn locals_in_render_function( + checker: &mut Checker, + func: &Expr, + args: &[Expr], + keywords: &[Keyword], +) { + if !checker.resolve_call_path(func).map_or(false, |call_path| { + call_path.as_slice() == ["django", "shortcuts", "render"] + }) { + return; + } + + let locals = if args.len() >= 3 { + if !is_locals_call(checker, &args[2]) { + return; + } + &args[2] + } else if let Some(keyword) = keywords.iter().find(|keyword| { + keyword + .node + .arg + .as_ref() + .map_or(false, |arg| arg == "context") + }) { + if !is_locals_call(checker, &keyword.node.value) { + return; + } + &keyword.node.value + } else { + return; + }; + + checker.diagnostics.push(Diagnostic::new( + LocalsInRenderFunction, + Range::from_located(locals), + )); +} + +fn is_locals_call(checker: &Checker, expr: &Expr) -> bool { + let ExprKind::Call { func, .. } = &expr.node else { + return false + }; + checker + .resolve_call_path(func) + .map_or(false, |call_path| call_path.as_slice() == ["", "locals"]) +} diff --git a/crates/ruff/src/rules/flake8_django/rules/mod.rs b/crates/ruff/src/rules/flake8_django/rules/mod.rs index 239086dc1fc39..85be2d14e1720 100644 --- a/crates/ruff/src/rules/flake8_django/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_django/rules/mod.rs @@ -1,10 +1,16 @@ +pub use all_with_model_form::{all_with_model_form, AllWithModelForm}; +pub use exclude_with_model_form::{exclude_with_model_form, ExcludeWithModelForm}; +pub use locals_in_render_function::{locals_in_render_function, LocalsInRenderFunction}; pub use model_without_dunder_str::{model_without_dunder_str, ModelWithoutDunderStr}; pub use non_leading_receiver_decorator::{ non_leading_receiver_decorator, NonLeadingReceiverDecorator, }; pub use nullable_model_string_field::{nullable_model_string_field, NullableModelStringField}; +mod all_with_model_form; +mod exclude_with_model_form; mod helpers; +mod locals_in_render_function; mod model_without_dunder_str; mod non_leading_receiver_decorator; mod nullable_model_string_field; diff --git a/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ003_DJ003.py.snap b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ003_DJ003.py.snap new file mode 100644 index 0000000000000..62fea871d2ff4 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ003_DJ003.py.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff/src/rules/flake8_django/mod.rs +expression: diagnostics +--- +- kind: + LocalsInRenderFunction: ~ + location: + row: 5 + column: 41 + end_location: + row: 5 + column: 49 + fix: ~ + parent: ~ +- kind: + LocalsInRenderFunction: ~ + location: + row: 9 + column: 49 + end_location: + row: 9 + column: 57 + fix: ~ + parent: ~ diff --git a/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ006_DJ006.py.snap b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ006_DJ006.py.snap new file mode 100644 index 0000000000000..122236d2b8c55 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ006_DJ006.py.snap @@ -0,0 +1,14 @@ +--- +source: crates/ruff/src/rules/flake8_django/mod.rs +expression: diagnostics +--- +- kind: + ExcludeWithModelForm: ~ + location: + row: 6 + column: 8 + end_location: + row: 6 + column: 15 + fix: ~ + parent: ~ diff --git a/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ007_DJ007.py.snap b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ007_DJ007.py.snap new file mode 100644 index 0000000000000..5d37a7b3956f0 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ007_DJ007.py.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff/src/rules/flake8_django/mod.rs +expression: diagnostics +--- +- kind: + AllWithModelForm: ~ + location: + row: 6 + column: 8 + end_location: + row: 6 + column: 26 + fix: ~ + parent: ~ +- kind: + AllWithModelForm: ~ + location: + row: 11 + column: 8 + end_location: + row: 11 + column: 27 + fix: ~ + parent: ~ diff --git a/crates/ruff/src/rules/pycodestyle/rules/missing_whitespace_after_keyword.rs b/crates/ruff/src/rules/pycodestyle/rules/missing_whitespace_after_keyword.rs index 0eb89c3efd42a..dab4df3fde49a 100644 --- a/crates/ruff/src/rules/pycodestyle/rules/missing_whitespace_after_keyword.rs +++ b/crates/ruff/src/rules/pycodestyle/rules/missing_whitespace_after_keyword.rs @@ -1,14 +1,13 @@ -#![allow(dead_code)] +#![allow(dead_code, unused_imports)] + +use rustpython_parser::ast::Location; +use rustpython_parser::Tok; use ruff_macros::{define_violation, derive_message_formats}; use crate::registry::DiagnosticKind; -use crate::violation::Violation; - use crate::rules::pycodestyle::helpers::{is_keyword_token, is_singleton_token}; - -use rustpython_parser::ast::Location; -use rustpython_parser::Tok; +use crate::violation::Violation; define_violation!( pub struct MissingWhitespaceAfterKeyword; diff --git a/ruff.schema.json b/ruff.schema.json index 187d608918f8d..99096119f17b2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1538,6 +1538,9 @@ "DJ0", "DJ00", "DJ001", + "DJ003", + "DJ006", + "DJ007", "DJ008", "DJ01", "DJ013",