Merge pull request rust-lang#5434 from eddyb/rustup

rustup: update for the new Ty::walk interface.

The first commit fixes a portability bug in `setup-toolchain.sh`, while the second rewrites the handling of "trait impl methods" in `use_self` - even if `Ty::walk` could've still been used, it was IMO a misuse.

This could also serve as a PSA: *please* use `hir_ty_to_ty` instead of trying to compare `hir::Ty`s between themselves or against semantic `Ty`s. Its "quasi-deprecation" is 3 years old and doesn't really mean anything, just that it's currently uncached and that we should eventually querify it (either for a single HIR node, or for all of the nodes in an entire definition).

---

changelog: none

clippy_lints/src/let_underscore.rs

@@ -2,6 +2,7 @@ use if_chain::if_chain;
 use rustc_hir::{Local, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::subst::GenericArgKind;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 use crate::utils::{is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help};
@@ -75,8 +76,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnderscore {
             if let PatKind::Wild = local.pat.kind;
             if let Some(ref init) = local.init;
             then {
-                let check_ty = |ty| SYNC_GUARD_PATHS.iter().any(|path| match_type(cx, ty, path));
-                if cx.tables.expr_ty(init).walk().any(check_ty) {
+                let init_ty = cx.tables.expr_ty(init);
+                let contains_sync_guard = init_ty.walk().any(|inner| match inner.unpack() {
+                    GenericArgKind::Type(inner_ty) => {
+                        SYNC_GUARD_PATHS.iter().any(|path| match_type(cx, inner_ty, path))
+                    },
+
+                    GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
+                });
+                if contains_sync_guard {
                     span_lint_and_help(
                         cx,
                         LET_UNDERSCORE_LOCK,

clippy_lints/src/methods/mod.rs

@@ -15,6 +15,7 @@ use rustc_hir::intravisit::{self, Visitor};
 use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::subst::GenericArgKind;
 use rustc_middle::ty::{self, Predicate, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
@@ -1407,7 +1408,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
         let parent = cx.tcx.hir().get_parent_item(impl_item.hir_id);
         let item = cx.tcx.hir().expect_item(parent);
         let def_id = cx.tcx.hir().local_def_id(item.hir_id);
-        let ty = cx.tcx.type_of(def_id);
+        let self_ty = cx.tcx.type_of(def_id);
         if_chain! {
             if let hir::ImplItemKind::Fn(ref sig, id) = impl_item.kind;
             if let Some(first_arg) = iter_input_pats(&sig.decl, cx.tcx.hir().body(id)).next();
@@ -1429,7 +1430,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
                         if name == method_name &&
                         sig.decl.inputs.len() == n_args &&
                         out_type.matches(cx, &sig.decl.output) &&
-                        self_kind.matches(cx, ty, first_arg_ty) {
+                        self_kind.matches(cx, self_ty, first_arg_ty) {
                             span_lint(cx, SHOULD_IMPLEMENT_TRAIT, impl_item.span, &format!(
                                 "defining a method called `{}` on this type; consider implementing \
                                 the `{}` trait or choosing a less ambiguous name", name, trait_name));
@@ -1441,7 +1442,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
                     .iter()
                     .find(|(ref conv, _)| conv.check(&name))
                 {
-                    if !self_kinds.iter().any(|k| k.matches(cx, ty, first_arg_ty)) {
+                    if !self_kinds.iter().any(|k| k.matches(cx, self_ty, first_arg_ty)) {
                         let lint = if item.vis.node.is_pub() {
                             WRONG_PUB_SELF_CONVENTION
                         } else {
@@ -1471,8 +1472,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
         if let hir::ImplItemKind::Fn(_, _) = impl_item.kind {
             let ret_ty = return_ty(cx, impl_item.hir_id);
 
+            let contains_self_ty = |ty: Ty<'tcx>| {
+                ty.walk().any(|inner| match inner.unpack() {
+                    GenericArgKind::Type(inner_ty) => same_tys(cx, self_ty, inner_ty),
+
+                    GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
+                })
+            };
+
             // walk the return type and check for Self (this does not check associated types)
-            if ret_ty.walk().any(|inner_type| same_tys(cx, ty, inner_type)) {
+            if contains_self_ty(ret_ty) {
                 return;
             }
 
@@ -1486,18 +1495,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
                             let associated_type = binder.skip_binder();
 
                             // walk the associated type and check for Self
-                            for inner_type in associated_type.walk() {
-                                if same_tys(cx, ty, inner_type) {
-                                    return;
-                                }
+                            if contains_self_ty(associated_type) {
+                                return;
                             }
                         },
                         (_, _) => {},
                     }
                 }
             }
 
-            if name == "new" && !same_tys(cx, ret_ty, ty) {
+            if name == "new" && !same_tys(cx, ret_ty, self_ty) {
                 span_lint(
                     cx,
                     NEW_RET_NO_SELF,

clippy_lints/src/use_self.rs

@@ -14,6 +14,7 @@ use rustc_middle::ty;
 use rustc_middle::ty::{DefIdTree, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::kw;
+use rustc_typeck::hir_ty_to_ty;
 
 use crate::utils::{differing_macro_contexts, span_lint_and_sugg};
 
@@ -80,37 +81,28 @@ fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path<'_>, last_segment: O
     );
 }
 
-struct TraitImplTyVisitor<'a, 'tcx> {
-    item_type: Ty<'tcx>,
+// FIXME: always use this (more correct) visitor, not just in method signatures.
+struct SemanticUseSelfVisitor<'a, 'tcx> {
     cx: &'a LateContext<'a, 'tcx>,
-    trait_type_walker: ty::walk::TypeWalker<'tcx>,
-    impl_type_walker: ty::walk::TypeWalker<'tcx>,
+    self_ty: Ty<'tcx>,
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> {
+impl<'a, 'tcx> Visitor<'tcx> for SemanticUseSelfVisitor<'a, 'tcx> {
     type Map = Map<'tcx>;
 
-    fn visit_ty(&mut self, t: &'tcx hir::Ty<'_>) {
-        let trait_ty = self.trait_type_walker.next();
-        let impl_ty = self.impl_type_walker.next();
-
-        if_chain! {
-            if let TyKind::Path(QPath::Resolved(_, path)) = &t.kind;
-
-            // The implementation and trait types don't match which means that
-            // the concrete type was specified by the implementation
-            if impl_ty != trait_ty;
-            if let Some(impl_ty) = impl_ty;
-            if self.item_type == impl_ty;
-            then {
-                match path.res {
-                    def::Res::SelfTy(..) => {},
-                    _ => span_use_self_lint(self.cx, path, None)
-                }
+    fn visit_ty(&mut self, hir_ty: &'tcx hir::Ty<'_>) {
+        if let TyKind::Path(QPath::Resolved(_, path)) = &hir_ty.kind {
+            match path.res {
+                def::Res::SelfTy(..) => {},
+                _ => {
+                    if hir_ty_to_ty(self.cx.tcx, hir_ty) == self.self_ty {
+                        span_use_self_lint(self.cx, path, None);
+                    }
+                },
             }
         }
 
-        walk_ty(self, t)
+        walk_ty(self, hir_ty)
     }
 
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
@@ -120,10 +112,9 @@ impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> {
 
 fn check_trait_method_impl_decl<'a, 'tcx>(
     cx: &'a LateContext<'a, 'tcx>,
-    item_type: Ty<'tcx>,
     impl_item: &ImplItem<'_>,
     impl_decl: &'tcx FnDecl<'_>,
-    impl_trait_ref: &ty::TraitRef<'_>,
+    impl_trait_ref: ty::TraitRef<'tcx>,
 ) {
     let trait_method = cx
         .tcx
@@ -134,34 +125,35 @@ fn check_trait_method_impl_decl<'a, 'tcx>(
     let trait_method_sig = cx.tcx.fn_sig(trait_method.def_id);
     let trait_method_sig = cx.tcx.erase_late_bound_regions(&trait_method_sig);
 
-    let impl_method_def_id = cx.tcx.hir().local_def_id(impl_item.hir_id);
-    let impl_method_sig = cx.tcx.fn_sig(impl_method_def_id);
-    let impl_method_sig = cx.tcx.erase_late_bound_regions(&impl_method_sig);
-
-    let output_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
+    let output_hir_ty = if let FnRetTy::Return(ty) = &impl_decl.output {
         Some(&**ty)
     } else {
         None
     };
 
-    // `impl_decl_ty` (of type `hir::Ty`) represents the type declared in the signature.
-    // `impl_ty` (of type `ty:TyS`) is the concrete type that the compiler has determined for
-    // that declaration. We use `impl_decl_ty` to see if the type was declared as `Self`
-    // and use `impl_ty` to check its concrete type.
-    for (impl_decl_ty, (impl_ty, trait_ty)) in impl_decl.inputs.iter().chain(output_ty).zip(
-        impl_method_sig
-            .inputs_and_output
-            .iter()
-            .zip(trait_method_sig.inputs_and_output),
-    ) {
-        let mut visitor = TraitImplTyVisitor {
-            cx,
-            item_type,
-            trait_type_walker: trait_ty.walk(),
-            impl_type_walker: impl_ty.walk(),
-        };
-
-        visitor.visit_ty(&impl_decl_ty);
+    // `impl_hir_ty` (of type `hir::Ty`) represents the type written in the signature.
+    // `trait_ty` (of type `ty::Ty`) is the semantic type for the signature in the trait.
+    // We use `impl_hir_ty` to see if the type was written as `Self`,
+    // `hir_ty_to_ty(...)` to check semantic types of paths, and
+    // `trait_ty` to determine which parts of the signature in the trait, mention
+    // the type being implemented verbatim (as opposed to `Self`).
+    for (impl_hir_ty, trait_ty) in impl_decl
+        .inputs
+        .iter()
+        .chain(output_hir_ty)
+        .zip(trait_method_sig.inputs_and_output)
+    {
+        // Check if the input/output type in the trait method specifies the implemented
+        // type verbatim, and only suggest `Self` if that isn't the case.
+        // This avoids suggestions to e.g. replace `Vec<u8>` with `Vec<Self>`,
+        // in an `impl Trait for u8`, when the trait always uses `Vec<u8>`.
+        // See also https://github.com/rust-lang/rust-clippy/issues/2894.
+        let self_ty = impl_trait_ref.self_ty();
+        if !trait_ty.walk().any(|inner| inner == self_ty.into()) {
+            let mut visitor = SemanticUseSelfVisitor { cx, self_ty };
+
+            visitor.visit_ty(&impl_hir_ty);
+        }
     }
 }
 
@@ -197,8 +189,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UseSelf {
                             let impl_item = cx.tcx.hir().impl_item(impl_item_ref.id);
                             if let ImplItemKind::Fn(FnSig{ decl: impl_decl, .. }, impl_body_id)
                                     = &impl_item.kind {
-                                let item_type = cx.tcx.type_of(impl_def_id);
-                                check_trait_method_impl_decl(cx, item_type, impl_item, impl_decl, &impl_trait_ref);
+                                check_trait_method_impl_decl(cx, impl_item, impl_decl, impl_trait_ref);
 
                                 let body = cx.tcx.hir().body(*impl_body_id);
                                 visitor.visit_body(body);

setup-toolchain.sh

@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
 # Set up the appropriate rustc toolchain
 
 set -e