Skip to content

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

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.

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
Oct 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 27 additions & 6 deletions clippy_lints/src/item_name_repetitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the inclusion of this Known issues section

///
/// ```no_run
/// pub mod foo {
/// mod iteration {
/// pub struct FooIter {}
/// }
/// pub use iteration::*; // creates the path `foo::FooIter`
/// }
/// ```
///
/// ### Example
/// ```no_run
Expand Down Expand Up @@ -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,
Expand All @@ -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();
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/literal_representation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,6 @@ impl LiteralDigitGrouping {
}
}

#[expect(clippy::module_name_repetitions)]
pub struct DecimalLiteralRepresentation {
threshold: u64,
}
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/macro_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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"].

Expand Down
Original file line number Diff line number Diff line change
@@ -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"].

Expand Down
18 changes: 17 additions & 1 deletion tests/ui/module_name_repetitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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() {}
Expand Down
8 changes: 7 additions & 1 deletion tests/ui/module_name_repetitions.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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