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

Add useless_anonymous_reexport lint #109003

Merged
merged 3 commits into from
Mar 19, 2023
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
3 changes: 3 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -508,3 +508,6 @@ lint_opaque_hidden_inferred_bound = opaque type `{$ty}` does not satisfy its ass
.specifically = this associated type bound is unsatisfied for `{$proj_ty}`

lint_opaque_hidden_inferred_bound_sugg = add this bound

lint_useless_anonymous_reexport = useless anonymous re-export
.note = only anonymous re-exports of traits are useful, this is {$article} `{$desc}`
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ mod opaque_hidden_inferred_bound;
mod pass_by_value;
mod passes;
mod redundant_semicolon;
mod reexports;
mod traits;
mod types;
mod unused;
Expand Down Expand Up @@ -111,6 +112,7 @@ use noop_method_call::*;
use opaque_hidden_inferred_bound::*;
use pass_by_value::*;
use redundant_semicolon::*;
use reexports::*;
use traits::*;
use types::*;
use unused::*;
Expand Down Expand Up @@ -242,6 +244,7 @@ late_lint_methods!(
OpaqueHiddenInferredBound: OpaqueHiddenInferredBound,
MultipleSupertraitUpcastable: MultipleSupertraitUpcastable,
MapUnitFn: MapUnitFn,
UselessAnonymousReexport: UselessAnonymousReexport,
]
]
);
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1530,3 +1530,11 @@ pub struct UnusedAllocationDiag;
#[derive(LintDiagnostic)]
#[diag(lint_unused_allocation_mut)]
pub struct UnusedAllocationMutDiag;

#[derive(LintDiagnostic)]
#[diag(lint_useless_anonymous_reexport)]
#[note]
pub struct UselessAnonymousReexportDiag {
pub article: &'static str,
pub desc: &'static str,
}
82 changes: 82 additions & 0 deletions compiler/rustc_lint/src/reexports.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
use crate::lints::UselessAnonymousReexportDiag;
use crate::{LateContext, LateLintPass, LintContext};
use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefId;
use rustc_hir::{Item, ItemKind, UseKind};
use rustc_middle::ty::Visibility;
use rustc_span::symbol::kw;
use rustc_span::Span;

declare_lint! {
/// The `useless_anonymous_reexport` lint checks if anonymous re-exports
/// are re-exports of traits.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(useless_anonymous_reexport)]
///
/// mod sub {
/// pub struct Bar;
/// }
///
/// pub use self::sub::Bar as _;
/// # fn main() {}
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Anonymous re-exports are only useful if it's a re-export of a trait
/// in case you want to give access to it. If you re-export any other kind,
/// you won't be able to use it since its name won't be accessible.
pub USELESS_ANONYMOUS_REEXPORT,
Warn,
"useless anonymous re-export"
}

declare_lint_pass!(UselessAnonymousReexport => [USELESS_ANONYMOUS_REEXPORT]);

fn emit_err(cx: &LateContext<'_>, span: Span, def_id: DefId) {
let article = cx.tcx.def_descr_article(def_id);
let desc = cx.tcx.def_descr(def_id);
cx.emit_spanned_lint(
USELESS_ANONYMOUS_REEXPORT,
span,
UselessAnonymousReexportDiag { article, desc },
);
}

impl<'tcx> LateLintPass<'tcx> for UselessAnonymousReexport {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
if let ItemKind::Use(path, kind) = item.kind &&
!matches!(kind, UseKind::Glob) &&
item.ident.name == kw::Underscore &&
// We only want re-exports. If it's just a `use X;`, then we ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this important?
A private use some::Struct as _ is equally useless.

The original suggestion from @cjgillot to use UnusedImportCheckVisitor and report this early looks like the right direction.

This case doesn't need a separate lint, existing unused_imports should be perfectly fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

This lint multiplies entities without necessity, I think it is better to remove it before it hit stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I noticed this while auditing uses of imports in HIR for #109176 (comment).)

Copy link
Member Author

Choose a reason for hiding this comment

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

This lint multiplies entities without necessity

How so?

I think it is better to remove it before it hit stable.

Replaced, sure. Otherwise please don't remove it altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

How so?

Is there any scenario when you'd want to enable/deny reporting this case specifically, separately from other unused imports? I don't think so.

Replaced, sure. Otherwise please don't remove it altogether.

Yeah, I mean merge into unused_imports, not remove outright.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also, the lint name doesn't match conventions.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I see. Well, since both you and cjgillot think so, I'll move it into unused imports.

match cx.tcx.local_visibility(item.owner_id.def_id) {
Visibility::Public => true,
Visibility::Restricted(level) => {
level != cx.tcx.parent_module_from_def_id(item.owner_id.def_id)
}
}
{
for def_id in path.res.iter().filter_map(|r| r.opt_def_id()) {
match cx.tcx.def_kind(def_id) {
DefKind::Trait | DefKind::TraitAlias => {}
DefKind::TyAlias => {
let ty = cx.tcx.type_of(def_id);
if !ty.0.is_trait() {
emit_err(cx, item.span, def_id);
break;
}
}
_ => {
emit_err(cx, item.span, def_id);
break;
}
}
}
}
}
}
2 changes: 1 addition & 1 deletion tests/ui/imports/issue-99695-b.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix
#![allow(unused, nonstandard_style)]
#![allow(unused, nonstandard_style, useless_anonymous_reexport)]
mod m {

mod p {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/imports/issue-99695-b.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix
#![allow(unused, nonstandard_style)]
#![allow(unused, nonstandard_style, useless_anonymous_reexport)]
mod m {

mod p {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/imports/issue-99695.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix
#![allow(unused, nonstandard_style)]
#![allow(unused, nonstandard_style, useless_anonymous_reexport)]
mod m {
#[macro_export]
macro_rules! nu {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/imports/issue-99695.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix
#![allow(unused, nonstandard_style)]
#![allow(unused, nonstandard_style, useless_anonymous_reexport)]
mod m {
#[macro_export]
macro_rules! nu {
Expand Down
21 changes: 21 additions & 0 deletions tests/ui/lint/anonymous-reexport.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#![deny(useless_anonymous_reexport)]
#![crate_type = "rlib"]

mod my_mod {
pub trait Foo {}
pub type TyFoo = dyn Foo;
pub struct Bar;
pub type TyBar = Bar;
}

pub use self::my_mod::Foo as _;
pub use self::my_mod::TyFoo as _;
pub use self::my_mod::Bar as _; //~ ERROR
pub use self::my_mod::TyBar as _; //~ ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we suggest to remove it?
Could you add tests with nested uses:

pub use self::my_mod::{Bar as _};
pub use self::my_mod::{Bar as _, Foo as _};
pub use self::my_mod::{Bar as _, TyBar as _};

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding tests with nested uses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we suggest to remove it?

I'm not sure. If you prefer I add this suggestion, I can.

pub use self::my_mod::{Bar as _}; //~ ERROR
pub use self::my_mod::{Bar as _, Foo as _}; //~ ERROR
pub use self::my_mod::{Bar as _, TyBar as _};
//~^ ERROR
//~| ERROR
#[allow(unused_imports)]
use self::my_mod::TyBar as _;
55 changes: 55 additions & 0 deletions tests/ui/lint/anonymous-reexport.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:13:1
|
LL | pub use self::my_mod::Bar as _;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `struct`
note: the lint level is defined here
--> $DIR/anonymous-reexport.rs:1:9
|
LL | #![deny(useless_anonymous_reexport)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:14:1
|
LL | pub use self::my_mod::TyBar as _;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `type alias`

error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:15:24
|
LL | pub use self::my_mod::{Bar as _};
| ^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `struct`

error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:16:24
|
LL | pub use self::my_mod::{Bar as _, Foo as _};
| ^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `struct`

error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:17:24
|
LL | pub use self::my_mod::{Bar as _, TyBar as _};
| ^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `struct`

error: useless anonymous re-export
--> $DIR/anonymous-reexport.rs:17:34
|
LL | pub use self::my_mod::{Bar as _, TyBar as _};
| ^^^^^^^^^^
|
= note: only anonymous re-exports of traits are useful, this is a `type alias`

error: aborting due to 6 previous errors