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: [PATCH 1/3] New lint [`error_impl_error`]

---
 CHANGELOG.md                         |  1 +
 clippy_lints/src/declared_lints.rs   |  1 +
 clippy_lints/src/error_impl_error.rs | 76 ++++++++++++++++++++++++++++
 clippy_lints/src/lib.rs              |  2 +
 tests/ui/error_impl_error.rs         | 71 ++++++++++++++++++++++++++
 tests/ui/error_impl_error.stderr     | 45 ++++++++++++++++
 6 files changed, 196 insertions(+)
 create mode 100644 clippy_lints/src/error_impl_error.rs
 create mode 100644 tests/ui/error_impl_error.rs
 create mode 100644 tests/ui/error_impl_error.stderr

diff --git a/CHANGELOG.md b/CHANGELOG.md
index db18bc296f70..0445e59480c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4819,6 +4819,7 @@ Released 2018-09-13
 [`equatable_if_let`]: https://rust-lang.github.io/rust-clippy/master/index.html#equatable_if_let
 [`erasing_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#erasing_op
 [`err_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#err_expect
+[`error_impl_error`]: https://rust-lang.github.io/rust-clippy/master/index.html#error_impl_error
 [`eval_order_dependence`]: https://rust-lang.github.io/rust-clippy/master/index.html#eval_order_dependence
 [`excessive_nesting`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_nesting
 [`excessive_precision`]: https://rust-lang.github.io/rust-clippy/master/index.html#excessive_precision
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
index 51035a9d66b2..cd507d8a9e99 100644
--- a/clippy_lints/src/declared_lints.rs
+++ b/clippy_lints/src/declared_lints.rs
@@ -155,6 +155,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::enum_variants::MODULE_INCEPTION_INFO,
     crate::enum_variants::MODULE_NAME_REPETITIONS_INFO,
     crate::equatable_if_let::EQUATABLE_IF_LET_INFO,
+    crate::error_impl_error::ERROR_IMPL_ERROR_INFO,
     crate::escape::BOXED_LOCAL_INFO,
     crate::eta_reduction::REDUNDANT_CLOSURE_INFO,
     crate::eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS_INFO,
diff --git a/clippy_lints/src/error_impl_error.rs b/clippy_lints/src/error_impl_error.rs
new file mode 100644
index 000000000000..7f361d9ba022
--- /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");
+                    }
+                );
+            }
+            _ => {},
+        }
+        {}
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 4ea25e4be9dc..1609eb396b43 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -120,6 +120,7 @@ mod entry;
 mod enum_clike;
 mod enum_variants;
 mod equatable_if_let;
+mod error_impl_error;
 mod escape;
 mod eta_reduction;
 mod excessive_bools;
@@ -1080,6 +1081,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move |_| Box::new(tuple_array_conversions::TupleArrayConversions { msrv: msrv() }));
     store.register_late_pass(|_| Box::new(manual_float_methods::ManualFloatMethods));
     store.register_late_pass(|_| Box::new(four_forward_slashes::FourForwardSlashes));
+    store.register_late_pass(|_| Box::new(error_impl_error::ErrorImplError));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/tests/ui/error_impl_error.rs b/tests/ui/error_impl_error.rs
new file mode 100644
index 000000000000..d85f95220815
--- /dev/null
+++ b/tests/ui/error_impl_error.rs
@@ -0,0 +1,71 @@
+#![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
new file mode 100644
index 000000000000..64af0594e0a2
--- /dev/null
+++ b/tests/ui/error_impl_error.stderr
@@ -0,0 +1,45 @@
+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
+

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: [PATCH 2/3] 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 +++
 .../allow_private/clippy.toml                 |  1 +
 .../disallow_private/clippy.toml              |  1 +
 .../error_impl_error.allow_private.stderr     | 33 +++++++++++++++++++
 .../error_impl_error.disallow_private.stderr} | 26 +++++++--------
 .../error_impl_error}/error_impl_error.rs     | 15 +++++----
 .../toml_unknown_key/conf_unknown_key.stderr  |  2 ++
 10 files changed, 93 insertions(+), 31 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
 rename tests/{ui/error_impl_error.stderr => ui-toml/error_impl_error/error_impl_error.disallow_private.stderr} (64%)
 rename tests/{ui => ui-toml/error_impl_error}/error_impl_error.rs (77%)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0445e59480c6..0676752b7662 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
 <!-- end autogenerated links to configuration documentation -->
diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
index f8073dac3301..1035f3e7fb69 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 7f361d9ba022..585a0ad04c71 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 76654bfe5360..22aef713db65 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 000000000000..042021480310
--- /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 000000000000..d42da3160a93
--- /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 000000000000..520c3e0a3e61
--- /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/error_impl_error.stderr b/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr
similarity index 64%
rename from tests/ui/error_impl_error.stderr
rename to tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr
index 64af0594e0a2..d29c5ef374ff 100644
--- a/tests/ui/error_impl_error.stderr
+++ b/tests/ui-toml/error_impl_error/error_impl_error.disallow_private.stderr
@@ -1,45 +1,45 @@
 error: type named `Error` that implements `Error`
-  --> $DIR/error_impl_error.rs:7:12
+  --> $DIR/error_impl_error.rs:10:16
    |
-LL |     struct Error;
-   |            ^^^^^
+LL |     pub struct Error;
+   |                ^^^^^
    |
 note: `Error` was implemented here
-  --> $DIR/error_impl_error.rs:15:5
+  --> $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:20:10
+  --> $DIR/error_impl_error.rs:23:10
    |
 LL |     enum Error {}
    |          ^^^^^
    |
 note: `Error` was implemented here
-  --> $DIR/error_impl_error.rs:28:5
+  --> $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:32:11
+  --> $DIR/error_impl_error.rs:35:15
    |
-LL |     union Error {
-   |           ^^^^^
+LL |     pub union Error {
+   |               ^^^^^
    |
 note: `Error` was implemented here
-  --> $DIR/error_impl_error.rs:49:5
+  --> $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:53:10
+  --> $DIR/error_impl_error.rs:56:14
    |
-LL |     type Error = std::fmt::Error;
-   |          ^^^^^
+LL |     pub type Error = std::fmt::Error;
+   |              ^^^^^
 
 error: aborting due to 4 previous errors
 
diff --git a/tests/ui/error_impl_error.rs b/tests/ui-toml/error_impl_error/error_impl_error.rs
similarity index 77%
rename from tests/ui/error_impl_error.rs
rename to tests/ui-toml/error_impl_error/error_impl_error.rs
index d85f95220815..3f14b777ee8a 100644
--- a/tests/ui/error_impl_error.rs
+++ b/tests/ui-toml/error_impl_error/error_impl_error.rs
@@ -1,10 +1,13 @@
+//@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]
 
-mod a {
+pub mod a {
     #[derive(Debug)]
-    struct Error;
+    pub struct Error;
 
     impl std::fmt::Display for Error {
         fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
@@ -28,8 +31,8 @@ mod b {
     impl std::error::Error for Error {}
 }
 
-mod c {
-    union Error {
+pub mod c {
+    pub union Error {
         a: u32,
         b: u32,
     }
@@ -49,8 +52,8 @@ mod c {
     impl std::error::Error for Error {}
 }
 
-mod d {
-    type Error = std::fmt::Error;
+pub mod d {
+    pub type Error = std::fmt::Error;
 }
 
 mod e {
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 6ba26e977302..227424f948cb 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

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: [PATCH 3/3] 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 --
 .../allow_private/clippy.toml                 |  1 -
 .../disallow_private/clippy.toml              |  1 -
 .../error_impl_error.allow_private.stderr     | 33 --------------
 .../toml_unknown_key/conf_unknown_key.stderr  |  2 -
 .../error_impl_error.rs                       | 30 ++++++++++---
 .../error_impl_error.stderr}                  | 26 +++++------
 10 files changed, 60 insertions(+), 92 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/CHANGELOG.md b/CHANGELOG.md
index 0676752b7662..0445e59480c6 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
 <!-- end autogenerated links to configuration documentation -->
diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
index 1035f3e7fb69..f8073dac3301 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 585a0ad04c71..379af9b2234c 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 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/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
index 227424f948cb..6ba26e977302 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 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;
    |              ^^^^^