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/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-toml/error_impl_error/error_impl_error.rs b/tests/ui/error_impl_error.rs similarity index 75% rename from tests/ui-toml/error_impl_error/error_impl_error.rs rename to tests/ui/error_impl_error.rs index 3f14b777ee8..40ce4181bf3 100644 --- a/tests/ui-toml/error_impl_error/error_impl_error.rs +++ b/tests/ui/error_impl_error.rs @@ -1,6 +1,3 @@ -//@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] @@ -20,7 +17,7 @@ pub mod a { mod b { #[derive(Debug)] - enum Error {} + pub(super) enum Error {} impl std::fmt::Display for Error { fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -58,7 +55,7 @@ pub mod d { mod e { #[derive(Debug)] - struct MyError; + pub(super) struct MyError; impl std::fmt::Display for MyError { fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -69,6 +66,25 @@ mod e { impl std::error::Error for MyError {} } -mod f { - type MyError = std::fmt::Error; +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-toml/error_impl_error/error_impl_error.disallow_private.stderr b/tests/ui/error_impl_error.stderr similarity index 55% rename from tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr rename to tests/ui/error_impl_error.stderr index d29c5ef374f..f3e04b64167 100644 --- a/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr +++ b/tests/ui/error_impl_error.stderr @@ -1,42 +1,42 @@ -error: type named `Error` that implements `Error` - --> $DIR/error_impl_error.rs:10:16 +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:18:5 + --> $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:23:10 +error: exported type named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:20:21 | -LL | enum Error {} - | ^^^^^ +LL | pub(super) enum Error {} + | ^^^^^ | note: `Error` was implemented here - --> $DIR/error_impl_error.rs:31:5 + --> $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:35:15 +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:52:5 + --> $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:56:14 +error: exported type alias named `Error` that implements `Error` + --> $DIR/error_impl_error.rs:53:14 | LL | pub type Error = std::fmt::Error; | ^^^^^