new module style lint: inline_modules (#16732)

*[View all
comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust-clippy/pull/16732)*

fixes: #15966

add new module style restriction lint that checks for use of inline
modules, with an exception for test module.

changelog: new lint: [`inline_modules`]

Author: ada4a

CHANGELOG.md

@@ -6804,6 +6804,7 @@ Released 2018-09-13
 [`inline_asm_x86_att_syntax`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_asm_x86_att_syntax
 [`inline_asm_x86_intel_syntax`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_asm_x86_intel_syntax
 [`inline_fn_without_body`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_fn_without_body
+[`inline_modules`]: https://rust-lang.github.io/rust-clippy/master/index.html#inline_modules
 [`inspect_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#inspect_for_each
 [`int_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#int_plus_one
 [`integer_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic

clippy_lints/src/declared_lints.rs

@@ -537,6 +537,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::missing_trait_methods::MISSING_TRAIT_METHODS_INFO,
     crate::mixed_read_write_in_expression::DIVERGING_SUB_EXPRESSION_INFO,
     crate::mixed_read_write_in_expression::MIXED_READ_WRITE_IN_EXPRESSION_INFO,
+    crate::module_style::INLINE_MODULES_INFO,
     crate::module_style::MOD_MODULE_FILES_INFO,
     crate::module_style::SELF_NAMED_MODULE_FILES_INFO,
     crate::multi_assignments::MULTI_ASSIGNMENTS_INFO,

clippy_lints/src/module_style.rs

@@ -1,12 +1,46 @@
+use clippy_utils::ast_utils::is_cfg_test;
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::snippet;
 use rustc_ast::ast::{self, Inline, ItemKind, ModKind};
 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 rustc_span::{FileName, Ident, SourceFile, Span, SyntaxContext, sym};
 use std::path::{Component, Path, PathBuf};
 use std::sync::Arc;
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks that module layout does not use inline modules.
+    /// Inline test modules (anything annotated with `#[cfg(test)]`) are not linted.
+    ///
+    /// ### Why restrict this?
+    /// Having multiple module layout styles in a project can be confusing.
+    ///
+    /// ### Known problems
+    /// The lint currently doesn't lint inline modules whose parent module is annotated
+    /// with the `#[path]` attribute.
+    ///
+    /// ### Example
+    /// ```ignore
+    /// // in `src/lib.rs`
+    /// mod foo {
+    ///    /* module contents */
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```ignore
+    /// // in `src/lib.rs`
+    /// mod foo;
+    /// // in `src/foo.rs` (or `src/foo/mod.rs`)
+    /// /* module contents */
+    /// ```
+    #[clippy::version = "1.96.0"]
+    pub INLINE_MODULES,
+    restriction,
+    "checks that module layout does not use inline modules"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks that module layout uses only self named module files; bans `mod.rs` files.
@@ -65,18 +99,36 @@ declare_clippy_lint! {
     "checks that module layout is consistent"
 }
 
-impl_lint_pass!(ModStyle => [MOD_MODULE_FILES, SELF_NAMED_MODULE_FILES]);
+impl_lint_pass!(ModStyle => [
+    INLINE_MODULES,
+    MOD_MODULE_FILES,
+    SELF_NAMED_MODULE_FILES,
+]);
 
 pub struct ModState {
+    mod_file: Arc<SourceFile>,
+    mod_ident: Ident,
+    path_from_working_dir: Option<PathBuf>,
     contains_external: bool,
     has_path_attr: bool,
-    mod_file: Arc<SourceFile>,
+    is_cfg_test: bool,
 }
 
 #[derive(Default)]
 pub struct ModStyle {
     working_dir: Option<PathBuf>,
-    module_stack: Vec<ModState>,
+    regular_mod_stack: Vec<ModState>,
+    inline_mod_stack: Vec<ModState>,
+}
+
+impl ModStyle {
+    fn inside_cfg_test_inline_mod(&self) -> bool {
+        self.inline_mod_stack.last().is_some_and(|last| last.is_cfg_test)
+    }
+
+    fn get_relative_path_from_working_dir(&self, file: &SourceFile) -> Option<PathBuf> {
+        try_trim_file_path_prefix(file, self.working_dir.as_ref()?).map(Path::to_path_buf)
+    }
 }
 
 impl EarlyLintPass for ModStyle {
@@ -87,45 +139,83 @@ impl EarlyLintPass for ModStyle {
     fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
         if cx.builder.lint_level(MOD_MODULE_FILES).level == Level::Allow
             && cx.builder.lint_level(SELF_NAMED_MODULE_FILES).level == Level::Allow
+            && cx.builder.lint_level(INLINE_MODULES).level == Level::Allow
         {
             return;
         }
-        if let ItemKind::Mod(.., ModKind::Loaded(_, Inline::No { .. }, mod_spans, ..)) = &item.kind {
+        if let ItemKind::Mod(_, mod_ident, ModKind::Loaded(_, inline_or_not, mod_spans, ..)) = &item.kind {
             let has_path_attr = item.attrs.iter().any(|attr| attr.has_name(sym::path));
-            if !has_path_attr && let Some(current) = self.module_stack.last_mut() {
-                current.contains_external = true;
-            }
             let mod_file = cx.sess().source_map().lookup_source_file(mod_spans.inner_span.lo());
-            self.module_stack.push(ModState {
+            let path_from_working_dir = self.get_relative_path_from_working_dir(&mod_file);
+            let current = ModState {
+                mod_file,
+                mod_ident: *mod_ident,
+                path_from_working_dir,
                 contains_external: false,
                 has_path_attr,
-                mod_file,
-            });
+                is_cfg_test: self.inside_cfg_test_inline_mod() || is_cfg_test(item),
+            };
+            match inline_or_not {
+                Inline::Yes => {
+                    if !current.is_cfg_test
+                        && !item.span.from_expansion()
+                        && self.regular_mod_stack.last().is_none_or(|last| !last.has_path_attr)
+                        && let Some(path) = &current.path_from_working_dir
+                    {
+                        let opt_extra_mod_dir = self.regular_mod_stack.last().and_then(|last| {
+                            if last.path_from_working_dir.as_ref()?.ends_with("mod.rs") {
+                                None
+                            } else {
+                                Some(&last.mod_ident)
+                            }
+                        });
+                        check_inline_module(
+                            cx,
+                            path,
+                            *mod_ident,
+                            item.span,
+                            opt_extra_mod_dir
+                                .into_iter()
+                                .chain(self.inline_mod_stack.iter().map(|state| &state.mod_ident)),
+                        );
+                    }
+                    self.inline_mod_stack.push(current);
+                },
+                Inline::No { .. } => {
+                    if !has_path_attr && let Some(last) = self.regular_mod_stack.last_mut() {
+                        last.contains_external = true;
+                    }
+                    self.regular_mod_stack.push(current);
+                },
+            }
         }
     }
 
     fn check_item_post(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
         if cx.builder.lint_level(MOD_MODULE_FILES).level == Level::Allow
             && cx.builder.lint_level(SELF_NAMED_MODULE_FILES).level == Level::Allow
+            && cx.builder.lint_level(INLINE_MODULES).level == Level::Allow
         {
             return;
         }
 
-        if let ItemKind::Mod(.., ModKind::Loaded(_, Inline::No { .. }, ..)) = &item.kind
-            && let Some(current) = self.module_stack.pop()
-            && !current.has_path_attr
-        {
-            let Some(path) = self
-                .working_dir
-                .as_ref()
-                .and_then(|src| try_trim_file_path_prefix(&current.mod_file, src))
-            else {
-                return;
-            };
-            if current.contains_external {
-                check_self_named_module(cx, path, &current.mod_file);
+        if let ItemKind::Mod(.., ModKind::Loaded(_, inline_or_not, ..)) = &item.kind {
+            match inline_or_not {
+                Inline::Yes => {
+                    self.inline_mod_stack.pop();
+                },
+                Inline::No { .. } => {
+                    if let Some(current) = self.regular_mod_stack.pop()
+                        && let Some(path) = &current.path_from_working_dir
+                        && !current.has_path_attr
+                    {
+                        if current.contains_external {
+                            check_self_named_module(cx, path, &current.mod_file);
+                        }
+                        check_mod_module(cx, path, &current.mod_file);
+                    }
+                },
             }
-            check_mod_module(cx, path, &current.mod_file);
         }
     }
 }
@@ -173,6 +263,31 @@ fn check_mod_module(cx: &EarlyContext<'_>, path: &Path, file: &SourceFile) {
     }
 }
 
+fn check_inline_module<'a>(
+    cx: &EarlyContext<'_>,
+    path: &Path,
+    mod_ident: Ident,
+    mod_span: Span,
+    ancestor_mods: impl Iterator<Item = &'a Ident>,
+) {
+    let Some(parent) = path.parent() else { return };
+
+    span_lint_and_then(cx, INLINE_MODULES, mod_span, "inline module found", |diag| {
+        let mut mod_folder = parent.to_path_buf();
+        mod_folder.extend(ancestor_mods.map(Ident::as_str));
+        let mod_name = mod_ident.as_str();
+
+        let mod_file = mod_folder.join(mod_name).join("mod.rs");
+        let self_named_mod_file = mod_folder.join(format!("{mod_name}.rs"));
+        let outlined_mod = snippet(cx, mod_span.with_hi(mod_ident.span.hi()), "");
+        diag.help(format!(
+            "move the contents of the module to `{}` or `{}`, and replace this with `{outlined_mod};`",
+            self_named_mod_file.display(),
+            mod_file.display(),
+        ));
+    });
+}
+
 fn try_trim_file_path_prefix<'a>(file: &'a SourceFile, prefix: &'a Path) -> Option<&'a Path> {
     if let FileName::Real(name) = &file.name
         && let Some(mut path) = name.local_path()

clippy_utils/src/ast_utils/mod.rs

@@ -5,7 +5,8 @@
 #![allow(clippy::wildcard_imports, clippy::enum_glob_use)]
 
 use crate::{both, over};
-use rustc_ast::{self as ast, *};
+use rustc_ast::{self as ast, HasAttrs, *};
+use rustc_span::sym;
 use rustc_span::symbol::Ident;
 use std::mem;
 
@@ -1044,3 +1045,17 @@ pub fn eq_delim_args(l: &DelimArgs, r: &DelimArgs) -> bool {
         && l.tokens.len() == r.tokens.len()
         && l.tokens.iter().zip(r.tokens.iter()).all(|(a, b)| a.eq_unspanned(b))
 }
+
+/// Checks whether `#[cfg(test)]` is directly applied to `item`.
+pub fn is_cfg_test(item: &impl HasAttrs) -> bool {
+    item.attrs().iter().any(|attr| {
+        if attr.has_name(sym::cfg)
+            && let Some(item_list) = attr.meta_item_list()
+            && item_list.iter().any(|item| item.has_name(sym::test))
+        {
+            true
+        } else {
+            false
+        }
+    })
+}

tests/ui-cargo/module_style/inline_mod/Cargo.stderr

@@ -0,0 +1,71 @@
+error: inline module found
+ --> src/other.rs:1:1
+  |
+1 | mod foo {}
+  | ^^^^^^^^^^
+  |
+  = help: move the contents of the module to `src/other/foo.rs` or `src/other/foo/mod.rs`, and replace this with `mod foo;`
+  = note: `-D clippy::inline-modules` implied by `-D warnings`
+  = help: to override `-D warnings` add `#[allow(clippy::inline_modules)]`
+
+error: inline module found
+ --> src/qux/foo.rs:1:1
+  |
+1 | mod bar {}
+  | ^^^^^^^^^^
+  |
+  = help: move the contents of the module to `src/qux/foo/bar.rs` or `src/qux/foo/bar/mod.rs`, and replace this with `mod bar;`
+
+error: inline module found
+  --> src/lib.rs:9:1
+   |
+ 9 | / pub mod test_nested_inline_mods {
+10 | |     mod bar {
+11 | |         mod baz {}
+12 | |     }
+13 | | }
+   | |_^
+   |
+   = help: move the contents of the module to `src/test_nested_inline_mods.rs` or `src/test_nested_inline_mods/mod.rs`, and replace this with `pub mod test_nested_inline_mods;`
+
+error: inline module found
+  --> src/lib.rs:10:5
+   |
+10 | /     mod bar {
+11 | |         mod baz {}
+12 | |     }
+   | |_____^
+   |
+   = help: move the contents of the module to `src/test_nested_inline_mods/bar.rs` or `src/test_nested_inline_mods/bar/mod.rs`, and replace this with `mod bar;`
+
+error: inline module found
+  --> src/lib.rs:11:9
+   |
+11 |         mod baz {}
+   |         ^^^^^^^^^^
+   |
+   = help: move the contents of the module to `src/test_nested_inline_mods/bar/baz.rs` or `src/test_nested_inline_mods/bar/baz/mod.rs`, and replace this with `mod baz;`
+
+error: inline module found
+  --> src/lib.rs:20:1
+   |
+20 | / mod partially_escaped_test_mod {
+21 | |     #[cfg(test)]
+22 | |     mod tests {
+23 | |         mod bar {}
+24 | |     }
+25 | |     mod baz {}
+26 | | }
+   | |_^
+   |
+   = help: move the contents of the module to `src/partially_escaped_test_mod.rs` or `src/partially_escaped_test_mod/mod.rs`, and replace this with `mod partially_escaped_test_mod;`
+
+error: inline module found
+  --> src/lib.rs:25:5
+   |
+25 |     mod baz {}
+   |     ^^^^^^^^^^
+   |
+   = help: move the contents of the module to `src/partially_escaped_test_mod/baz.rs` or `src/partially_escaped_test_mod/baz/mod.rs`, and replace this with `mod baz;`
+
+error: could not compile `inline-mod` (lib) due to 7 previous errors

tests/ui-cargo/module_style/inline_mod/Cargo.toml

@@ -0,0 +1,9 @@
+[package]
+name = "inline-mod"
+version = "0.1.0"
+edition = "2024"
+publish = false
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]

tests/ui-cargo/module_style/inline_mod/src/foo.rs

@@ -0,0 +1,2 @@
+/// Lint shouldn't fire because parent mod has a path attribute.
+mod foo {}

tests/ui-cargo/module_style/inline_mod/src/lib.rs

@@ -0,0 +1,34 @@
+#![warn(clippy::inline_modules)]
+
+pub mod other;
+#[path = "qux/mod.rs"]
+pub mod something;
+#[path = "foo.rs"]
+pub mod stuff;
+
+pub mod test_nested_inline_mods {
+    mod bar {
+        mod baz {}
+    }
+}
+
+#[cfg(test)]
+mod escaped_test_mod {
+    mod bar {}
+}
+
+mod partially_escaped_test_mod {
+    #[cfg(test)]
+    mod tests {
+        mod bar {}
+    }
+    mod baz {}
+}
+
+macro_rules! inline_mod_from_expansion {
+    () => {
+        mod _foo {}
+    };
+}
+
+inline_mod_from_expansion!();

tests/ui-cargo/module_style/inline_mod/src/other.rs

@@ -0,0 +1 @@
+mod foo {}

tests/ui-cargo/module_style/inline_mod/src/qux/foo.rs

@@ -0,0 +1 @@
+mod bar {}