module_name_repetitions: don't warn if the item is in a private module.

Fixes <https://github.com/rust-lang/rust-clippy/issues/8524>.

There is still a warning (as there should be) if the item is reexported
by name, but not by glob; that would require further work to examine the
names in the glob, and I haven't looked into that.

Credit to @Centri3 for suggesting approximately this simple fix in
<https://github.com/rust-lang/rust-clippy/issues/8524#issuecomment-1729036149>.
However, per later comment <https://github.com/rust-lang/rust-clippy/issues/8524#issuecomment-2035836495>,
I am not making it configuration-dependent, but *always* checking public
items in public modules only.
This commit is contained in:
Kevin Reid 2024-09-23 11:58:28 -07:00
parent b39323bf25
commit a235cbd8ac
8 changed files with 53 additions and 13 deletions

View File

@ -50,11 +50,28 @@
declare_clippy_lint! {
/// ### What it does
/// Detects type names that are prefixed or suffixed by the
/// containing module's name.
/// Detects public item names that are prefixed or suffixed by the
/// containing public module's name.
///
/// ### Why is this bad?
/// It requires the user to type the module name twice.
/// It requires the user to type the module name twice in each usage,
/// especially if they choose to import the module rather than its contents.
///
/// Lack of such repetition is also the style used in the Rust standard library;
/// e.g. `io::Error` and `fmt::Error` rather than `io::IoError` and `fmt::FmtError`;
/// and `array::from_ref` rather than `array::array_from_ref`.
///
/// ### Known issues
/// Glob re-exports are ignored; e.g. this will not warn even though it should:
///
/// ```no_run
/// pub mod foo {
/// mod iteration {
/// pub struct FooIter {}
/// }
/// pub use iteration::*; // creates the path `foo::FooIter`
/// }
/// ```
///
/// ### Example
/// ```no_run
@ -389,12 +406,12 @@ fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
let item_name = item.ident.name.as_str();
let item_camel = to_camel_case(item_name);
if !item.span.from_expansion() && is_present_in_source(cx, item.span) {
if let [.., (mod_name, mod_camel, owner_id)] = &*self.modules {
if let [.., (mod_name, mod_camel, mod_owner_id)] = &*self.modules {
// constants don't have surrounding modules
if !mod_camel.is_empty() {
if mod_name == &item.ident.name
&& let ItemKind::Mod(..) = item.kind
&& (!self.allow_private_module_inception || cx.tcx.visibility(owner_id.def_id).is_public())
&& (!self.allow_private_module_inception || cx.tcx.visibility(mod_owner_id.def_id).is_public())
{
span_lint(
cx,
@ -403,9 +420,13 @@ fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
"module has the same name as its containing module",
);
}
// The `module_name_repetitions` lint should only trigger if the item has the module in its
// name. Having the same name is accepted.
if cx.tcx.visibility(item.owner_id).is_public() && item_camel.len() > mod_camel.len() {
if cx.tcx.visibility(item.owner_id).is_public()
&& cx.tcx.visibility(mod_owner_id.def_id).is_public()
&& item_camel.len() > mod_camel.len()
{
let matching = count_match_start(mod_camel, &item_camel);
let rmatching = count_match_end(mod_camel, &item_camel);
let nchars = mod_camel.chars().count();

View File

@ -412,7 +412,6 @@ fn get_group_size<'a>(
}
}
#[expect(clippy::module_name_repetitions)]
pub struct DecimalLiteralRepresentation {
threshold: u64,
}

View File

@ -43,7 +43,6 @@ pub fn new(name: String) -> Self {
}
#[derive(Default)]
#[expect(clippy::module_name_repetitions)]
pub struct MacroUseImports {
/// the actual import path used and the span of the attribute above it. The value is
/// the location, where the lint should be emitted.

View File

@ -17,7 +17,6 @@
use rustc_session::impl_lint_pass;
use rustc_span::{DesugaringKind, Span, sym};
#[expect(clippy::module_name_repetitions)]
pub struct UselessVec {
too_large_for_stack: u64,
msrv: Msrv,

View File

@ -1,7 +1,7 @@
#![warn(clippy::module_name_repetitions)]
#![allow(dead_code)]
mod foo {
pub mod foo {
// #12544 - shouldn't warn if item name consists only of an allowed prefix and a module name.
// In this test, allowed prefixes are configured to be ["bar"].

View File

@ -1,7 +1,7 @@
#![warn(clippy::module_name_repetitions)]
#![allow(dead_code)]
mod foo {
pub mod foo {
// #12544 - shouldn't warn if item name consists only of an allowed prefix and a module name.
// In this test, allowed prefixes are configured to be all of the default prefixes and ["bar"].

View File

@ -3,7 +3,7 @@
#![warn(clippy::module_name_repetitions)]
#![allow(dead_code)]
mod foo {
pub mod foo {
pub fn foo() {}
pub fn foo_bar() {}
//~^ ERROR: item name starts with its containing module's name
@ -20,6 +20,22 @@ pub enum CakeFoo {}
// Should not warn
pub struct Foobar;
// #8524 - shouldn't warn when item is declared in a private module...
mod error {
pub struct Error;
pub struct FooError;
}
pub use error::Error;
// ... but should still warn when the item is reexported to create a *public* path with repetition.
pub use error::FooError;
//~^ ERROR: item name starts with its containing module's name
// FIXME: This should also warn because it creates the public path `foo::FooIter`.
mod iter {
pub struct FooIter;
}
pub use iter::*;
// #12544 - shouldn't warn if item name consists only of an allowed prefix and a module name.
pub fn to_foo() {}
pub fn into_foo() {}

View File

@ -31,5 +31,11 @@ error: item name starts with its containing module's name
LL | pub struct Foo7Bar;
| ^^^^^^^
error: aborting due to 5 previous errors
error: item name starts with its containing module's name
--> tests/ui/module_name_repetitions.rs:30:20
|
LL | pub use error::FooError;
| ^^^^^^^^
error: aborting due to 6 previous errors