diff --git a/CHANGELOG.md b/CHANGELOG.md index acc9d8ffb..437f08dd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4819,6 +4819,7 @@ Released 2018-09-13 [`equatable_if_let`]: https://rust-lang.github.io/rust-clippy/master/index.html#equatable_if_let [`erasing_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#erasing_op [`err_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#err_expect +[`error_impl_error`]: https://rust-lang.github.io/rust-clippy/master/index.html#error_impl_error [`eval_order_dependence`]: https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence [`excessive_nesting`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_nesting [`excessive_precision`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_precision diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 47995fea9..a0d181be3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -155,6 +155,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::enum_variants::MODULE_INCEPTION_INFO, crate::enum_variants::MODULE_NAME_REPETITIONS_INFO, crate::equatable_if_let::EQUATABLE_IF_LET_INFO, + crate::error_impl_error::ERROR_IMPL_ERROR_INFO, crate::escape::BOXED_LOCAL_INFO, crate::eta_reduction::REDUNDANT_CLOSURE_INFO, crate::eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS_INFO, diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs new file mode 100644 index 000000000..379af9b22 --- /dev/null +++ b/clippy_lints/src/error_impl_error.rs @@ -0,0 +1,87 @@ +use clippy_utils::diagnostics::{span_lint, span_lint_hir_and_then}; +use clippy_utils::path_res; +use clippy_utils::ty::implements_trait; +use rustc_hir::def_id::{DefId, LocalDefId}; +use rustc_hir::{Item, ItemKind}; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Visibility; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Checks for types named `Error` that implement `Error`. + /// + /// ### Why is this bad? + /// It can become confusing when a codebase has 20 types all named `Error`, requiring either + /// aliasing them in the `use` statement or qualifying them like `my_module::Error`. This + /// hinders comprehension, as it requires you to memorize every variation of importing `Error` + /// used across a codebase. + /// + /// ### Example + /// ```rust,ignore + /// #[derive(Debug)] + /// pub enum Error { ... } + /// + /// impl std::fmt::Display for Error { ... } + /// + /// impl std::error::Error for Error { ... } + /// ``` + #[clippy::version = "1.72.0"] + pub ERROR_IMPL_ERROR, + restriction, + "exported types named `Error` that implement `Error`" +} +declare_lint_pass!(ErrorImplError => [ERROR_IMPL_ERROR]); + +impl<'tcx> LateLintPass<'tcx> for ErrorImplError { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let Some(error_def_id) = cx.tcx.get_diagnostic_item(sym::Error) else { + return; + }; + + match item.kind { + ItemKind::TyAlias(ty, _) if implements_trait(cx, hir_ty_to_ty(cx.tcx, ty), error_def_id, &[]) + && item.ident.name == sym::Error + && is_visible_outside_module(cx, item.owner_id.def_id) => + { + span_lint( + cx, + ERROR_IMPL_ERROR, + item.ident.span, + "exported type alias named `Error` that implements `Error`", + ); + }, + ItemKind::Impl(imp) if let Some(trait_def_id) = imp.of_trait.and_then(|t| t.trait_def_id()) + && error_def_id == trait_def_id + && let Some(def_id) = path_res(cx, imp.self_ty).opt_def_id().and_then(DefId::as_local) + && let hir_id = cx.tcx.hir().local_def_id_to_hir_id(def_id) + && let Some(ident) = cx.tcx.opt_item_ident(def_id.to_def_id()) + && ident.name == sym::Error + && is_visible_outside_module(cx, def_id) => + { + span_lint_hir_and_then( + cx, + ERROR_IMPL_ERROR, + hir_id, + ident.span, + "exported type named `Error` that implements `Error`", + |diag| { + diag.span_note(item.span, "`Error` was implemented here"); + } + ); + } + _ => {}, + } + } +} + +/// Do not lint private `Error`s, i.e., ones without any `pub` (minus `pub(self)` of course) and +/// which aren't reexported +fn is_visible_outside_module(cx: &LateContext<'_>, def_id: LocalDefId) -> bool { + !matches!( + cx.tcx.visibility(def_id), + Visibility::Restricted(mod_def_id) if cx.tcx.parent_module_from_def_id(def_id).to_def_id() == mod_def_id + ) +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4ea25e4be..1609eb396 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -120,6 +120,7 @@ mod entry; mod enum_clike; mod enum_variants; mod equatable_if_let; +mod error_impl_error; mod escape; mod eta_reduction; mod excessive_bools; @@ -1080,6 +1081,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move |_| Box::new(tuple_array_conversions::TupleArrayConversions { msrv: msrv() })); store.register_late_pass(|_| Box::new(manual_float_methods::ManualFloatMethods)); store.register_late_pass(|_| Box::new(four_forward_slashes::FourForwardSlashes)); + store.register_late_pass(|_| Box::new(error_impl_error::ErrorImplError)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/error_impl_error.rs b/tests/ui/error_impl_error.rs new file mode 100644 index 000000000..40ce4181b --- /dev/null +++ b/tests/ui/error_impl_error.rs @@ -0,0 +1,90 @@ +#![allow(unused)] +#![warn(clippy::error_impl_error)] +#![no_main] + +pub mod a { + #[derive(Debug)] + pub struct Error; + + impl std::fmt::Display for Error { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::error::Error for Error {} +} + +mod b { + #[derive(Debug)] + pub(super) enum Error {} + + impl std::fmt::Display for Error { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::error::Error for Error {} +} + +pub mod c { + pub union Error { + a: u32, + b: u32, + } + + impl std::fmt::Debug for Error { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::fmt::Display for Error { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::error::Error for Error {} +} + +pub mod d { + pub type Error = std::fmt::Error; +} + +mod e { + #[derive(Debug)] + pub(super) struct MyError; + + impl std::fmt::Display for MyError { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::error::Error for MyError {} +} + +pub mod f { + pub type MyError = std::fmt::Error; +} + +// Do not lint module-private types + +mod g { + #[derive(Debug)] + enum Error {} + + impl std::fmt::Display for Error { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + todo!() + } + } + + impl std::error::Error for Error {} +} + +mod h { + type Error = std::fmt::Error; +} diff --git a/tests/ui/error_impl_error.stderr b/tests/ui/error_impl_error.stderr new file mode 100644 index 000000000..f3e04b641 --- /dev/null +++ b/tests/ui/error_impl_error.stderr @@ -0,0 +1,45 @@ +error: exported type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:7:16 + | +LL | pub struct Error; + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:15:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::error-impl-error` implied by `-D warnings` + +error: exported type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:20:21 + | +LL | pub(super) enum Error {} + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:28:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: exported type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:32:15 + | +LL | pub union Error { + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:49:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: exported type alias named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:53:14 + | +LL | pub type Error = std::fmt::Error; + | ^^^^^ + +error: aborting due to 4 previous errors +