From 9525a0c4e8f1ae45b11ba910bcbfeb32bf2eb691 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 26 Oct 2026 11:15:49 -0700 Subject: [PATCH] [suspicious] Add `shared-clock-mock` (#29074) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR adds a `shared-clock-mock` rule in preview. Patching a clock or sleep function on a shared module can affect unrelated threads or tasks, including when the target is qualified with the consuming module: ```python from unittest.mock import Mock, patch from example import worker sleep = Mock() with patch.object(worker.time, "sleep", sleep): worker.retry() ``` Instead, you should replace the consuming module's binding to keep the fake local: ```python from types import SimpleNamespace from unittest.mock import Mock, patch from example import worker sleep = Mock() with patch.object(worker, "time", SimpleNamespace(sleep=sleep)): worker.retry() ``` We also flag patches of wall-clock, monotonic, and performance-counter readings, plus `asyncio.sleep`. The rule recognizes `patch`, `patch.object`, pytest-mock fixtures, and `monkeypatch.setattr` / `delattr`. (Nested `.time` and `.asyncio` targets use a naming heuristic, so a fixture that already installed a private clock can produce a false positive.) ## Case analysis The ecosystem run adds 601 diagnostics across eight projects. ### Counting sleep calls in LangChain This reduced version of LangChain’s device-login test records the polling interval before and after the server asks the client to slow down. ```python sleeps = [] monkeypatch.setattr(oauth_module.time, "sleep", sleeps.append) login_chatgpt_device(store_path=tmp_path / "x.json", poll_interval=2.0) assert sleeps[:2] == [2.0, 7.0] ``` The OAuth module does `import time`, so `oauth_module.time` and `time` are the same object. If another thread calls `time.sleep(1)` between the two polls, the recorder sees `[2.0, 1, 7.0]`. Installing `SimpleNamespace(sleep=sleeps.append)` on `oauth_module.time` keeps that call out of the list. ### Running out of clock readings in Superset Superset tests how long a screenshot request waits for a lock. It patches `superset.dashboards.api.time.monotonic` with `side_effect=[10.0, 10.5, 11.0]` and patches `time.sleep` on the same module. An unrelated `time.monotonic()` can consume the next reading. The request can then see the deadline early or exhaust the iterator and raise `StopIteration`. A private clock for `superset.dashboards.api` needs both `monotonic` and `sleep`. ### Retrying an async request in Prefect Prefect simulates three failed SQS requests and expects one two-second backoff. The test uses `@patch("prefect_aws.observers.ecs.asyncio.sleep")`, but the observer does `import asyncio`. A second task awaiting `asyncio.sleep(0)` would also call the mock and break `assert_called_once_with(2)`. ### A fixture that already installed a private clock Ruff can see the same patch in two tests even though only one reaches the shared module. For example, an autouse fixture can replace `worker.time` before the test starts. ```python @pytest.fixture(autouse=True) def private_clock(monkeypatch): monkeypatch.setattr(worker, "time", SimpleNamespace(sleep=lambda _: None)) def test_retry(monkeypatch): sleep = Mock() monkeypatch.setattr(worker.time, "sleep", sleep) # shared-clock-mock worker.retry() ``` The warning in `test_retry` is a false positive. At runtime the receiver is the fixture’s `SimpleNamespace`. If the fixture returns that namespace, the test can use `monkeypatch.setattr(private_clock, "sleep", sleep)` and Ruff accepts it. ### A package with its own `time` module Gevent ships a `gevent.time` module with its own `sleep` binding. The dotted string is enough for Ruff to flag this reduced example. ```python import time from unittest.mock import patch original_sleep = time.sleep with patch("gevent.time.sleep"): assert time.sleep is original_sleep ``` The assertion passes. Callers that look up `gevent.time.sleep` see the patch. A caller of standard-library `time.sleep` still gets the original function. Ruff infers the owner from the `.time.sleep` suffix in a string target, so it reports the wrong module here. --- .../mdtest/ruff/shared-clock-mock.md | 287 ++++++++++++++++++ .../src/checkers/ast/analyze/expression.rs | 8 + crates/ruff_linter/src/codes.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../src/rules/ruff/rules/shared_clock_mock.rs | 224 ++++++++++++++ crates/ruff_linter/src/settings/mod.rs | 1 + ruff.schema.json | 1 + 7 files changed, 524 insertions(+) create mode 100644 crates/ruff_linter/resources/mdtest/ruff/shared-clock-mock.md create mode 100644 crates/ruff_linter/src/rules/ruff/rules/shared_clock_mock.rs diff --git a/crates/ruff_linter/resources/mdtest/ruff/shared-clock-mock.md b/crates/ruff_linter/resources/mdtest/ruff/shared-clock-mock.md new file mode 100644 index 0000000000..7c62ccbb72 --- /dev/null +++ b/crates/ruff_linter/resources/mdtest/ruff/shared-clock-mock.md @@ -0,0 +1,287 @@ +# `shared-clock-mock` + +```toml +lint.preview = true +lint.select = ["shared-clock-mock"] +``` + +## Shared sleep functions + +Patching a shared module's sleep function can intercept unrelated callers, even +when the target is qualified with the consuming module. + +```py +import time +from unittest.mock import patch + +from example import worker + +with patch("time.sleep"): # snapshot: shared-clock-mock + pass + +with patch.object(time, "sleep"): # error: [shared-clock-mock] + pass + +with patch("example.worker.time.sleep"): # error: [shared-clock-mock] + pass + +with patch.object(worker.time, "sleep"): # error: [shared-clock-mock] + pass +``` + +```snapshot +error[shared-clock-mock]: Changing `time.sleep` may affect unrelated threads or tasks + --> src/mdtest_snippet.py:6:12 + | +6 | with patch("time.sleep"): # snapshot: shared-clock-mock + | ^^^^^^^^^^^^ +help: Replace the binding in the consuming module with a private clock +``` + +## Clock readings + +Unrelated callers can also consume a clock mock's finite sequence of readings. + +```py +import time +from unittest.mock import patch + +patch.object(time, "time") # error: [shared-clock-mock] +patch.object(time, "time_ns") # error: [shared-clock-mock] +patch.object(time, "monotonic") # error: [shared-clock-mock] +patch.object(time, "monotonic_ns") # error: [shared-clock-mock] +patch.object(time, "perf_counter") # error: [shared-clock-mock] +patch.object(time, "perf_counter_ns") # error: [shared-clock-mock] +patch("example.worker.time.perf_counter") # error: [shared-clock-mock] + +# Other time APIs are outside this rule's scope. +patch.object(time, "strftime") +patch("time.localtime") +``` + +## Import aliases and keyword arguments + +```py +import asyncio as aio +import time as clock +from unittest import mock as testing +from unittest.mock import patch as replace + +testing.patch.object(clock, "sleep") # error: [shared-clock-mock] +testing.patch(target="asyncio.sleep") # error: [shared-clock-mock] +replace.object(target=aio, attribute="sleep") # error: [shared-clock-mock] +replace(target="time.monotonic", return_value=0) # error: [shared-clock-mock] + + +@replace("example.worker.asyncio.sleep") # error: [shared-clock-mock] +async def example(sleep): + pass +``` + +## Pytest fixtures + +The conventional fixture names identify pytest's monkeypatch API and pytest-mock's +patch APIs without requiring an import in the test file. + +```py +import time + +from example import worker + + +def example(monkeypatch, mocker, class_mocker, module_mocker, package_mocker, session_mocker): + monkeypatch.setattr(time, "sleep", lambda _: None) # error: [shared-clock-mock] + monkeypatch.setattr(worker.time, "sleep", lambda _: None) # error: [shared-clock-mock] + monkeypatch.setattr(worker.time, "monotonic", lambda: 0) # error: [shared-clock-mock] + monkeypatch.setattr("example.worker.asyncio.sleep", fake_sleep) # error: [shared-clock-mock] + monkeypatch.setattr(target=time, name="perf_counter", value=lambda: 0) # error: [shared-clock-mock] + + mocker.patch("time.sleep") # error: [shared-clock-mock] + class_mocker.patch("time.sleep") # error: [shared-clock-mock] + module_mocker.patch("time.sleep") # error: [shared-clock-mock] + package_mocker.patch("time.sleep") # error: [shared-clock-mock] + session_mocker.patch("time.sleep") # error: [shared-clock-mock] + + mocker.patch.object(time, "sleep") # error: [shared-clock-mock] + class_mocker.patch.object(time, "sleep") # error: [shared-clock-mock] + module_mocker.patch.object(time, "sleep") # error: [shared-clock-mock] + package_mocker.patch.object(time, "sleep") # error: [shared-clock-mock] + session_mocker.patch.object(time, "sleep") # error: [shared-clock-mock] +``` + +## Writes to clock attributes + +Assigning or deleting a shared attribute changes it for every caller. Reading +the attribute, annotating it without a value, or replacing the consuming +module's binding does not. + +```py +import asyncio as aio +import time as clock +from types import SimpleNamespace + +from example import worker + +clock.sleep = fake # error: [shared-clock-mock] +worker.time.monotonic = fake # error: [shared-clock-mock] +del aio.sleep # error: [shared-clock-mock] +worker.time.time: object = fake # error: [shared-clock-mock] + +saved = worker.time.sleep +worker.time.sleep: object +worker.time = SimpleNamespace(sleep=fake) +``` + +Attribute targets also write to the shared module when bound by a loop, a +context manager, or a comprehension. + +```py +import time + +for time.sleep in replacements: # error: [shared-clock-mock] + pass +with replacement() as time.monotonic: # error: [shared-clock-mock] + pass +readings = [time.time() for time.time in clocks] # error: [shared-clock-mock] +``` + +## Removing attributes with monkeypatch + +```py +import time + +def example(monkeypatch): + monkeypatch.delattr(time, "sleep") # error: [shared-clock-mock] + monkeypatch.delattr("example.worker.time.monotonic") # error: [shared-clock-mock] + monkeypatch.delattr("example.worker.time") +``` + +## Constructed monkeypatch instances + +```py +import time +from pytest import MonkeyPatch + +mp = MonkeyPatch() +mp.setattr(time, "sleep", lambda _: None) # error: [shared-clock-mock] +``` + +## Multiline patch targets + +The diagnostic spans both the module and the attribute that will change. + +```py +from example import worker + +def test_retry(monkeypatch): + monkeypatch.setattr( + worker.time, # snapshot: shared-clock-mock + "sleep", + lambda _: None, + ) +``` + +```snapshot +error[shared-clock-mock]: Changing `time.sleep` may affect unrelated threads or tasks + --> src/mdtest_snippet.py:5:9 + | +5 | / worker.time, # snapshot: shared-clock-mock +6 | | "sleep", + | |_______________^ +help: Replace the binding in the consuming module with a private clock +``` + +## Module-local replacements + +Replacing the consuming module's binding leaves the shared module intact. A local +sleep binding and a directly constructed clock can also be patched independently. + +```py +import asyncio +import time +from types import SimpleNamespace +from unittest.mock import AsyncMock, Mock, patch + +from example import worker + + +def example(monkeypatch, mocker): + monkeypatch.setattr(worker, "time", SimpleNamespace(sleep=Mock())) + monkeypatch.setattr("example.worker.time", SimpleNamespace(sleep=Mock())) + mocker.patch.object(worker, "time", SimpleNamespace(sleep=Mock())) + mocker.patch("example.worker.time", SimpleNamespace(sleep=Mock())) + monkeypatch.setattr(worker, "asyncio", SimpleNamespace(sleep=AsyncMock())) + monkeypatch.setattr(worker, "sleep", Mock()) + mocker.patch("example.worker.sleep") + + +with patch.object(worker, "time", SimpleNamespace(sleep=Mock())): + pass + +with patch("example.worker.asyncio", SimpleNamespace(**(vars(asyncio) | {"sleep": AsyncMock()}))): + pass + +time = SimpleNamespace(sleep=Mock()) +patch.object(time, "sleep") +``` + +## Unrelated methods and shadowed patch APIs + +```py +import time +from unittest.mock import patch + +from example import mocker, monkeypatch + +mocker.patch("time.sleep") +monkeypatch.setattr(time, "sleep", fake) +other.patch("time.sleep") +other.setattr(time, "sleep", fake) + + +def example(patch): + patch("time.sleep") + patch.object(time, "sleep") + + +patch("asyncio.wait") +patch("example.sleep") +patch("example.clock.sleep") +patch.object(time, dynamic_attribute) +patch(dynamic_target) +patch(f"{module_name}.time.sleep") +``` + +## Attribute heuristic + +Attribute targets ending in `time` or `asyncio` are treated as shared modules, +including on dynamically loaded modules. An already-isolated attribute is a known +false positive and can be suppressed by rule name. + +```py +from types import SimpleNamespace +from unittest.mock import patch + +module = load_module() +patch.object(module.time, "sleep") # error: [shared-clock-mock] + +module.time = SimpleNamespace(sleep=fake) +patch.object(module.time, "sleep") # ruff: ignore[shared-clock-mock] +``` + +A clock stored directly on an instance or class is usually part of its own +state. A module stored on an instance can still expose the shared clock. + +```py +from unittest.mock import patch + +class Worker: + def set_clock(self): + self.time.sleep = fake + patch.object(self.time, "monotonic") + patch.object(self.module.time, "sleep") # error: [shared-clock-mock] + + @classmethod + def reset_clock(cls): + del cls.asyncio.sleep +``` diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 76f2ff7e37..7817b3d5fe 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -399,6 +399,11 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { } } Expr::Attribute(attribute) => { + if checker.is_rule_enabled(Rule::SharedClockMock) + && matches!(attribute.ctx, ExprContext::Store | ExprContext::Del) + { + ruff::rules::shared_clock_attribute(checker, attribute); + } if attribute.ctx == ExprContext::Load { if checker.any_rule_enabled(&[ Rule::SuspiciousPickleUsage, @@ -1026,6 +1031,9 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { if checker.is_rule_enabled(Rule::PytestPatchWithLambda) { flake8_pytest_style::rules::patch_with_lambda(checker, call); } + if checker.is_rule_enabled(Rule::SharedClockMock) { + ruff::rules::shared_clock_mock(checker, call); + } if checker.any_rule_enabled(&[ Rule::PytestParametrizeNamesWrongType, Rule::PytestParametrizeValuesWrongType, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 2cc3ac036c..ee5fb8ede0 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1371,6 +1371,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleStatus, Rule)> { // Rules that belong only to categories, without linter groups or codes. () => rules::ruff::rules::PytestFixtureAutouse, + () => rules::ruff::rules::SharedClockMock, () => rules::ruff::rules::UnnecessaryRegularExpressionCompile, _ => return None, diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index b19ece18fc..256b343838 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -53,6 +53,7 @@ pub(crate) use redirected_noqa::*; pub(crate) use redundant_bool_literal::*; pub(crate) use rule_codes_in_selectors::*; pub(crate) use rule_codes_in_suppression_comments::*; +pub(crate) use shared_clock_mock::*; pub(crate) use sort_dunder_all::*; pub(crate) use sort_dunder_slots::*; pub(crate) use starmap_zip::*; @@ -137,6 +138,7 @@ mod redundant_bool_literal; mod rule_codes_in_selectors; mod rule_codes_in_suppression_comments; mod sequence_sorting; +mod shared_clock_mock; mod sort_dunder_all; mod sort_dunder_slots; mod starmap_zip; diff --git a/crates/ruff_linter/src/rules/ruff/rules/shared_clock_mock.rs b/crates/ruff_linter/src/rules/ruff/rules/shared_clock_mock.rs new file mode 100644 index 0000000000..bebee850ca --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/shared_clock_mock.rs @@ -0,0 +1,224 @@ +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast::name::UnqualifiedName; +use ruff_python_ast::{Expr, ExprAttribute, ExprCall, Stmt}; +use ruff_python_semantic::analyze::typing; +use ruff_text_size::{Ranged, TextRange}; + +use crate::Violation; +use crate::checkers::ast::Checker; +use crate::codes::Category; + +/// ## What it does +/// Checks for patches, assignments, and deletions of shared clock and sleep +/// functions in `time` and `asyncio`. +/// +/// ## Why is this bad? +/// If `worker` uses `import time`, then `worker.time` and `time` refer to +/// the same module. Patching `worker.time.sleep` can make an +/// `assert_called_once()` check fail when another thread calls `time.sleep`. +/// The same can happen when another task calls a patched `asyncio.sleep`. +/// +/// To keep the patch local to `worker`, replace `worker.time` or +/// `worker.asyncio` with an object that provides the functions `worker` uses. +/// +/// ## Example +/// ```python +/// from unittest.mock import Mock, patch +/// +/// from example import worker +/// +/// sleep = Mock() +/// with patch.object(worker.time, "sleep", sleep): +/// worker.retry() +/// ``` +/// +/// Use instead: +/// ```python +/// from types import SimpleNamespace +/// from unittest.mock import Mock, patch +/// +/// from example import worker +/// +/// sleep = Mock() +/// with patch.object(worker, "time", SimpleNamespace(sleep=sleep)): +/// worker.retry() +/// ``` +/// +/// ## Known problems +/// Ruff can flag a patch to `worker.time.sleep` even if a fixture has +/// replaced `worker.time` with a private clock. It can miss a patch when +/// the target is a dynamically built string or when the test assigns +/// `clock = worker.time` and then patches `clock.sleep`. +/// +/// ## References +/// - [Python documentation: Where to patch](https://docs.python.org/3/library/unittest.mock.html#where-to-patch) +/// - [pytest documentation: Monkeypatching](https://docs.pytest.org/en/stable/how-to/monkeypatch.html) +#[derive(ViolationMetadata)] +#[violation_metadata(preview_since = "NEXT_RUFF_VERSION", category = Category::Suspicious)] +pub(crate) struct SharedClockMock { + function: String, +} + +impl Violation for SharedClockMock { + #[derive_message_formats] + fn message(&self) -> String { + let Self { function } = self; + format!("Changing `{function}` may affect unrelated threads or tasks") + } +} + +#[derive(Clone, Copy)] +enum PatchApi { + String, + Object, + MonkeyPatch, +} + +impl PatchApi { + /// Recognize imported patch functions before falling back to fixture names. + fn from_call(checker: &Checker, call: &ExprCall) -> Option { + if let Some(name) = checker + .semantic() + .resolve_qualified_name(&call.func) + .or_else(|| typing::resolve_assignment(&call.func, checker.semantic())) + { + return match name.segments() { + ["unittest", "mock", "patch"] | ["mock", "patch"] => Some(Self::String), + ["unittest", "mock", "patch", "object"] | ["mock", "patch", "object"] => { + Some(Self::Object) + } + ["pytest", "MonkeyPatch", "setattr" | "delattr"] + | [ + "_pytest", + "monkeypatch", + "MonkeyPatch", + "setattr" | "delattr", + ] => Some(Self::MonkeyPatch), + _ => None, + }; + } + + let name = UnqualifiedName::from_expr(&call.func)?; + match name.segments() { + ["monkeypatch", "setattr" | "delattr"] => Some(Self::MonkeyPatch), + [ + "mocker" | "class_mocker" | "module_mocker" | "package_mocker" | "session_mocker", + "patch", + ] => Some(Self::String), + [ + "mocker" | "class_mocker" | "module_mocker" | "package_mocker" | "session_mocker", + "patch", + "object", + ] => Some(Self::Object), + _ => None, + } + } +} + +/// `shared-clock-mock` +pub(crate) fn shared_clock_mock(checker: &Checker, call: &ExprCall) { + let Some(api) = PatchApi::from_call(checker, call) else { + return; + }; + let Some(target) = call.arguments.find_argument_value("target", 0) else { + return; + }; + + let (module, member, range) = match api { + PatchApi::String | PatchApi::MonkeyPatch + if let Some(target) = target.as_string_literal_expr() => + { + let Some((module, member)) = target.value.to_str().rsplit_once('.') else { + return; + }; + (module.rsplit('.').next(), member, target.range()) + } + PatchApi::Object | PatchApi::MonkeyPatch => { + let attribute = match api { + PatchApi::MonkeyPatch => "name", + _ => "attribute", + }; + let Some(member) = call + .arguments + .find_argument_value(attribute, 1) + .and_then(Expr::as_string_literal_expr) + else { + return; + }; + ( + clock_module(checker, target), + member.value.to_str(), + target.range().cover(member.range()), + ) + } + PatchApi::String => return, + }; + + report_shared_clock(checker, module, member, range); +} + +/// `shared-clock-mock` +pub(crate) fn shared_clock_attribute(checker: &Checker, attribute: &ExprAttribute) { + // An annotation without a value does not write to the attribute. + if let Stmt::AnnAssign(assignment) = checker.semantic().current_statement() + && assignment.value.is_none() + && assignment.target.range() == attribute.range() + { + return; + } + report_shared_clock( + checker, + clock_module(checker, &attribute.value), + attribute.attr.as_str(), + attribute.range(), + ); +} + +fn report_shared_clock(checker: &Checker, module: Option<&str>, member: &str, range: TextRange) { + if let Some(module) = module + && matches!( + (module, member), + ( + "time", + "sleep" + | "time" + | "time_ns" + | "monotonic" + | "monotonic_ns" + | "perf_counter" + | "perf_counter_ns" + ) | ("asyncio", "sleep") + ) + { + let mut diagnostic = checker.report_diagnostic( + SharedClockMock { + function: format!("{module}.{member}"), + }, + range, + ); + diagnostic.help("Replace the binding in the consuming module with a private clock"); + } +} + +/// Resolve direct imports and use the final attribute as a heuristic for consumers. +fn clock_module<'a>(checker: &Checker, target: &'a Expr) -> Option<&'a str> { + if let Some(name) = checker.semantic().resolve_qualified_name(target) { + match name.segments() { + ["time"] => return Some("time"), + ["asyncio"] => return Some("asyncio"), + _ => {} + } + } + + let attribute = target.as_attribute_expr()?; + if let Some(receiver) = attribute.value.as_name_expr() + && matches!(receiver.id.as_str(), "self" | "cls") + && checker + .semantic() + .resolve_name(receiver) + .is_some_and(|id| checker.semantic().binding(id).kind.is_argument()) + { + return None; + } + Some(attribute.attr.as_str()) +} diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index 1c379ba985..29ccd53c61 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -1059,6 +1059,7 @@ mod tests { invalid-suppression-comment (RUF103), unmatched-suppression-comment (RUF104), rule-codes-in-suppression-comments (RUF106), + shared-clock-mock, unnecessary-regular-expression-compile, ] diff --git a/ruff.schema.json b/ruff.schema.json index 63757b595e..5c30e48791 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -5310,6 +5310,7 @@ "self-or-cls-assignment", "set-attr-with-constant", "shallow-copy-environ", + "shared-clock-mock", "shebang-leading-whitespace", "shebang-missing-executable-file", "shebang-missing-python",