From 2606ce2de47252feb80df1d756ac1346b0a30b80 Mon Sep 17 00:00:00 2001 From: Jannic Warken Date: Tue, 13 May 2025 09:53:27 +0200 Subject: [PATCH] [`sqlalchemy`] Implement first SQLAlchemy rule (`SA001`) --- .../test/fixtures/sqlalchemy/SA001.py | 71 +++++++++++++ .../src/checkers/ast/analyze/statement.rs | 6 +- crates/ruff_linter/src/codes.rs | 3 + crates/ruff_linter/src/registry.rs | 3 + crates/ruff_linter/src/rules/mod.rs | 1 + .../src/rules/sqlalchemy/helpers.rs | 22 ++++ .../ruff_linter/src/rules/sqlalchemy/mod.rs | 25 +++++ .../rules/missing_mapped_type_annotation.rs | 98 +++++++++++++++++ .../src/rules/sqlalchemy/rules/mod.rs | 3 + ...es__sqlalchemy__tests__SA001_SA001.py.snap | 100 ++++++++++++++++++ crates/ruff_python_semantic/src/model.rs | 2 + ruff.schema.json | 4 + 12 files changed, 337 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/sqlalchemy/SA001.py create mode 100644 crates/ruff_linter/src/rules/sqlalchemy/helpers.rs create mode 100644 crates/ruff_linter/src/rules/sqlalchemy/mod.rs create mode 100644 crates/ruff_linter/src/rules/sqlalchemy/rules/missing_mapped_type_annotation.rs create mode 100644 crates/ruff_linter/src/rules/sqlalchemy/rules/mod.rs create mode 100644 crates/ruff_linter/src/rules/sqlalchemy/snapshots/ruff_linter__rules__sqlalchemy__tests__SA001_SA001.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/sqlalchemy/SA001.py b/crates/ruff_linter/resources/test/fixtures/sqlalchemy/SA001.py new file mode 100644 index 0000000000..f13a6fd702 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/sqlalchemy/SA001.py @@ -0,0 +1,71 @@ +from typing import List + +from sqlalchemy import Integer, ForeignKey, String, func +from sqlalchemy.ext.associationproxy import association_proxy, AssociationProxy +from sqlalchemy.orm import ( + Mapped, + mapped_column, + DeclarativeBase, + relationship, + column_property, + MappedSQLExpression, + synonym, + Synonym, +) + + +class Base(DeclarativeBase): + pass + + +class Parent(Base): + __tablename__ = "parent" + + id: Mapped[int] = mapped_column(primary_key=True) + children: Mapped[List["Child"]] = relationship(back_populates="parent") + + name: Mapped[str] + + parent_name: Synonym[str] = synonym("name") + + +class Child(Base): + __tablename__ = "child" + + id: Mapped[int] = mapped_column(primary_key=True) + parent_id: Mapped[int] = mapped_column(ForeignKey("parent.id")) + parent: Mapped["Parent"] = relationship(back_populates="children") + + parent_name: AssociationProxy[str] = association_proxy("parent", "name") + + first_name: Mapped[str] = mapped_column(String) + last_name: Mapped[str] = mapped_column() + + name_length: MappedSQLExpression[int] = column_property( + func.length(first_name + last_name) + ) + + +class Company(Base): + __tablename__ = "company" + + id = mapped_column(Integer, primary_key=True) + employees = relationship("Employee", back_populates="company") + + name = mapped_column(String) + company_name = synonym("name") + + +class Employee(Base): + __tablename__ = "employee" + + id = mapped_column(Integer, primary_key=True) + company_id = mapped_column(ForeignKey("company.id")) + company = relationship("Company", back_populates="employees") + + company_name = association_proxy("company", "name") + + first_name = mapped_column(String) + last_name = mapped_column(String) + + name_length = column_property(func.length(first_name + last_name)) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 84563726a2..d6fb7d12a2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -12,7 +12,8 @@ use crate::rules::{ flake8_builtins, flake8_debugger, flake8_django, flake8_errmsg, flake8_import_conventions, flake8_pie, flake8_pyi, flake8_pytest_style, flake8_raise, flake8_return, flake8_simplify, flake8_slots, flake8_tidy_imports, flake8_type_checking, mccabe, pandas_vet, pep8_naming, - perflint, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, tryceratops, + perflint, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade, refurb, ruff, sqlalchemy, + tryceratops, }; use ruff_python_ast::PythonVersion; @@ -412,6 +413,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::DjangoUnorderedBodyContentInModel) { flake8_django::rules::unordered_body_content_in_model(checker, class_def); } + if checker.enabled(Rule::SQLAlchemyMissingMappedTypeAnnotation) { + sqlalchemy::rules::missing_mapped_type_annotation(checker, body); + } if !checker.source_type.is_stub() { if checker.enabled(Rule::DjangoModelWithoutDunderStr) { flake8_django::rules::model_without_dunder_str(checker, class_def); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f2bde04729..eb4682a79b 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1142,6 +1142,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Logging, "014") => (RuleGroup::Preview, rules::flake8_logging::rules::ExcInfoOutsideExceptHandler), (Flake8Logging, "015") => (RuleGroup::Stable, rules::flake8_logging::rules::RootLoggerCall), + // SQLAlchemy + (SQLAlchemy, "001") => (RuleGroup::Preview, rules::sqlalchemy::rules::SQLAlchemyMissingMappedTypeAnnotation), + _ => return None, }) } diff --git a/crates/ruff_linter/src/registry.rs b/crates/ruff_linter/src/registry.rs index 9b03a8b7f3..5a929c35bc 100644 --- a/crates/ruff_linter/src/registry.rs +++ b/crates/ruff_linter/src/registry.rs @@ -208,6 +208,9 @@ pub enum Linter { /// Ruff-specific rules #[prefix = "RUF"] Ruff, + /// [SQLAlchemy](https://pypi.org/project/SQLAlchemy/) + #[prefix = "SA"] + SQLAlchemy, /// [tryceratops](https://pypi.org/project/tryceratops/) #[prefix = "TRY"] Tryceratops, diff --git a/crates/ruff_linter/src/rules/mod.rs b/crates/ruff_linter/src/rules/mod.rs index 735864aecb..fe2aca0556 100644 --- a/crates/ruff_linter/src/rules/mod.rs +++ b/crates/ruff_linter/src/rules/mod.rs @@ -56,4 +56,5 @@ pub mod pylint; pub mod pyupgrade; pub mod refurb; pub mod ruff; +pub mod sqlalchemy; pub mod tryceratops; diff --git a/crates/ruff_linter/src/rules/sqlalchemy/helpers.rs b/crates/ruff_linter/src/rules/sqlalchemy/helpers.rs new file mode 100644 index 0000000000..21762f5378 --- /dev/null +++ b/crates/ruff_linter/src/rules/sqlalchemy/helpers.rs @@ -0,0 +1,22 @@ +use ruff_python_ast::Expr; +use ruff_python_semantic::SemanticModel; + +pub(in crate::rules::sqlalchemy) fn is_mapped_attribute( + expr: &Expr, + semantic: &SemanticModel, +) -> bool { + semantic + .resolve_qualified_name(expr) + .is_some_and(|qualified_name| { + [ + ["sqlalchemy", "ext", "associationproxy"], + ["sqlalchemy", "orm", "column_property"], + ["sqlalchemy", "orm", "composite"], + ["sqlalchemy", "orm", "mapped_column"], + ["sqlalchemy", "orm", "relationship"], + ["sqlalchemy", "orm", "synonym"], + ] + .iter() + .any(|n| qualified_name.segments().starts_with(n)) + }) +} diff --git a/crates/ruff_linter/src/rules/sqlalchemy/mod.rs b/crates/ruff_linter/src/rules/sqlalchemy/mod.rs new file mode 100644 index 0000000000..19d5446de9 --- /dev/null +++ b/crates/ruff_linter/src/rules/sqlalchemy/mod.rs @@ -0,0 +1,25 @@ +mod helpers; +pub(crate) mod rules; + +#[cfg(test)] +mod tests { + use std::path::Path; + + use anyhow::Result; + use test_case::test_case; + + use crate::registry::Rule; + use crate::test::test_path; + use crate::{assert_messages, settings}; + + #[test_case(Rule::SQLAlchemyMissingMappedTypeAnnotation, Path::new("SA001.py"))] + fn rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("sqlalchemy").join(path).as_path(), + &settings::LinterSettings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/crates/ruff_linter/src/rules/sqlalchemy/rules/missing_mapped_type_annotation.rs b/crates/ruff_linter/src/rules/sqlalchemy/rules/missing_mapped_type_annotation.rs new file mode 100644 index 0000000000..299d3109d1 --- /dev/null +++ b/crates/ruff_linter/src/rules/sqlalchemy/rules/missing_mapped_type_annotation.rs @@ -0,0 +1,98 @@ +use ruff_diagnostics::{Diagnostic, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; + +use ruff_python_ast::{self as ast, Expr, Stmt}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +use crate::rules::sqlalchemy::helpers; +use ruff_python_semantic::Modules; + +/// ## What it does +/// Checks for existence of `Mapped` or other ORM container class type annotations in SQLAlchemy +/// models. +/// +/// ## Why is this bad? +/// If an annotation is missing, type checkers will treat the corresponding field as type `Any`. +/// +/// ## Example +/// ```python +/// from sqlalchemy import Integer +/// from sqlalchemy.orm import DeclarativeBase +/// from sqlalchemy.orm import Mapped +/// from sqlalchemy.orm import mapped_column +/// +/// class Base(DeclarativeBase): +/// pass +/// +/// +/// class MyModel(Base): +/// __tablename__ = "my_model" +/// id: Mapped[int] = mapped_column(primary_key=True) +/// +/// count = mapped_column(Integer) +/// +/// +/// m = MyModel() +/// reveal_type(m.count) # note: Revealed type is "Any" +/// ``` +/// +/// Use instead: +/// ```python +/// from sqlalchemy.orm import DeclarativeBase +/// from sqlalchemy.orm import Mapped +/// from sqlalchemy.orm import mapped_column +/// +/// class Base(DeclarativeBase): +/// pass +/// +/// +/// class MyModel(Base): +/// __tablename__ = "my_model" +/// id: Mapped[int] = mapped_column(primary_key=True) +/// +/// count: Mapped[int] +/// +/// +/// m = MyModel() +/// reveal_type(m.count) # note: Revealed type is "builtins.int" +/// ``` +#[derive(ViolationMetadata)] +pub(crate) struct SQLAlchemyMissingMappedTypeAnnotation; + +impl Violation for SQLAlchemyMissingMappedTypeAnnotation { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; + + #[derive_message_formats] + fn message(&self) -> String { + "Missing `Mapped` or other ORM container class type annotation".to_string() + } +} + +/// SA001 +pub(crate) fn missing_mapped_type_annotation(checker: &mut Checker, body: &[Stmt]) { + if !checker.semantic().seen_module(Modules::SQLALCHEMY) { + return; + } + + for statement in body { + let Stmt::Assign(ast::StmtAssign { value, targets, .. }) = statement else { + continue; + }; + + if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { + if helpers::is_mapped_attribute(func, checker.semantic()) { + // SQLAlchemy does not allow multiple targets for column assignments. + let [target] = targets.as_slice() else { + continue; + }; + + checker.report_diagnostic(Diagnostic::new( + SQLAlchemyMissingMappedTypeAnnotation {}, + target.range(), + )); + } + } + } +} diff --git a/crates/ruff_linter/src/rules/sqlalchemy/rules/mod.rs b/crates/ruff_linter/src/rules/sqlalchemy/rules/mod.rs new file mode 100644 index 0000000000..2bdc22e894 --- /dev/null +++ b/crates/ruff_linter/src/rules/sqlalchemy/rules/mod.rs @@ -0,0 +1,3 @@ +pub(crate) use missing_mapped_type_annotation::*; + +mod missing_mapped_type_annotation; diff --git a/crates/ruff_linter/src/rules/sqlalchemy/snapshots/ruff_linter__rules__sqlalchemy__tests__SA001_SA001.py.snap b/crates/ruff_linter/src/rules/sqlalchemy/snapshots/ruff_linter__rules__sqlalchemy__tests__SA001_SA001.py.snap new file mode 100644 index 0000000000..408bf3e928 --- /dev/null +++ b/crates/ruff_linter/src/rules/sqlalchemy/snapshots/ruff_linter__rules__sqlalchemy__tests__SA001_SA001.py.snap @@ -0,0 +1,100 @@ +--- +source: crates/ruff_linter/src/rules/sqlalchemy/mod.rs +--- +SA001.py:52:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +50 | __tablename__ = "company" +51 | +52 | id = mapped_column(Integer, primary_key=True) + | ^^ SA001 +53 | employees = relationship("Employee", back_populates="company") + | + +SA001.py:53:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +52 | id = mapped_column(Integer, primary_key=True) +53 | employees = relationship("Employee", back_populates="company") + | ^^^^^^^^^ SA001 +54 | +55 | name = mapped_column(String) + | + +SA001.py:55:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +53 | employees = relationship("Employee", back_populates="company") +54 | +55 | name = mapped_column(String) + | ^^^^ SA001 +56 | company_name = synonym("name") + | + +SA001.py:56:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +55 | name = mapped_column(String) +56 | company_name = synonym("name") + | ^^^^^^^^^^^^ SA001 + | + +SA001.py:62:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +60 | __tablename__ = "employee" +61 | +62 | id = mapped_column(Integer, primary_key=True) + | ^^ SA001 +63 | company_id = mapped_column(ForeignKey("company.id")) +64 | company = relationship("Company", back_populates="employees") + | + +SA001.py:63:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +62 | id = mapped_column(Integer, primary_key=True) +63 | company_id = mapped_column(ForeignKey("company.id")) + | ^^^^^^^^^^ SA001 +64 | company = relationship("Company", back_populates="employees") + | + +SA001.py:64:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +62 | id = mapped_column(Integer, primary_key=True) +63 | company_id = mapped_column(ForeignKey("company.id")) +64 | company = relationship("Company", back_populates="employees") + | ^^^^^^^ SA001 +65 | +66 | company_name = association_proxy("company", "name") + | + +SA001.py:66:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +64 | company = relationship("Company", back_populates="employees") +65 | +66 | company_name = association_proxy("company", "name") + | ^^^^^^^^^^^^ SA001 +67 | +68 | first_name = mapped_column(String) + | + +SA001.py:68:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +66 | company_name = association_proxy("company", "name") +67 | +68 | first_name = mapped_column(String) + | ^^^^^^^^^^ SA001 +69 | last_name = mapped_column(String) + | + +SA001.py:69:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +68 | first_name = mapped_column(String) +69 | last_name = mapped_column(String) + | ^^^^^^^^^ SA001 +70 | +71 | name_length = column_property(func.length(first_name + last_name)) + | + +SA001.py:71:5: SA001 Missing `Mapped` or other ORM container class type annotation + | +69 | last_name = mapped_column(String) +70 | +71 | name_length = column_property(func.length(first_name + last_name)) + | ^^^^^^^^^^^ SA001 + | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 70c26cd615..a3659fcc28 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1482,6 +1482,7 @@ impl<'a> SemanticModel<'a> { "airflow" => self.seen.insert(Modules::AIRFLOW), "hashlib" => self.seen.insert(Modules::HASHLIB), "crypt" => self.seen.insert(Modules::CRYPT), + "sqlalchemy" => self.seen.insert(Modules::SQLALCHEMY), _ => {} } } @@ -2157,6 +2158,7 @@ bitflags! { const AIRFLOW = 1 << 27; const HASHLIB = 1 << 28; const CRYPT = 1 << 29; + const SQLALCHEMY = 1 << 30; } } diff --git a/ruff.schema.json b/ruff.schema.json index 0059b7bb2f..0bc2a50051 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4137,6 +4137,10 @@ "S701", "S702", "S704", + "SA", + "SA0", + "SA00", + "SA001", "SIM", "SIM1", "SIM10",