Skip to content
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

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

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Sep 23, 2024

Fixes #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 #8524 (comment). However, per later comment #8524 (comment), I am not making it configuration-dependent, but always checking public items in public modules only.

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

…ule.

Fixes <rust-lang#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
<rust-lang#8524 (comment)>.
However, per later comment <rust-lang#8524 (comment)>,
I am not making it configuration-dependent, but *always* checking public
items in public modules only.
@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2024

r? @Centri3

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 23, 2024
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️ 🌈 And sorry for the delay, I'm not sure why nobody picked this up before 😅

This may affect #13541

/// 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

@blyxyas
Copy link
Member

blyxyas commented Oct 12, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

📌 Commit a235cbd has been approved by blyxyas

It is now in the queue for this repository.

bors added a commit that referenced this pull request Oct 12, 2024
`module_name_repetitions`: don't warn if the item is in a private module.

Fixes <#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 <#8524 (comment)>. However, per later comment <#8524 (comment)>, I am not making it configuration-dependent, but *always* checking public items in public modules only.

changelog: [`module_name_repetitions`]: don't warn if the item is in a private module.
@blyxyas
Copy link
Member

blyxyas commented Oct 12, 2024

@bors r-

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

⌛ Testing commit a235cbd with merge 9eaf82b...

@blyxyas
Copy link
Member

blyxyas commented Oct 12, 2024

Just making sure @kpreid, why are there so many removed instances of this lint on lintcheck?

Like this one:
Removed clippy::module_name_repetitions at addr2line-0.24.0/src/lookup.rs:60

warning: item name starts with its containing module's name
  --> target/lintcheck/sources/addr2line-0.24.0/src/lookup.rs:60:11
   |
60 | pub trait LookupContinuation: Sized {
   |           ^^^^^^^^^^^^^^^^^^

The module is called lookup.rs, and the trait is public, is there anything that I'm not noticing?

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 9eaf82b (9eaf82be62c53ee20edb1cec6accf9c600f3e595)

@kpreid
Copy link
Contributor Author

kpreid commented Oct 12, 2024

The module is called lookup.rs, and the trait is public, is there anything that I'm not noticing?

The module is not public. In the parent its items are re-exported, and so their public paths have no repetition:

mod lookup;
pub use lookup::{LookupContinuation, LookupResult, SplitDwarfLoad};

This is precisely an example of the false positive that this PR is intended to fix.

There may also be cases where the item’s public path does contain a repetition, and the instance of the lint firing on the item has disappeared, but the lint still fires on the re-export. For example, in

pub mod foo {
    mod builder {
        pub struct FooBuilder;
    }
    pub use builder::FooBuilder;
}

the lint used to fire on struct FooBuilder due to the suffix Builder, and fire on pub use builder::FooBuilder due to the prefix Foo, but will now only, correctly, fire on the latter. (But if the re-export is a glob, then it won’t fire at all, and that's mentioned in the known issues section.)

@blyxyas
Copy link
Member

blyxyas commented Oct 12, 2024

I've verified a some more of these removed positives, and you're right! Everything's correct now, thanks for your contribution.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

📌 Commit a235cbd has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

⌛ Testing commit a235cbd with merge 55f6029...

@bors
Copy link
Collaborator

bors commented Oct 12, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 55f6029 to master...

@bors bors merged commit 55f6029 into rust-lang:master Oct 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

module_name_repetitions false positive when in private module
5 participants