From 75e1329aac850a15f29a86ac685b3cb4794b8b4f Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Tue, 4 Jul 2023 13:36:56 -0500 Subject: New lint [`error_impl_error`] --- clippy_lints/src/error_impl_error.rs | 76 ++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 clippy_lints/src/error_impl_error.rs (limited to 'clippy_lints/src/error_impl_error.rs') diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs new file mode 100644 index 00000000000..7f361d9ba02 --- /dev/null +++ b/clippy_lints/src/error_impl_error.rs @@ -0,0 +1,76 @@ +use clippy_utils::{ + diagnostics::{span_lint, span_lint_hir_and_then}, + path_res, + ty::implements_trait, +}; +use rustc_hir::{def_id::DefId, Item, ItemKind, Node}; +use rustc_hir_analysis::hir_ty_to_ty; +use rustc_lint::{LateContext, LateLintPass}; +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 them or qualifying them like `my_module::Error`. This + /// severely hinders readability. + /// + /// ### 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, + "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 => + { + span_lint( + cx, + ERROR_IMPL_ERROR, + item.ident.span, + "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 Node::Item(ty_item) = cx.tcx.hir().get(hir_id) + && ty_item.ident.name == sym::Error => + { + span_lint_hir_and_then( + cx, + ERROR_IMPL_ERROR, + hir_id, + ty_item.ident.span, + "type named `Error` that implements `Error`", + |diag| { + diag.span_note(item.span, "`Error` was implemented here"); + } + ); + } + _ => {}, + } + {} + } +} -- cgit 1.4.1-3-g733a5 From f4b3bb19c18cbce910875d6fc97de70005b8dcdb Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Thu, 6 Jul 2023 21:58:11 -0500 Subject: Add `allow_private_error` config option --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 10 +++ clippy_lints/src/error_impl_error.rs | 31 +++++---- clippy_lints/src/utils/conf.rs | 4 ++ .../error_impl_error/allow_private/clippy.toml | 1 + .../error_impl_error/disallow_private/clippy.toml | 1 + .../error_impl_error.allow_private.stderr | 33 ++++++++++ .../error_impl_error.disallow_private.stderr | 45 +++++++++++++ tests/ui-toml/error_impl_error/error_impl_error.rs | 74 ++++++++++++++++++++++ .../toml_unknown_key/conf_unknown_key.stderr | 2 + tests/ui/error_impl_error.rs | 71 --------------------- tests/ui/error_impl_error.stderr | 45 ------------- 12 files changed, 190 insertions(+), 128 deletions(-) create mode 100644 tests/ui-toml/error_impl_error/allow_private/clippy.toml create mode 100644 tests/ui-toml/error_impl_error/disallow_private/clippy.toml create mode 100644 tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr create mode 100644 tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr create mode 100644 tests/ui-toml/error_impl_error/error_impl_error.rs delete mode 100644 tests/ui/error_impl_error.rs delete mode 100644 tests/ui/error_impl_error.stderr (limited to 'clippy_lints/src/error_impl_error.rs') diff --git a/CHANGELOG.md b/CHANGELOG.md index 0445e59480c..0676752b766 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5465,4 +5465,5 @@ Released 2018-09-13 [`accept-comment-above-statement`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-statement [`accept-comment-above-attributes`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-attributes [`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings +[`allow-private-error`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-private-error diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index f8073dac330..1035f3e7fb6 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -730,3 +730,13 @@ Whether to allow `r#""#` when `r""` can be used * [`unnecessary_raw_string_hashes`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_raw_string_hashes) +## `allow-private-error` +Whether to allow private types named `Error` that implement `Error` + +**Default Value:** `true` (`bool`) + +--- +**Affected lints:** +* [`error_impl_error`](https://rust-lang.github.io/rust-clippy/master/index.html#error_impl_error) + + diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs index 7f361d9ba02..585a0ad04c7 100644 --- a/clippy_lints/src/error_impl_error.rs +++ b/clippy_lints/src/error_impl_error.rs @@ -1,12 +1,11 @@ -use clippy_utils::{ - diagnostics::{span_lint, span_lint_hir_and_then}, - path_res, - ty::implements_trait, -}; -use rustc_hir::{def_id::DefId, Item, ItemKind, Node}; +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; +use rustc_hir::{Item, ItemKind, Node}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::sym; declare_clippy_lint! { @@ -15,8 +14,9 @@ declare_clippy_lint! { /// /// ### 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 them or qualifying them like `my_module::Error`. This - /// severely hinders readability. + /// 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 @@ -32,14 +32,22 @@ declare_clippy_lint! { restriction, "types named `Error` that implement `Error`" } -declare_lint_pass!(ErrorImplError => [ERROR_IMPL_ERROR]); +impl_lint_pass!(ErrorImplError => [ERROR_IMPL_ERROR]); + +#[derive(Clone, Copy)] +pub struct ErrorImplError { + pub allow_private_error: bool, +} impl<'tcx> LateLintPass<'tcx> for ErrorImplError { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + let Self { allow_private_error } = *self; let Some(error_def_id) = cx.tcx.get_diagnostic_item(sym::Error) else { return; }; - + if allow_private_error && !cx.effective_visibilities.is_exported(item.owner_id.def_id) { + 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 => @@ -71,6 +79,5 @@ impl<'tcx> LateLintPass<'tcx> for ErrorImplError { } _ => {}, } - {} } } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 76654bfe536..22aef713db6 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -551,6 +551,10 @@ define_Conf! { /// /// Whether to allow `r#""#` when `r""` can be used (allow_one_hash_in_raw_strings: bool = false), + /// Lint: ERROR_IMPL_ERROR. + /// + /// Whether to allow private types named `Error` that implement `Error` + (allow_private_error: bool = true), } /// Search for the configuration file. diff --git a/tests/ui-toml/error_impl_error/allow_private/clippy.toml b/tests/ui-toml/error_impl_error/allow_private/clippy.toml new file mode 100644 index 00000000000..04202148031 --- /dev/null +++ b/tests/ui-toml/error_impl_error/allow_private/clippy.toml @@ -0,0 +1 @@ +allow-private-error = true diff --git a/tests/ui-toml/error_impl_error/disallow_private/clippy.toml b/tests/ui-toml/error_impl_error/disallow_private/clippy.toml new file mode 100644 index 00000000000..d42da3160a9 --- /dev/null +++ b/tests/ui-toml/error_impl_error/disallow_private/clippy.toml @@ -0,0 +1 @@ +allow-private-error = false diff --git a/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr b/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr new file mode 100644 index 00000000000..520c3e0a3e6 --- /dev/null +++ b/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr @@ -0,0 +1,33 @@ +error: type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:10:16 + | +LL | pub struct Error; + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:18:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::error-impl-error` implied by `-D warnings` + +error: type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:35:15 + | +LL | pub union Error { + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:52:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: type alias named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:56:14 + | +LL | pub type Error = std::fmt::Error; + | ^^^^^ + +error: aborting due to 3 previous errors + diff --git a/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr b/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr new file mode 100644 index 00000000000..d29c5ef374f --- /dev/null +++ b/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr @@ -0,0 +1,45 @@ +error: type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:10:16 + | +LL | pub struct Error; + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:18:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::error-impl-error` implied by `-D warnings` + +error: type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:23:10 + | +LL | enum Error {} + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:31:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:35:15 + | +LL | pub union Error { + | ^^^^^ + | +note: `Error` was implemented here + --> $DIR/error_impl_error.rs:52:5 + | +LL | impl std::error::Error for Error {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: type alias named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:56:14 + | +LL | pub type Error = std::fmt::Error; + | ^^^^^ + +error: aborting due to 4 previous errors + diff --git a/tests/ui-toml/error_impl_error/error_impl_error.rs b/tests/ui-toml/error_impl_error/error_impl_error.rs new file mode 100644 index 00000000000..3f14b777ee8 --- /dev/null +++ b/tests/ui-toml/error_impl_error/error_impl_error.rs @@ -0,0 +1,74 @@ +//@revisions: allow_private disallow_private +//@[allow_private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/error_impl_error/allow_private +//@[disallow_private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/error_impl_error/disallow_private +#![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)] + 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)] + 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 {} +} + +mod f { + type MyError = std::fmt::Error; +} diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 6ba26e97730..227424f948c 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -6,6 +6,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect allow-mixed-uninlined-format-args allow-one-hash-in-raw-strings allow-print-in-tests + allow-private-error allow-private-module-inception allow-unwrap-in-tests allowed-idents-below-min-chars @@ -75,6 +76,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect allow-mixed-uninlined-format-args allow-one-hash-in-raw-strings allow-print-in-tests + allow-private-error allow-private-module-inception allow-unwrap-in-tests allowed-idents-below-min-chars diff --git a/tests/ui/error_impl_error.rs b/tests/ui/error_impl_error.rs deleted file mode 100644 index d85f9522081..00000000000 --- a/tests/ui/error_impl_error.rs +++ /dev/null @@ -1,71 +0,0 @@ -#![allow(unused)] -#![warn(clippy::error_impl_error)] -#![no_main] - -mod a { - #[derive(Debug)] - 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)] - 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 c { - 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 {} -} - -mod d { - type Error = std::fmt::Error; -} - -mod e { - #[derive(Debug)] - 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 {} -} - -mod f { - type MyError = std::fmt::Error; -} diff --git a/tests/ui/error_impl_error.stderr b/tests/ui/error_impl_error.stderr deleted file mode 100644 index 64af0594e0a..00000000000 --- a/tests/ui/error_impl_error.stderr +++ /dev/null @@ -1,45 +0,0 @@ -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:7:12 - | -LL | 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: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:20:10 - | -LL | enum Error {} - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:28:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:32:11 - | -LL | union Error { - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:49:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: type alias named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:53:10 - | -LL | type Error = std::fmt::Error; - | ^^^^^ - -error: aborting due to 4 previous errors - -- cgit 1.4.1-3-g733a5 From 19b0e84187fd0c7ada499adc256d671e2d6ad78c Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Tue, 18 Jul 2023 10:25:58 -0500 Subject: Remove the configuration option Also no longer lints non-exported types now --- CHANGELOG.md | 1 - book/src/lint_configuration.md | 10 --- clippy_lints/src/error_impl_error.rs | 44 ++++++----- clippy_lints/src/utils/conf.rs | 4 - .../error_impl_error/allow_private/clippy.toml | 1 - .../error_impl_error/disallow_private/clippy.toml | 1 - .../error_impl_error.allow_private.stderr | 33 -------- .../error_impl_error.disallow_private.stderr | 45 ----------- tests/ui-toml/error_impl_error/error_impl_error.rs | 74 ------------------ .../toml_unknown_key/conf_unknown_key.stderr | 2 - tests/ui/error_impl_error.rs | 90 ++++++++++++++++++++++ tests/ui/error_impl_error.stderr | 45 +++++++++++ 12 files changed, 159 insertions(+), 191 deletions(-) delete mode 100644 tests/ui-toml/error_impl_error/allow_private/clippy.toml delete mode 100644 tests/ui-toml/error_impl_error/disallow_private/clippy.toml delete mode 100644 tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr delete mode 100644 tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr delete mode 100644 tests/ui-toml/error_impl_error/error_impl_error.rs create mode 100644 tests/ui/error_impl_error.rs create mode 100644 tests/ui/error_impl_error.stderr (limited to 'clippy_lints/src/error_impl_error.rs') diff --git a/CHANGELOG.md b/CHANGELOG.md index 0676752b766..0445e59480c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5465,5 +5465,4 @@ Released 2018-09-13 [`accept-comment-above-statement`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-statement [`accept-comment-above-attributes`]: https://doc.rust-lang.org/clippy/lint_configuration.html#accept-comment-above-attributes [`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings -[`allow-private-error`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-private-error diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 1035f3e7fb6..f8073dac330 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -730,13 +730,3 @@ Whether to allow `r#""#` when `r""` can be used * [`unnecessary_raw_string_hashes`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_raw_string_hashes) -## `allow-private-error` -Whether to allow private types named `Error` that implement `Error` - -**Default Value:** `true` (`bool`) - ---- -**Affected lints:** -* [`error_impl_error`](https://rust-lang.github.io/rust-clippy/master/index.html#error_impl_error) - - diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs index 585a0ad04c7..379af9b2234 100644 --- a/clippy_lints/src/error_impl_error.rs +++ b/clippy_lints/src/error_impl_error.rs @@ -1,11 +1,12 @@ 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; -use rustc_hir::{Item, ItemKind, Node}; +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_session::{declare_tool_lint, impl_lint_pass}; +use rustc_middle::ty::Visibility; +use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; declare_clippy_lint! { @@ -30,48 +31,42 @@ declare_clippy_lint! { #[clippy::version = "1.72.0"] pub ERROR_IMPL_ERROR, restriction, - "types named `Error` that implement `Error`" -} -impl_lint_pass!(ErrorImplError => [ERROR_IMPL_ERROR]); - -#[derive(Clone, Copy)] -pub struct ErrorImplError { - pub allow_private_error: bool, + "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 Self { allow_private_error } = *self; let Some(error_def_id) = cx.tcx.get_diagnostic_item(sym::Error) else { return; }; - if allow_private_error && !cx.effective_visibilities.is_exported(item.owner_id.def_id) { - 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 => + && item.ident.name == sym::Error + && is_visible_outside_module(cx, item.owner_id.def_id) => { span_lint( cx, ERROR_IMPL_ERROR, item.ident.span, - "type alias named `Error` that implements `Error`", + "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 Node::Item(ty_item) = cx.tcx.hir().get(hir_id) - && ty_item.ident.name == sym::Error => + && 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, - ty_item.ident.span, - "type named `Error` that implements `Error`", + ident.span, + "exported type named `Error` that implements `Error`", |diag| { diag.span_note(item.span, "`Error` was implemented here"); } @@ -81,3 +76,12 @@ impl<'tcx> LateLintPass<'tcx> for ErrorImplError { } } } + +/// 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/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 22aef713db6..76654bfe536 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -551,10 +551,6 @@ define_Conf! { /// /// Whether to allow `r#""#` when `r""` can be used (allow_one_hash_in_raw_strings: bool = false), - /// Lint: ERROR_IMPL_ERROR. - /// - /// Whether to allow private types named `Error` that implement `Error` - (allow_private_error: bool = true), } /// Search for the configuration file. diff --git a/tests/ui-toml/error_impl_error/allow_private/clippy.toml b/tests/ui-toml/error_impl_error/allow_private/clippy.toml deleted file mode 100644 index 04202148031..00000000000 --- a/tests/ui-toml/error_impl_error/allow_private/clippy.toml +++ /dev/null @@ -1 +0,0 @@ -allow-private-error = true diff --git a/tests/ui-toml/error_impl_error/disallow_private/clippy.toml b/tests/ui-toml/error_impl_error/disallow_private/clippy.toml deleted file mode 100644 index d42da3160a9..00000000000 --- a/tests/ui-toml/error_impl_error/disallow_private/clippy.toml +++ /dev/null @@ -1 +0,0 @@ -allow-private-error = false diff --git a/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr b/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr deleted file mode 100644 index 520c3e0a3e6..00000000000 --- a/tests/ui-toml/error_impl_error/error_impl_error.allow_private.stderr +++ /dev/null @@ -1,33 +0,0 @@ -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:10:16 - | -LL | pub struct Error; - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:18:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: `-D clippy::error-impl-error` implied by `-D warnings` - -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:35:15 - | -LL | pub union Error { - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:52:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: type alias named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:56:14 - | -LL | pub type Error = std::fmt::Error; - | ^^^^^ - -error: aborting due to 3 previous errors - diff --git a/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr b/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr deleted file mode 100644 index d29c5ef374f..00000000000 --- a/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr +++ /dev/null @@ -1,45 +0,0 @@ -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:10:16 - | -LL | pub struct Error; - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:18:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: `-D clippy::error-impl-error` implied by `-D warnings` - -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:23:10 - | -LL | enum Error {} - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:31:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:35:15 - | -LL | pub union Error { - | ^^^^^ - | -note: `Error` was implemented here - --> $DIR/error_impl_error.rs:52:5 - | -LL | impl std::error::Error for Error {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: type alias named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:56:14 - | -LL | pub type Error = std::fmt::Error; - | ^^^^^ - -error: aborting due to 4 previous errors - diff --git a/tests/ui-toml/error_impl_error/error_impl_error.rs b/tests/ui-toml/error_impl_error/error_impl_error.rs deleted file mode 100644 index 3f14b777ee8..00000000000 --- a/tests/ui-toml/error_impl_error/error_impl_error.rs +++ /dev/null @@ -1,74 +0,0 @@ -//@revisions: allow_private disallow_private -//@[allow_private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/error_impl_error/allow_private -//@[disallow_private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/error_impl_error/disallow_private -#![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)] - 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)] - 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 {} -} - -mod f { - type MyError = std::fmt::Error; -} diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 227424f948c..6ba26e97730 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -6,7 +6,6 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect allow-mixed-uninlined-format-args allow-one-hash-in-raw-strings allow-print-in-tests - allow-private-error allow-private-module-inception allow-unwrap-in-tests allowed-idents-below-min-chars @@ -76,7 +75,6 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect allow-mixed-uninlined-format-args allow-one-hash-in-raw-strings allow-print-in-tests - allow-private-error allow-private-module-inception allow-unwrap-in-tests allowed-idents-below-min-chars diff --git a/tests/ui/error_impl_error.rs b/tests/ui/error_impl_error.rs new file mode 100644 index 00000000000..40ce4181bf3 --- /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 00000000000..f3e04b64167 --- /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 + -- cgit 1.4.1-3-g733a5