New lint: definition_in_module_root#16965
Conversation
|
rustbot has assigned @samueltardieu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Lintcheck changes for 810fe1d
This comment will be updated if you push new changes |
abca430 to
fa6e565
Compare
Flags definitions (struct, enum, fn, impl, trait, etc.) in `mod.rs` files. Encourages putting each definition in its own named file so filenames are descriptive and unique. `lib.rs` and `main.rs` are not checked, since defining a small crate's API there is common and reasonable. Filenames are resolved through the source map so `#[path]` is handled by the real filename. `#[macro_export]` macros and inline modules are exempt. Items produced by macro expansion are skipped. Projects opting into this lint will typically also `allow(module_inception)`, since enforcing declarations-only in `mod.rs` makes `foo/foo.rs` the expected layout. Category: `restriction`. changelog: new lint: [`definition_in_module_root`]
fa6e565 to
810fe1d
Compare
| ItemKind::Struct(..) => Some("struct"), | ||
| ItemKind::Enum(..) => Some("enum"), | ||
| ItemKind::Union(..) => Some("union"), | ||
| ItemKind::Fn(..) => Some("function"), | ||
| ItemKind::Const(..) => Some("const"), | ||
| ItemKind::Static(..) => Some("static"), | ||
| ItemKind::Impl(..) => Some("impl block"), | ||
| ItemKind::Trait(..) => Some("trait"), | ||
| ItemKind::TraitAlias(..) => Some("trait alias"), | ||
| ItemKind::TyAlias(..) => Some("type alias"), | ||
| ItemKind::ForeignMod(..) => Some("extern block"), | ||
| ItemKind::MacroDef(..) if !has_macro_export(item) => Some("macro"), | ||
| // Allowed: mod, use, extern crate, #[macro_export] macros, etc. | ||
| _ => None, |
There was a problem hiding this comment.
It is best to use the official terms, and to be exhaustive to make sure that newly added variants won't be forgotten:
| ItemKind::Struct(..) => Some("struct"), | |
| ItemKind::Enum(..) => Some("enum"), | |
| ItemKind::Union(..) => Some("union"), | |
| ItemKind::Fn(..) => Some("function"), | |
| ItemKind::Const(..) => Some("const"), | |
| ItemKind::Static(..) => Some("static"), | |
| ItemKind::Impl(..) => Some("impl block"), | |
| ItemKind::Trait(..) => Some("trait"), | |
| ItemKind::TraitAlias(..) => Some("trait alias"), | |
| ItemKind::TyAlias(..) => Some("type alias"), | |
| ItemKind::ForeignMod(..) => Some("extern block"), | |
| ItemKind::MacroDef(..) if !has_macro_export(item) => Some("macro"), | |
| // Allowed: mod, use, extern crate, #[macro_export] macros, etc. | |
| _ => None, | |
| i @ (ItemKind::Struct(..) | |
| | ItemKind::Enum(..) | |
| | ItemKind::Union(..) | |
| | ItemKind::Fn(..) | |
| | ItemKind::Const(..) | |
| | ItemKind::Static(..) | |
| | ItemKind::Impl(..) | |
| | ItemKind::Trait(..) | |
| | ItemKind::TraitAlias(..) | |
| | ItemKind::TyAlias(..) | |
| | ItemKind::ForeignMod(..)) => Some(i.descr()), | |
| i @ ItemKind::MacroDef(..) if !has_macro_export(item) => Some(i.descr()), | |
| ItemKind::ExternCrate(..) | |
| | ItemKind::Use(..) | |
| | ItemKind::ConstBlock(..) | |
| | ItemKind::Mod(..) | |
| | ItemKind::GlobalAsm(..) | |
| | ItemKind::MacCall(..) | |
| | ItemKind::MacroDef(..) | |
| | ItemKind::Delegation(..) | |
| | ItemKind::DelegationMac(..) => None, |
| let help = if let Some(ident) = item.kind.ident() { | ||
| format!("move {kind} `{ident}` to a dedicated file") | ||
| } else { | ||
| format!("move {kind} to a dedicated file") |
There was a problem hiding this comment.
| format!("move {kind} to a dedicated file") | |
| format!("move the {kind} to a dedicated file") |
| /// [`self_named_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_module_files | ||
| /// [`mod_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#mod_module_files | ||
| /// [`module_inception`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_inception | ||
| #[clippy::version = "1.96.0"] |
There was a problem hiding this comment.
Won't be in before Rust 1.97 at the soonest, if it gets in.
| #[clippy::version = "1.96.0"] | |
| #[clippy::version = "1.97.0"] |
| /// produces `foo/foo.rs`, which [`module_inception`] flags — projects | ||
| /// in that situation typically also `allow(module_inception)`. | ||
| /// | ||
| /// [`self_named_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_module_files |
There was a problem hiding this comment.
Please don't include those links as they may not refer to the same version (master vs. a specific version, or beta).
|
Reminder, once the PR becomes ready for a review, use |
|
This lint has been nominated for inclusion. |
|
☔ The latest upstream changes (possibly #16486) made this pull request unmergeable. Please resolve the merge conflicts. |
What this lint does
definition_in_module_rootis arestrictionlint that warns when amod.rsfile contains a definition (struct, enum, fn, impl, trait, type alias, etc.).
Items that introduce module structure —
mod,use,extern crate,#[macro_export]macros — are unaffected.lib.rsandmain.rsare notchecked.
The motivation is to keep
mod.rsfiles acting as a table of contents and topush every definition into a named file, so filenames stay descriptive and
unique across a project. This is the declaration / definition distinction
applied to Rust's module system: declarations introduce names, definitions
bind names to meanings, and
mod.rsis naturally suited to the former.How this fits with existing module-style lints
Clippy already has two lints that pick a side on
mod.rsversus self-namedfiles:
mod_module_files(the "nomod.rs" school) forbidsmod.rsfiles,requiring
foo.rsnext to afoo/directory of submodules.self_named_module_files(the "alwaysmod.rs" school) is theopposite: it requires
foo/mod.rsand forbids thefoo.rs-next-to-foo/layout.
definition_in_module_rootdoesn't pick a side; it complementsself_named_module_filesby adding a constraint on the contents ofmod.rs. Undermod_module_filesnomod.rsfiles exist, so this lint hasnothing to fire on (vacuously satisfied).
The interaction with
module_inceptionis scoped: it fires only when adefinition's name happens to match its parent module's name, in which case
moving the definition produces
foo/foo.rs, whichmodule_inceptionflags. For most definitions (where the name doesn't equal the parent
module's), the layout is
parent/named_file.rsandmodule_inceptiondoesn't fire. Within that scoped class of types, the two lints are
formally unsatisfiable together — the next section is the proof.
Canonical layouts
Mathematical motivation.
This lint's value is more than stylistic. Combined with the convention of
naming each file after its primary definition,
definition_in_module_rootcollapses the remaining degrees of freedom in Rust's module-to-file mapping
so that the file layout is uniquely determined by the module tree.
The freedom problem
Given N symbols to organize into files, most partitions retain
combinatorial ambiguity: (N choose K) > 1 for 1 < K < N. Of all groupings,
only two are unambiguous: all symbols in one file, or one file per symbol.
The rest involve arbitrary choices about which symbols share a file with
which. The one-per-symbol extreme is the only scalable choice that
preserves canonicality.
Even under one-per-symbol, a residual freedom remains: a definition can
live in its parent's
mod.rsor in a named sibling file. This lintremoves that last freedom — every definition goes in a named file. Once
both pins are in place, the filesystem tree is a direct projection of
the module tree: different authors converge on structurally identical
code, merges flatten, and automated refactoring becomes tractable.
Formal interaction with
module_inceptionThe interaction noted in the previous section is the formal expression of
this property. Consider:
For any type whose name matches its parent module's name, C1 ∧ C2 is
unsatisfiable:
foo/mod.rs.type — here,
foo.rs.foo/, that file isfoo/foo.rs.foo/foo.rs.From ¬(C1 ∧ C2) it follows that any project enforcing C1 must accept ¬C2
for the relevant class — i.e.,
allow(module_inception)for types whosename matches their parent module's name. The choice between dropping C1
or C2 is not symmetric: C1 is strictly stronger than C2, since
module_inception's premise (that definitions inmod.rsareacceptable) is exactly what C1 denies.
Therefore the canonical-layout property requires this lint. C1 is
the load-bearing constraint; without enforcement of "declarations-only
in
mod.rs," the whole property collapses to convention. The PRprovides C1;
allow(module_inception)for the matching-name class isthe cost of that choice; canonical layouts are the payoff.
Portability and the codegen gap
The structural role of
mod.rsis shared across module systems —Python's
__init__.py, TypeScript'sindex.ts, and Rust'smod.rsallserve as the parent-as-table-of-contents file. The canonicality property
described above is therefore portable: enforce "declarations only" in
those slots in any language, and the layout becomes uniquely determined.
This matters for code-generation pipelines that target multiple languages
— schema- or IDL-driven generators that emit corresponding implementations
from one source of truth. For canonical layouts to hold across the
generated code, each target language needs its own enforcement of the
rule. Until this PR, Rust's slot was empty. Landing this lint fills it.
Why
restrictionThis is a stylistic constraint that some teams want and others don't — the
classic
restrictionshape. Same tier asabsolute_paths,allow_attributes,and
self_named_module_files. Off by default; users opt in.Implementation
EarlyLintPass. AVec<bool>module stack tracks, per nesting level, whetheritems live in a
mod.rs.check_itemconsults the top of the stack;check_item_postpops. Filenames are resolved through the source map(
lookup_source_file), so#[path]is honoured by the real filename ratherthan the declaration syntax.
Exemptions:
#[macro_export]macros (Rust hoists these to crate root regardless ofenclosing file)
#[cfg(test)] mod tests { ... }) — children pushfalseonto the stack
item.span.from_expansion())Tests
Six
ui-cargotest directories undertests/ui-cargo/definition_in_module_root/:fail_modmod.rs;#[macro_export]exemptionfail_path_attr#[path = "custom/mod.rs"]— flagged via real filenamefail_edge_cases#[derive](only the user-written struct fires, not derived impls), genericimpl<T>,async fn,const fn, non-exportedmacro_rules!, trait with default body, sibling separate-filemod sub;(definitions insub.rsdon't fire), inline#[cfg(test)] mod tests,pub use, glob re-exports,extern crate,staticpass_lib_with_definitionslib.rsare not flaggedpass_binmain.rsare not flaggedpass_path_attr#[path = "custom/impl.rs"]— not flagged because the real filename isn'tmod.rsLintcheck
This is a
restrictionlint, so it's off by default. When opted into, it isexpected to produce many warnings on existing crates that organize code in
mod.rs— that is the lint's purpose. Flagging here so the lintcheck volumeisn't read as a false-positive signal.
Verification
cargo build --releaseclean (0 warnings)TESTNAME=definition_in_module_root cargo uitest— all 6 directories passcargo dev fmt --checkcleancargo dev update_lints --checkcleancargo dev blessproduces no diffChecklist
.stderrfiles)cargo testpasses locally (relevant subset)cargo dev update_lintscargo dev fmtchangelog: new lint: [
definition_in_module_root]