Implement pygrep-hook's Mock-mistake diagnostic (#4366)

crates/ruff/resources/test/fixtures/pygrep-hooks/PGH005_0.py

@@ -0,0 +1,19 @@
+# Errors
+assert my_mock.not_called()
+assert my_mock.called_once_with()
+assert my_mock.not_called
+assert my_mock.called_once_with
+my_mock.assert_not_called
+my_mock.assert_called
+my_mock.assert_called_once_with
+my_mock.assert_called_once_with
+MyMock.assert_called_once_with
+
+# OK
+assert my_mock.call_count == 1
+assert my_mock.called
+my_mock.assert_not_called()
+my_mock.assert_called()
+my_mock.assert_called_once_with()
+"""like :meth:`Mock.assert_called_once_with`"""
+"""like :meth:`MagicMock.assert_called_once_with`"""

crates/ruff/src/checkers/ast/mod.rs

@@ -1657,6 +1657,9 @@ where
                 if self.settings.rules.enabled(Rule::AssertOnStringLiteral) {
                     pylint::rules::assert_on_string_literal(self, test);
                 }
+                if self.settings.rules.enabled(Rule::InvalidMockAccess) {
+                    pygrep_hooks::rules::non_existent_mock_method(self, test);
+                }
             }
             StmtKind::With { items, body, .. } => {
                 if self.settings.rules.enabled(Rule::AssertRaisesException) {
@@ -1948,6 +1951,9 @@ where
                 if self.settings.rules.enabled(Rule::UselessExpression) {
                     flake8_bugbear::rules::useless_expression(self, value);
                 }
+                if self.settings.rules.enabled(Rule::InvalidMockAccess) {
+                    pygrep_hooks::rules::uncalled_mock_method(self, value);
+                }
                 if self.settings.rules.enabled(Rule::AsyncioDanglingTask) {
                     if let Some(diagnostic) = ruff::rules::asyncio_dangling_task(value, |expr| {
                         self.ctx.resolve_call_path(expr)

crates/ruff/src/codes.rs

@@ -563,6 +563,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (PygrepHooks, "002") => Rule::DeprecatedLogWarn,
         (PygrepHooks, "003") => Rule::BlanketTypeIgnore,
         (PygrepHooks, "004") => Rule::BlanketNOQA,
+        (PygrepHooks, "005") => Rule::InvalidMockAccess,
 
         // pandas-vet
         (PandasVet, "002") => Rule::PandasUseOfInplaceArgument,

crates/ruff/src/registry.rs

@@ -506,10 +506,11 @@ ruff_macros::register_rules!(
     rules::flake8_datetimez::rules::CallDateToday,
     rules::flake8_datetimez::rules::CallDateFromtimestamp,
     // pygrep-hooks
-    rules::pygrep_hooks::rules::Eval,
-    rules::pygrep_hooks::rules::DeprecatedLogWarn,
-    rules::pygrep_hooks::rules::BlanketTypeIgnore,
     rules::pygrep_hooks::rules::BlanketNOQA,
+    rules::pygrep_hooks::rules::BlanketTypeIgnore,
+    rules::pygrep_hooks::rules::DeprecatedLogWarn,
+    rules::pygrep_hooks::rules::Eval,
+    rules::pygrep_hooks::rules::InvalidMockAccess,
     // pandas-vet
     rules::pandas_vet::rules::PandasUseOfInplaceArgument,
     rules::pandas_vet::rules::PandasUseOfDotIsNull,

crates/ruff/src/rules/pygrep_hooks/mod.rs

@@ -20,6 +20,7 @@ mod tests {
     #[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_0.py"); "PGH003_0")]
     #[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_1.py"); "PGH003_1")]
     #[test_case(Rule::BlanketNOQA, Path::new("PGH004_0.py"); "PGH004_0")]
+    #[test_case(Rule::InvalidMockAccess, Path::new("PGH005_0.py"); "PGH005_0")]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
         let diagnostics = test_path(

crates/ruff/src/rules/pygrep_hooks/rules/invalid_mock_access.rs

@@ -0,0 +1,99 @@
+use rustpython_parser::ast::{Expr, ExprKind};
+
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+
+use crate::checkers::ast::Checker;
+
+#[derive(Debug, PartialEq, Eq)]
+enum Reason {
+    UncalledMethod(String),
+    NonExistentMethod(String),
+}
+
+/// ## What it does
+/// Checks for common mistakes when using mock objects.
+///
+/// ## Why is this bad?
+/// The `mock` module exposes an assertion API that can be used to verify that
+/// mock objects undergo expected interactions. This rule checks for common
+/// mistakes when using this API.
+///
+/// For example, it checks for mock attribute accesses that should be replaced
+/// with mock method calls.
+///
+/// ## Example
+/// ```python
+/// my_mock.assert_called
+/// ```
+///
+/// Use instead:
+/// ```python
+/// my_mock.assert_called()
+/// ```
+#[violation]
+pub struct InvalidMockAccess {
+    reason: Reason,
+}
+
+impl Violation for InvalidMockAccess {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let InvalidMockAccess { reason } = self;
+        match reason {
+            Reason::UncalledMethod(name) => format!("Mock method should be called: `{name}`"),
+            Reason::NonExistentMethod(name) => format!("Non-existent mock method: `{name}`"),
+        }
+    }
+}
+
+/// PGH005
+pub fn uncalled_mock_method(checker: &mut Checker, expr: &Expr) {
+    if let ExprKind::Attribute { attr, .. } = &expr.node {
+        if matches!(
+            attr.as_str(),
+            "assert_any_call"
+                | "assert_called"
+                | "assert_called_once"
+                | "assert_called_once_with"
+                | "assert_called_with"
+                | "assert_has_calls"
+                | "assert_not_called"
+        ) {
+            checker.diagnostics.push(Diagnostic::new(
+                InvalidMockAccess {
+                    reason: Reason::UncalledMethod(attr.to_string()),
+                },
+                expr.range(),
+            ));
+        }
+    }
+}
+
+/// PGH005
+pub fn non_existent_mock_method(checker: &mut Checker, test: &Expr) {
+    let attr = match &test.node {
+        ExprKind::Attribute { attr, .. } => attr,
+        ExprKind::Call { func, .. } => match &func.node {
+            ExprKind::Attribute { attr, .. } => attr,
+            _ => return,
+        },
+        _ => return,
+    };
+    if matches!(
+        attr.as_str(),
+        "any_call"
+            | "called_once"
+            | "called_once_with"
+            | "called_with"
+            | "has_calls"
+            | "not_called"
+    ) {
+        checker.diagnostics.push(Diagnostic::new(
+            InvalidMockAccess {
+                reason: Reason::NonExistentMethod(attr.to_string()),
+            },
+            test.range(),
+        ));
+    }
+}

crates/ruff/src/rules/pygrep_hooks/rules/mod.rs

@@ -1,9 +1,13 @@
 pub(crate) use blanket_noqa::{blanket_noqa, BlanketNOQA};
 pub(crate) use blanket_type_ignore::{blanket_type_ignore, BlanketTypeIgnore};
-pub use deprecated_log_warn::{deprecated_log_warn, DeprecatedLogWarn};
-pub use no_eval::{no_eval, Eval};
+pub(crate) use deprecated_log_warn::{deprecated_log_warn, DeprecatedLogWarn};
+pub(crate) use invalid_mock_access::{
+    non_existent_mock_method, uncalled_mock_method, InvalidMockAccess,
+};
+pub(crate) use no_eval::{no_eval, Eval};
 
 mod blanket_noqa;
 mod blanket_type_ignore;
 mod deprecated_log_warn;
+mod invalid_mock_access;
 mod no_eval;

crates/ruff/src/rules/pygrep_hooks/snapshots/ruff__rules__pygrep_hooks__tests__PGH005_PGH005_0.py.snap

@@ -0,0 +1,92 @@
+---
+source: crates/ruff/src/rules/pygrep_hooks/mod.rs
+---
+PGH005_0.py:2:8: PGH005 Non-existent mock method: `not_called`
+  |
+2 | # Errors
+3 | assert my_mock.not_called()
+  |        ^^^^^^^^^^^^^^^^^^^^ PGH005
+4 | assert my_mock.called_once_with()
+5 | assert my_mock.not_called
+  |
+
+PGH005_0.py:3:8: PGH005 Non-existent mock method: `called_once_with`
+  |
+3 | # Errors
+4 | assert my_mock.not_called()
+5 | assert my_mock.called_once_with()
+  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+6 | assert my_mock.not_called
+7 | assert my_mock.called_once_with
+  |
+
+PGH005_0.py:4:8: PGH005 Non-existent mock method: `not_called`
+  |
+4 | assert my_mock.not_called()
+5 | assert my_mock.called_once_with()
+6 | assert my_mock.not_called
+  |        ^^^^^^^^^^^^^^^^^^ PGH005
+7 | assert my_mock.called_once_with
+8 | my_mock.assert_not_called
+  |
+
+PGH005_0.py:5:8: PGH005 Non-existent mock method: `called_once_with`
+  |
+5 | assert my_mock.called_once_with()
+6 | assert my_mock.not_called
+7 | assert my_mock.called_once_with
+  |        ^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+8 | my_mock.assert_not_called
+9 | my_mock.assert_called
+  |
+
+PGH005_0.py:6:1: PGH005 Mock method should be called: `assert_not_called`
+   |
+ 6 | assert my_mock.not_called
+ 7 | assert my_mock.called_once_with
+ 8 | my_mock.assert_not_called
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+ 9 | my_mock.assert_called
+10 | my_mock.assert_called_once_with
+   |
+
+PGH005_0.py:7:1: PGH005 Mock method should be called: `assert_called`
+   |
+ 7 | assert my_mock.called_once_with
+ 8 | my_mock.assert_not_called
+ 9 | my_mock.assert_called
+   | ^^^^^^^^^^^^^^^^^^^^^ PGH005
+10 | my_mock.assert_called_once_with
+11 | my_mock.assert_called_once_with
+   |
+
+PGH005_0.py:8:1: PGH005 Mock method should be called: `assert_called_once_with`
+   |
+ 8 | my_mock.assert_not_called
+ 9 | my_mock.assert_called
+10 | my_mock.assert_called_once_with
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+11 | my_mock.assert_called_once_with
+12 | MyMock.assert_called_once_with
+   |
+
+PGH005_0.py:9:1: PGH005 Mock method should be called: `assert_called_once_with`
+   |
+ 9 | my_mock.assert_called
+10 | my_mock.assert_called_once_with
+11 | my_mock.assert_called_once_with
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+12 | MyMock.assert_called_once_with
+   |
+
+PGH005_0.py:10:1: PGH005 Mock method should be called: `assert_called_once_with`
+   |
+10 | my_mock.assert_called_once_with
+11 | my_mock.assert_called_once_with
+12 | MyMock.assert_called_once_with
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PGH005
+13 | 
+14 | # OK
+   |
+
+