-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #11107 - Centri3:error_impl_error, r=Jarcho
New lint [`error_impl_error`] Closes #11101 changelog: New lint [`error_impl_error`]
- Loading branch information
Showing
6 changed files
with
226 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|