Auto merge of #9649 - Alexendoo:from-over-into-suggestion, r=llogiq

bors · bors · commit 50f192f86a12 · 2022-10-15T08:37:11.000Z
Add a suggestion and a note about orphan rules for `from_over_into` Adds a machine applicable suggestion to convert the `Into` impl into a `From` one to `from_over_into` Also adds a note explaining that `impl From<Local> for Foreign` is fine if the `Into` type is foreign Closes #7444 Addresses half of #9638 changelog: [`from_over_into`] Add a suggestion and a note about orphan rules
diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs
@@ -1,11 +1,19 @@
-use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::{meets_msrv, msrvs};
-use if_chain::if_chain;
-use rustc_hir as hir;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::macros::span_is_local;
+use clippy_utils::source::snippet_opt;
+use clippy_utils::{meets_msrv, msrvs, path_def_id};
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{walk_path, Visitor};
+use rustc_hir::{
+    GenericArg, GenericArgs, HirId, Impl, ImplItemKind, ImplItemRef, Item, ItemKind, PatKind, Path, PathSegment, Ty,
+    TyKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::nested_filter::OnlyBodies;
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::symbol::sym;
+use rustc_span::symbol::{kw, sym};
+use rustc_span::{Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -54,28 +62,152 @@ impl FromOverInto {
 impl_lint_pass!(FromOverInto => [FROM_OVER_INTO]);
 
 impl<'tcx> LateLintPass<'tcx> for FromOverInto {
-    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
-        if !meets_msrv(self.msrv, msrvs::RE_REBALANCING_COHERENCE) {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
+        if !meets_msrv(self.msrv, msrvs::RE_REBALANCING_COHERENCE) || !span_is_local(item.span) {
             return;
         }
 
-        if_chain! {
-            if let hir::ItemKind::Impl{ .. } = &item.kind;
-            if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.def_id);
-            if cx.tcx.is_diagnostic_item(sym::Into, impl_trait_ref.def_id);
-
-            then {
-                span_lint_and_help(
-                    cx,
-                    FROM_OVER_INTO,
-                    cx.tcx.sess.source_map().guess_head_span(item.span),
-                    "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true",
-                    None,
-                    &format!("consider to implement `From<{}>` instead", impl_trait_ref.self_ty()),
-                );
-            }
+        if let ItemKind::Impl(Impl {
+            of_trait: Some(hir_trait_ref),
+            self_ty,
+            items: [impl_item_ref],
+            ..
+        }) = item.kind
+            && let Some(into_trait_seg) = hir_trait_ref.path.segments.last()
+            // `impl Into<target_ty> for self_ty`
+            && let Some(GenericArgs { args: [GenericArg::Type(target_ty)], .. }) = into_trait_seg.args
+            && let Some(middle_trait_ref) = cx.tcx.impl_trait_ref(item.def_id)
+            && cx.tcx.is_diagnostic_item(sym::Into, middle_trait_ref.def_id)
+        {
+            span_lint_and_then(
+                cx,
+                FROM_OVER_INTO,
+                cx.tcx.sess.source_map().guess_head_span(item.span),
+                "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true",
+                |diag| {
+                    // If the target type is likely foreign mention the orphan rules as it's a common source of confusion
+                    if path_def_id(cx, target_ty.peel_refs()).map_or(true, |id| !id.is_local()) {
+                        diag.help(
+                            "`impl From<Local> for Foreign` is allowed by the orphan rules, for more information see\n\
+                            https://doc.rust-lang.org/reference/items/implementations.html#trait-implementation-coherence"
+                        );
+                    }
+
+                    let message = format!("replace the `Into` implentation with `From<{}>`", middle_trait_ref.self_ty());
+                    if let Some(suggestions) = convert_to_from(cx, into_trait_seg, target_ty, self_ty, impl_item_ref) {
+                        diag.multipart_suggestion(message, suggestions, Applicability::MachineApplicable);
+                    } else {
+                        diag.help(message);
+                    }
+                },
+            );
         }
     }
 
     extract_msrv_attr!(LateContext);
 }
+
+/// Finds the occurences of `Self` and `self`
+struct SelfFinder<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    /// Occurences of `Self`
+    upper: Vec<Span>,
+    /// Occurences of `self`
+    lower: Vec<Span>,
+    /// If any of the `self`/`Self` usages were from an expansion, or the body contained a binding
+    /// already named `val`
+    invalid: bool,
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for SelfFinder<'a, 'tcx> {
+    type NestedFilter = OnlyBodies;
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.cx.tcx.hir()
+    }
+
+    fn visit_path(&mut self, path: &'tcx Path<'tcx>, _id: HirId) {
+        for segment in path.segments {
+            match segment.ident.name {
+                kw::SelfLower => self.lower.push(segment.ident.span),
+                kw::SelfUpper => self.upper.push(segment.ident.span),
+                _ => continue,
+            }
+        }
+
+        self.invalid |= path.span.from_expansion();
+        if !self.invalid {
+            walk_path(self, path);
+        }
+    }
+
+    fn visit_name(&mut self, name: Symbol) {
+        if name == sym::val {
+            self.invalid = true;
+        }
+    }
+}
+
+fn convert_to_from(
+    cx: &LateContext<'_>,
+    into_trait_seg: &PathSegment<'_>,
+    target_ty: &Ty<'_>,
+    self_ty: &Ty<'_>,
+    impl_item_ref: &ImplItemRef,
+) -> Option<Vec<(Span, String)>> {
+    let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
+    let ImplItemKind::Fn(ref sig, body_id) = impl_item.kind else { return None };
+    let body = cx.tcx.hir().body(body_id);
+    let [input] = body.params else { return None };
+    let PatKind::Binding(.., self_ident, None) = input.pat.kind else { return None };
+
+    let from = snippet_opt(cx, self_ty.span)?;
+    let into = snippet_opt(cx, target_ty.span)?;
+
+    let mut suggestions = vec![
+        // impl Into<T> for U  ->  impl From<T> for U
+        //      ~~~~                    ~~~~
+        (into_trait_seg.ident.span, String::from("From")),
+        // impl Into<T> for U  ->  impl Into<U> for U
+        //           ~                       ~
+        (target_ty.span, from.clone()),
+        // impl Into<T> for U  ->  impl Into<T> for T
+        //                  ~                       ~
+        (self_ty.span, into),
+        // fn into(self) -> T  ->  fn from(self) -> T
+        //    ~~~~                    ~~~~
+        (impl_item.ident.span, String::from("from")),
+        // fn into([mut] self) -> T  ->  fn into([mut] v: T) -> T
+        //               ~~~~                          ~~~~
+        (self_ident.span, format!("val: {from}")),
+        // fn into(self) -> T  ->  fn into(self) -> Self
+        //                  ~                       ~~~~
+        (sig.decl.output.span(), String::from("Self")),
+    ];
+
+    let mut finder = SelfFinder {
+        cx,
+        upper: Vec::new(),
+        lower: Vec::new(),
+        invalid: false,
+    };
+    finder.visit_expr(body.value);
+
+    if finder.invalid {
+        return None;
+    }
+
+    // don't try to replace e.g. `Self::default()` with `&[T]::default()`
+    if !finder.upper.is_empty() && !matches!(self_ty.kind, TyKind::Path(_)) {
+        return None;
+    }
+
+    for span in finder.upper {
+        suggestions.push((span, from.clone()));
+    }
+    for span in finder.lower {
+        suggestions.push((span, String::from("val")));
+    }
+
+    Some(suggestions)
+}
diff --git a/tests/ui/from_over_into.fixed b/tests/ui/from_over_into.fixed
@@ -0,0 +1,62 @@
+// run-rustfix
+
+#![warn(clippy::from_over_into)]
+#![allow(unused)]
+
+// this should throw an error
+struct StringWrapper(String);
+
+impl From<String> for StringWrapper {
+    fn from(val: String) -> Self {
+        StringWrapper(val)
+    }
+}
+
+struct SelfType(String);
+
+impl From<String> for SelfType {
+    fn from(val: String) -> Self {
+        SelfType(String::new())
+    }
+}
+
+#[derive(Default)]
+struct X;
+
+impl X {
+    const FOO: &'static str = "a";
+}
+
+struct SelfKeywords;
+
+impl From<X> for SelfKeywords {
+    fn from(val: X) -> Self {
+        let _ = X::default();
+        let _ = X::FOO;
+        let _: X = val;
+
+        SelfKeywords
+    }
+}
+
+struct ExplicitPaths(bool);
+
+impl core::convert::From<crate::ExplicitPaths> for bool {
+    fn from(mut val: crate::ExplicitPaths) -> Self {
+        let in_closure = || val.0;
+
+        val.0 = false;
+        val.0
+    }
+}
+
+// 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.rs b/tests/ui/from_over_into.rs
@@ -1,4 +1,7 @@
+// run-rustfix
+
 #![warn(clippy::from_over_into)]
+#![allow(unused)]
 
 // this should throw an error
 struct StringWrapper(String);
@@ -9,6 +12,44 @@ impl Into<StringWrapper> for String {
     }
 }
 
+struct SelfType(String);
+
+impl Into<SelfType> for String {
+    fn into(self) -> SelfType {
+        SelfType(Self::new())
+    }
+}
+
+#[derive(Default)]
+struct X;
+
+impl X {
+    const FOO: &'static str = "a";
+}
+
+struct SelfKeywords;
+
+impl Into<SelfKeywords> for X {
+    fn into(self) -> SelfKeywords {
+        let _ = Self::default();
+        let _ = Self::FOO;
+        let _: Self = self;
+
+        SelfKeywords
+    }
+}
+
+struct ExplicitPaths(bool);
+
+impl core::convert::Into<bool> for crate::ExplicitPaths {
+    fn into(mut self) -> bool {
+        let in_closure = || self.0;
+
+        self.0 = false;
+        self.0
+    }
+}
+
 // this is fine
 struct A(String);
 
diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr
@@ -1,11 +1,62 @@
 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
+  --> $DIR/from_over_into.rs:9:1
    |
 LL | impl Into<StringWrapper> for String {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: consider to implement `From<std::string::String>` instead
    = note: `-D clippy::from-over-into` implied by `-D warnings`
+help: replace the `Into` implentation with `From<std::string::String>`
+   |
+LL ~ impl From<String> for StringWrapper {
+LL ~     fn from(val: String) -> Self {
+LL ~         StringWrapper(val)
+   |
+
+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:17:1
+   |
+LL | impl Into<SelfType> for String {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace the `Into` implentation with `From<std::string::String>`
+   |
+LL ~ impl From<String> for SelfType {
+LL ~     fn from(val: String) -> Self {
+LL ~         SelfType(String::new())
+   |
+
+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:32:1
+   |
+LL | impl Into<SelfKeywords> for X {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: replace the `Into` implentation with `From<X>`
+   |
+LL ~ impl From<X> for SelfKeywords {
+LL ~     fn from(val: X) -> Self {
+LL ~         let _ = X::default();
+LL ~         let _ = X::FOO;
+LL ~         let _: X = val;
+   |
+
+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:44:1
+   |
+LL | impl core::convert::Into<bool> for crate::ExplicitPaths {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: `impl From<Local> for Foreign` is allowed by the orphan rules, for more information see
+           https://doc.rust-lang.org/reference/items/implementations.html#trait-implementation-coherence
+help: replace the `Into` implentation with `From<ExplicitPaths>`
+   |
+LL ~ impl core::convert::From<crate::ExplicitPaths> for bool {
+LL ~     fn from(mut val: crate::ExplicitPaths) -> Self {
+LL ~         let in_closure = || val.0;
+LL | 
+LL ~         val.0 = false;
+LL ~         val.0
+   |
 
-error: aborting due to previous error
+error: aborting due to 4 previous errors
 
diff --git a/tests/ui/from_over_into_unfixable.rs b/tests/ui/from_over_into_unfixable.rs
diff --git a/tests/ui/from_over_into_unfixable.stderr b/tests/ui/from_over_into_unfixable.stderr
