about summary refs log tree commit diff
diff options
context:
space:
mode:
authorKevin Reid <kpreid@switchb.org>2024-09-23 11:58:28 -0700
committerKevin Reid <kpreid@switchb.org>2024-09-23 12:25:01 -0700
commita235cbd8ac347f323084c99d198f96d27269ec85 (patch)
treec01e80c5d5b89de9ef9e799fb08d88749fa0d04f
parentb39323bf255fa79b20f75b7650d64b91d2e0517e (diff)
downloadrust-a235cbd8ac347f323084c99d198f96d27269ec85.tar.gz
rust-a235cbd8ac347f323084c99d198f96d27269ec85.zip
`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.
-rw-r--r--clippy_lints/src/item_name_repetitions.rs33
-rw-r--r--clippy_lints/src/literal_representation.rs1
-rw-r--r--clippy_lints/src/macro_use.rs1
-rw-r--r--clippy_lints/src/vec.rs1
-rw-r--r--tests/ui-toml/item_name_repetitions/allowed_prefixes/item_name_repetitions.rs2
-rw-r--r--tests/ui-toml/item_name_repetitions/allowed_prefixes_extend/item_name_repetitions.rs2
-rw-r--r--tests/ui/module_name_repetitions.rs18
-rw-r--r--tests/ui/module_name_repetitions.stderr8
8 files changed, 53 insertions, 13 deletions
diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs
index 66a8a3167a4..511317c8b43 100644
--- a/clippy_lints/src/item_name_repetitions.rs
+++ b/clippy_lints/src/item_name_repetitions.rs
@@ -50,11 +50,28 @@ declare_clippy_lint! {
 
 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 @@ impl LateLintPass<'_> for ItemNameRepetitions {
         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 @@ impl LateLintPass<'_> for ItemNameRepetitions {
                             "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();
diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs
index 81f2a03fb55..e2dcb20f906 100644
--- a/clippy_lints/src/literal_representation.rs
+++ b/clippy_lints/src/literal_representation.rs
@@ -412,7 +412,6 @@ impl LiteralDigitGrouping {
     }
 }
 
-#[expect(clippy::module_name_repetitions)]
 pub struct DecimalLiteralRepresentation {
     threshold: u64,
 }
diff --git a/clippy_lints/src/macro_use.rs b/clippy_lints/src/macro_use.rs
index bd6b3f1a47b..d5d6226f0aa 100644
--- a/clippy_lints/src/macro_use.rs
+++ b/clippy_lints/src/macro_use.rs
@@ -43,7 +43,6 @@ impl MacroRefData {
 }
 
 #[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.
diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs
index ce4f41e854d..0fdef51d6fc 100644
--- a/clippy_lints/src/vec.rs
+++ b/clippy_lints/src/vec.rs
@@ -17,7 +17,6 @@ use rustc_middle::ty::layout::LayoutOf;
 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,
diff --git a/tests/ui-toml/item_name_repetitions/allowed_prefixes/item_name_repetitions.rs b/tests/ui-toml/item_name_repetitions/allowed_prefixes/item_name_repetitions.rs
index 4142ced5f6b..2ae673a6def 100644
--- a/tests/ui-toml/item_name_repetitions/allowed_prefixes/item_name_repetitions.rs
+++ b/tests/ui-toml/item_name_repetitions/allowed_prefixes/item_name_repetitions.rs
@@ -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"].
 
diff --git a/tests/ui-toml/item_name_repetitions/allowed_prefixes_extend/item_name_repetitions.rs b/tests/ui-toml/item_name_repetitions/allowed_prefixes_extend/item_name_repetitions.rs
index b132305d01c..dbd61992c0d 100644
--- a/tests/ui-toml/item_name_repetitions/allowed_prefixes_extend/item_name_repetitions.rs
+++ b/tests/ui-toml/item_name_repetitions/allowed_prefixes_extend/item_name_repetitions.rs
@@ -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"].
 
diff --git a/tests/ui/module_name_repetitions.rs b/tests/ui/module_name_repetitions.rs
index b75ef87ab36..71d8ac7a1f0 100644
--- a/tests/ui/module_name_repetitions.rs
+++ b/tests/ui/module_name_repetitions.rs
@@ -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 @@ mod foo {
     // 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() {}
diff --git a/tests/ui/module_name_repetitions.stderr b/tests/ui/module_name_repetitions.stderr
index bffb08f6f87..8fd8b394875 100644
--- a/tests/ui/module_name_repetitions.stderr
+++ b/tests/ui/module_name_repetitions.stderr
@@ -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