Auto merge of #6476 - 1c3t3a:1c3t3a-from-over-into, r=llogiq

Added from_over_into lint Closes #6456 Added a lint that searches for implementations of `Into<..>` and suggests to implement `From<..>` instead, as it comes with a default implementation of `Into`. Category: style.
rust-lang · Dec 21, 2020 · 74bce2d · 74bce2d
2 parents 4911ab1 + 0d6c128
commit 74bce2d
Show file tree

Hide file tree

Showing 10 changed files with 158 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1841,6 +1841,7 @@ Released 2018-09-13
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
 [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
+[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
 [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send
 [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap

diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs
@@ -0,0 +1,83 @@
+use crate::utils::paths::INTO;
+use crate::utils::{match_def_path, meets_msrv, span_lint_and_help};
+use if_chain::if_chain;
+use rustc_hir as hir;
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+
+const FROM_OVER_INTO_MSRV: RustcVersion = RustcVersion::new(1, 41, 0);
+
+declare_clippy_lint! {
+    /// **What it does:** Searches for implementations of the `Into<..>` trait and suggests to implement `From<..>` instead.
+    ///
+    /// **Why is this bad?** According the std docs implementing `From<..>` is preferred since it gives you `Into<..>` for free where the reverse isn't true.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// struct StringWrapper(String);
+    ///
+    /// impl Into<StringWrapper> for String {
+    ///     fn into(self) -> StringWrapper {
+    ///         StringWrapper(self)
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// struct StringWrapper(String);
+    ///
+    /// impl From<String> for StringWrapper {
+    ///     fn from(s: String) -> StringWrapper {
+    ///         StringWrapper(s)
+    ///     }
+    /// }
+    /// ```
+    pub FROM_OVER_INTO,
+    style,
+    "Warns on implementations of `Into<..>` to use `From<..>`"
+}
+
+pub struct FromOverInto {
+    msrv: Option<RustcVersion>,
+}
+
+impl FromOverInto {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        FromOverInto { msrv }
+    }
+}
+
+impl_lint_pass!(FromOverInto => [FROM_OVER_INTO]);
+
+impl LateLintPass<'_> for FromOverInto {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
+        if !meets_msrv(self.msrv.as_ref(), &FROM_OVER_INTO_MSRV) {
+            return;
+        }
+
+        let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id);
+        if_chain! {
+            if let hir::ItemKind::Impl{ .. } = &item.kind;
+            if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(impl_def_id);
+            if match_def_path(cx, impl_trait_ref.def_id, &INTO);
+
+            then {
+                span_lint_and_help(
+                    cx,
+                    FROM_OVER_INTO,
+                    item.span,
+                    "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true",
+                    None,
+                    "consider to implement `From` instead",
+                );
+            }
+        }
+    }
+
+    extract_msrv_attr!(LateContext);
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -207,6 +207,7 @@ mod float_literal;
 mod floating_point_arithmetic;
 mod format;
 mod formatting;
+mod from_over_into;
 mod functions;
 mod future_not_send;
 mod get_last_with_len;
@@ -614,6 +615,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
         &formatting::SUSPICIOUS_ELSE_FORMATTING,
         &formatting::SUSPICIOUS_UNARY_OP_FORMATTING,
+        &from_over_into::FROM_OVER_INTO,
         &functions::DOUBLE_MUST_USE,
         &functions::MUST_USE_CANDIDATE,
         &functions::MUST_USE_UNIT,
@@ -1014,6 +1016,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || box checked_conversions::CheckedConversions::new(msrv));
     store.register_late_pass(move || box mem_replace::MemReplace::new(msrv));
     store.register_late_pass(move || box ranges::Ranges::new(msrv));
+    store.register_late_pass(move || box from_over_into::FromOverInto::new(msrv));
     store.register_late_pass(move || box use_self::UseSelf::new(msrv));
     store.register_late_pass(move || box missing_const_for_fn::MissingConstForFn::new(msrv));
 
@@ -1417,6 +1420,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
+        LintId::of(&from_over_into::FROM_OVER_INTO),
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF),
@@ -1663,6 +1667,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING),
         LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
+        LintId::of(&from_over_into::FROM_OVER_INTO),
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::RESULT_UNIT_ERR),

diff --git a/tests/ui/from_over_into.rs b/tests/ui/from_over_into.rs
@@ -0,0 +1,21 @@
+#![warn(clippy::from_over_into)]
+
+// this should throw an error
+struct StringWrapper(String);
+
+impl Into<StringWrapper> for String {
+    fn into(self) -> StringWrapper {
+        StringWrapper(self)
+    }
+}
+
+// this is fine
+struct A(String);
+
+impl From<String> for A {
+    fn from(s: String) -> A {
+        A(s)
+    }
+}
+
+fn main() {}
diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr
@@ -0,0 +1,15 @@
+error: An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true.
+  --> $DIR/from_over_into.rs:6:1
+   |
+LL | / impl Into<StringWrapper> for String {
+LL | |     fn into(self) -> StringWrapper {
+LL | |         StringWrapper(self)
+LL | |     }
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::from-over-into` implied by `-D warnings`
+   = help: consider to implement `From` instead
+
+error: aborting due to previous error
+
diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs
@@ -57,6 +57,14 @@ pub fn checked_conversion() {
     let _ = value <= (u32::MAX as i64) && value >= 0;
 }
 
+pub struct FromOverInto(String);
+
+impl Into<FromOverInto> for String {
+    fn into(self) -> FromOverInto {
+        FromOverInto(self)
+    }
+}
+
 pub fn filter_map_next() {
     let a = ["1", "lol", "3", "NaN", "5"];
 

diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr
@@ -1,12 +1,12 @@
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:142:24
+  --> $DIR/min_rust_version_attr.rs:150:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::manual-strip` implied by `-D warnings`
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:141:9
+  --> $DIR/min_rust_version_attr.rs:149:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -17,13 +17,13 @@ LL |             assert_eq!(<stripped>.to_uppercase(), "WORLD!");
    |
 
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:154:24
+  --> $DIR/min_rust_version_attr.rs:162:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:153:9
+  --> $DIR/min_rust_version_attr.rs:161:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

diff --git a/tests/ui/unused_unit.fixed b/tests/ui/unused_unit.fixed
@@ -11,6 +11,7 @@
 
 #![deny(clippy::unused_unit)]
 #![allow(dead_code)]
+#![allow(clippy::from_over_into)]
 
 struct Unitter;
 impl Unitter {

diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs
@@ -11,6 +11,7 @@
 
 #![deny(clippy::unused_unit)]
 #![allow(dead_code)]
+#![allow(clippy::from_over_into)]
 
 struct Unitter;
 impl Unitter {

diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr
@@ -1,5 +1,5 @@
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:18:28
+  --> $DIR/unused_unit.rs:19:28
    |
 LL |     pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
    |                            ^^^^^^ help: remove the `-> ()`
@@ -11,109 +11,109 @@ LL | #![deny(clippy::unused_unit)]
    |         ^^^^^^^^^^^^^^^^^^^
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:19:18
+  --> $DIR/unused_unit.rs:20:18
    |
 LL |     where G: Fn() -> () {
    |                  ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:18:58
+  --> $DIR/unused_unit.rs:19:58
    |
 LL |     pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
    |                                                          ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:20:26
+  --> $DIR/unused_unit.rs:21:26
    |
 LL |         let _y: &dyn Fn() -> () = &f;
    |                          ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:27:18
+  --> $DIR/unused_unit.rs:28:18
    |
 LL |     fn into(self) -> () {
    |                  ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit expression
-  --> $DIR/unused_unit.rs:28:9
+  --> $DIR/unused_unit.rs:29:9
    |
 LL |         ()
    |         ^^ help: remove the final `()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:33:29
+  --> $DIR/unused_unit.rs:34:29
    |
 LL |     fn redundant<F: FnOnce() -> (), G, H>(&self, _f: F, _g: G, _h: H)
    |                             ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:35:19
+  --> $DIR/unused_unit.rs:36:19
    |
 LL |         G: FnMut() -> (),
    |                   ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:36:16
+  --> $DIR/unused_unit.rs:37:16
    |
 LL |         H: Fn() -> ();
    |                ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:40:29
+  --> $DIR/unused_unit.rs:41:29
    |
 LL |     fn redundant<F: FnOnce() -> (), G, H>(&self, _f: F, _g: G, _h: H)
    |                             ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:42:19
+  --> $DIR/unused_unit.rs:43:19
    |
 LL |         G: FnMut() -> (),
    |                   ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:43:16
+  --> $DIR/unused_unit.rs:44:16
    |
 LL |         H: Fn() -> () {}
    |                ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:46:17
+  --> $DIR/unused_unit.rs:47:17
    |
 LL | fn return_unit() -> () { () }
    |                 ^^^^^^ help: remove the `-> ()`
 
 error: unneeded unit expression
-  --> $DIR/unused_unit.rs:46:26
+  --> $DIR/unused_unit.rs:47:26
    |
 LL | fn return_unit() -> () { () }
    |                          ^^ help: remove the final `()`
 
 error: unneeded `()`
-  --> $DIR/unused_unit.rs:56:14
+  --> $DIR/unused_unit.rs:57:14
    |
 LL |         break();
    |              ^^ help: remove the `()`
 
 error: unneeded `()`
-  --> $DIR/unused_unit.rs:58:11
+  --> $DIR/unused_unit.rs:59:11
    |
 LL |     return();
    |           ^^ help: remove the `()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:75:10
+  --> $DIR/unused_unit.rs:76:10
    |
 LL | fn test()->(){}
    |          ^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:78:11
+  --> $DIR/unused_unit.rs:79:11
    |
 LL | fn test2() ->(){}
    |           ^^^^^ help: remove the `-> ()`
 
 error: unneeded unit return type
-  --> $DIR/unused_unit.rs:81:11
+  --> $DIR/unused_unit.rs:82:11
    |
 LL | fn test3()-> (){}
    |           ^^^^^ help: remove the `-> ()`