From 09a7c645535d23f5c899d8002ecd08ab1826757c 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: [PATCH] Remove the configuration option Also no longer lints non-exported types now --- clippy_lints/src/error_impl_error.rs | 36 ++++++++++--------- clippy_lints/src/utils/conf.rs | 4 --- .../allow_private/clippy.toml | 1 - .../disallow_private/clippy.toml | 1 - .../error_impl_error.allow_private.stderr | 33 ----------------- .../error_impl_error.rs | 30 ++++++++++++---- .../error_impl_error.stderr} | 26 +++++++------- 7 files changed, 56 insertions(+), 75 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 rename tests/{ui-toml/error_impl_error => ui}/error_impl_error.rs (75%) rename tests/{ui-toml/error_impl_error/error_impl_error.disallow_private.stderr => ui/error_impl_error.stderr} (55%) diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs index 585a0ad04c71..4a6445d5053c 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::def_id::{DefId, LocalDefId}; use rustc_hir::{Item, ItemKind, Node}; 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! { @@ -32,31 +33,24 @@ declare_clippy_lint! { 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, -} +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()) @@ -64,14 +58,15 @@ impl<'tcx> LateLintPass<'tcx> for ErrorImplError { && 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 => + && ty_item.ident.name == sym::Error + && is_visible_outside_module(cx, ty_item.owner_id.def_id) => { span_lint_hir_and_then( cx, ERROR_IMPL_ERROR, hir_id, ty_item.ident.span, - "type named `Error` that implements `Error`", + "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 22aef713db65..76654bfe5360 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 042021480310..000000000000 --- 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 d42da3160a93..000000000000 --- 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 520c3e0a3e61..000000000000 --- 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.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 3f14b777ee8a..40ce4181bf34 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 d29c5ef374ff..f3e04b641673 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; | ^^^^^