Skip to content

Fix mod_module_files false positive for tests in workspaces #16048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 10, 2025
+8 −2
Merged

Fix mod_module_files false positive for tests in workspaces #16048

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions clippy_lints/src/module_style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_lint::{EarlyContext, EarlyLintPass, Level, LintContext};
use rustc_session::impl_lint_pass;
use rustc_span::def_id::LOCAL_CRATE;
use rustc_span::{FileName, SourceFile, Span, SyntaxContext, sym};
use std::path::{Path, PathBuf};
use std::path::{Component, Path, PathBuf};
use std::sync::Arc;

declare_clippy_lint! {
Expand Down Expand Up @@ -150,7 +150,13 @@ fn check_self_named_module(cx: &EarlyContext<'_>, path: &Path, file: &SourceFile
/// Using `mod.rs` in integration tests is a [common pattern](https://doc.rust-lang.org/book/ch11-03-test-organization.html#submodules-in-integration-test)
/// for code-sharing between tests.
fn check_mod_module(cx: &EarlyContext<'_>, path: &Path, file: &SourceFile) {
if path.ends_with("mod.rs") && !path.starts_with("tests") {
if path.ends_with("mod.rs")
&& !path
.components()
.filter_map(|c| if let Component::Normal(d) = c { Some(d) } else { None })
.take_while(|&c| c != "src")
.any(|c| c == "tests")
Comment on lines +157 to +158
Copy link

@miguelpduarte miguelpduarte Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm misreading this, it seems like it will work for some-path/nested/crates/my-crate/src/tests/ but not for some-path/crates/my-crate/tests/. (I read this as "find the first src directory and then check that it doesn't have tests in any directory under it")

Copy link
Contributor Author

@TomFryersMidsummer TomFryersMidsummer Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be what it would be if I’d used skip_while. My intention here, with take_while, is to ‘find if there’s a tests directory, excluding if it’s under a src directory’.

I think just .any(|c| c == "tests"), without the take_while, would be good enough for almost all cases, but excluding the contents of src seemed like a good way to eliminate a few false positives (for the check, leading to false negatives for the lint).

Copy link

@miguelpduarte miguelpduarte Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally correct - I should have double-checked in the docs, it's been a while since I used take_while and skip_while so I got them mixed up 🤦 Sorry for the confusion!

{
span_lint_and_then(
cx,
MOD_MODULE_FILES,
Expand Down