Remove the configuration option

Also no longer lints non-exported types now

Author: Centri3

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,46 +33,40 @@ 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())
                 && 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 =>
+                && 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
+    )
+}

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.

tests/ui-toml/error_impl_error/allow_private/clippy.toml

@@ -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;
 }

tests/ui-toml/error_impl_error/disallow_private/clippy.toml

@@ -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;
    |              ^^^^^