From 650093ff3bd44be3023692417b2b0502a09687cc Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 29 Nov 2023 18:55:07 -0500 Subject: [PATCH] Django --- .../test/fixtures/pep8_naming/N806.py | 14 ++++- .../src/checkers/ast/analyze/expression.rs | 16 +---- .../src/rules/pep8_naming/helpers.rs | 63 +++++++++++++++++++ .../non_lowercase_variable_in_function.rs | 10 +++ ...les__pep8_naming__tests__N806_N806.py.snap | 18 ++++++ 5 files changed, 107 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pep8_naming/N806.py b/crates/ruff_linter/resources/test/fixtures/pep8_naming/N806.py index e509a27e12bc5..bbaf2b785fc90 100644 --- a/crates/ruff_linter/resources/test/fixtures/pep8_naming/N806.py +++ b/crates/ruff_linter/resources/test/fixtures/pep8_naming/N806.py @@ -1,6 +1,6 @@ import collections from collections import namedtuple -from typing import TypeAlias, TypeVar, NewType, NamedTuple, TypedDict +from typing import Type, TypeAlias, TypeVar, NewType, NamedTuple, TypedDict GLOBAL: str = "foo" @@ -40,3 +40,15 @@ def loop_assign(): global CURRENT_PORT for CURRENT_PORT in range(5): pass + + +def model_assign() -> None: + Bad = apps.get_model("zerver", "Stream") # N806 + Attachment = apps.get_model("zerver", "Attachment") # OK + Recipient = apps.get_model("zerver", model_name="Recipient") # OK + Address: Type = apps.get_model("zerver", "Address") # OK + + from django.utils.module_loading import import_string + + Bad = import_string("django.core.exceptions.ValidationError") # N806 + ValidationError = import_string("django.core.exceptions.ValidationError") # OK diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 7a158449bfcc1..f0e2cdb16bf73 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -205,19 +205,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ExprContext::Store => { if checker.enabled(Rule::NonLowercaseVariableInFunction) { if checker.semantic.current_scope().kind.is_function() { - // Ignore globals. - if !checker - .semantic - .current_scope() - .get(id) - .is_some_and(|binding_id| { - checker.semantic.binding(binding_id).is_global() - }) - { - pep8_naming::rules::non_lowercase_variable_in_function( - checker, expr, id, - ); - } + pep8_naming::rules::non_lowercase_variable_in_function( + checker, expr, id, + ); } } if checker.enabled(Rule::MixedCaseVariableInClassScope) { diff --git a/crates/ruff_linter/src/rules/pep8_naming/helpers.rs b/crates/ruff_linter/src/rules/pep8_naming/helpers.rs index 24fcb6d92dbb3..fc8f6568af827 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/helpers.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/helpers.rs @@ -1,4 +1,5 @@ use itertools::Itertools; +use ruff_python_ast::call_path::collect_call_path; use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use ruff_python_semantic::SemanticModel; @@ -72,6 +73,7 @@ pub(super) fn is_type_alias_assignment(stmt: &Stmt, semantic: &SemanticModel) -> } } +/// Returns `true` if the statement is an assignment to a `TypedDict`. pub(super) fn is_typed_dict_class(arguments: Option<&Arguments>, semantic: &SemanticModel) -> bool { arguments.is_some_and(|arguments| { arguments @@ -81,6 +83,67 @@ pub(super) fn is_typed_dict_class(arguments: Option<&Arguments>, semantic: &Sema }) } +/// Returns `true` if a statement appears to be a dynamic import of a Django model. +/// +/// For example, in Django, it's common to use `get_model` to access a model dynamically, as in: +/// ```python +/// def migrate_existing_attachment_data( +/// apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +/// ) -> None: +/// Attachment = apps.get_model("zerver", "Attachment") +/// ``` +pub(super) fn is_django_model_import(name: &str, stmt: &Stmt, semantic: &SemanticModel) -> bool { + fn match_model_import(name: &str, expr: &Expr, semantic: &SemanticModel) -> bool { + let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = expr + else { + return false; + }; + + // Match against, e.g., `apps.get_model("zerver", "Attachment")`. + if let Some(call_path) = collect_call_path(func.as_ref()) { + if matches!(call_path.as_slice(), [.., "get_model"]) { + if let Some(argument) = + arguments.find_argument("model_name", arguments.args.len() - 1) + { + if let Some(string_literal) = argument.as_string_literal_expr() { + return string_literal.value.to_str() == name; + } + } + } + } + + // Match against, e.g., `import_string("zerver.models.Attachment")`. + if let Some(call_path) = semantic.resolve_call_path(func.as_ref()) { + if matches!( + call_path.as_slice(), + ["django", "utils", "module_loading", "import_string"] + ) { + if let Some(argument) = arguments.find_argument("dotted_path", 0) { + if let Some(string_literal) = argument.as_string_literal_expr() { + if let Some((.., model)) = string_literal.value.to_str().rsplit_once('.') { + return model == name; + } + } + } + } + } + + false + } + + match stmt { + Stmt::AnnAssign(ast::StmtAnnAssign { + value: Some(value), .. + }) => match_model_import(name, value.as_ref(), semantic), + Stmt::Assign(ast::StmtAssign { value, .. }) => { + match_model_import(name, value.as_ref(), semantic) + } + _ => false, + } +} + #[cfg(test)] mod tests { use super::{is_acronym, is_camelcase, is_mixed_case}; diff --git a/crates/ruff_linter/src/rules/pep8_naming/rules/non_lowercase_variable_in_function.rs b/crates/ruff_linter/src/rules/pep8_naming/rules/non_lowercase_variable_in_function.rs index 59df8bf60eeae..3e073f343c9c9 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/rules/non_lowercase_variable_in_function.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/rules/non_lowercase_variable_in_function.rs @@ -53,6 +53,15 @@ impl Violation for NonLowercaseVariableInFunction { /// N806 pub(crate) fn non_lowercase_variable_in_function(checker: &mut Checker, expr: &Expr, name: &str) { + // Ignore globals. + if checker + .semantic() + .lookup_symbol(name) + .is_some_and(|id| checker.semantic().binding(id).is_global()) + { + return; + } + if checker .settings .pep8_naming @@ -72,6 +81,7 @@ pub(crate) fn non_lowercase_variable_in_function(checker: &mut Checker, expr: &E || helpers::is_typed_dict_assignment(parent, checker.semantic()) || helpers::is_type_var_assignment(parent, checker.semantic()) || helpers::is_type_alias_assignment(parent, checker.semantic()) + || helpers::is_django_model_import(name, parent, checker.semantic()) { return; } diff --git a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N806_N806.py.snap b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N806_N806.py.snap index 16718b5a9938b..e572eecccb0ad 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N806_N806.py.snap +++ b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N806_N806.py.snap @@ -20,4 +20,22 @@ N806.py:13:5: N806 Variable `CONSTANT` in function should be lowercase 14 | _ = 0 | +N806.py:46:5: N806 Variable `Bad` in function should be lowercase + | +45 | def model_assign() -> None: +46 | Bad = apps.get_model("zerver", "Stream") # N806 + | ^^^ N806 +47 | Attachment = apps.get_model("zerver", "Attachment") # OK +48 | Recipient = apps.get_model("zerver", model_name="Recipient") # OK + | + +N806.py:53:5: N806 Variable `Bad` in function should be lowercase + | +51 | from django.utils.module_loading import import_string +52 | +53 | Bad = import_string("django.core.exceptions.ValidationError") # N806 + | ^^^ N806 +54 | ValidationError = import_string("django.core.exceptions.ValidationError") # OK + | +