From 18520aa8b2e2aa200c5f1756bfd9ab4d3294c995 Mon Sep 17 00:00:00 2001 From: Eduard Burtescu Date: Tue, 7 Apr 2020 19:51:59 +0000 Subject: [PATCH 1/4] Only /usr/bin/env is portable in shebangs. --- setup-toolchain.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup-toolchain.sh b/setup-toolchain.sh index 469cba1874d6..6038ed697f91 100755 --- a/setup-toolchain.sh +++ b/setup-toolchain.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/usr/bin/env bash # Set up the appropriate rustc toolchain set -e From 89e14d201db8c8dc49fab4c1d17d6546b00817ae Mon Sep 17 00:00:00 2001 From: Eduard Burtescu Date: Tue, 7 Apr 2020 19:53:02 +0000 Subject: [PATCH 2/4] use_self: switch to hir_ty_to_ty. --- clippy_lints/src/use_self.rs | 91 ++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 4fcaf3cdb96e..412bebb9bc6e 100644 --- a/clippy_lints/src/use_self.rs +++ b/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 { @@ -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` with `Vec`, + // in an `impl Trait for u8`, when the trait always uses `Vec`. + // 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); From 2ad4d6a057e17c727ab007faed346d83fc1f33d3 Mon Sep 17 00:00:00 2001 From: Eduard Burtescu Date: Tue, 7 Apr 2020 19:53:56 +0000 Subject: [PATCH 3/4] rustup: update for the new Ty::walk interface. --- clippy_lints/src/let_underscore.rs | 10 ++++++++-- clippy_lints/src/methods/mod.rs | 25 ++++++++++++++++--------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/let_underscore.rs b/clippy_lints/src/let_underscore.rs index a68f7edd8370..da83b1f3ce66 100644 --- a/clippy_lints/src/let_underscore.rs +++ b/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,13 @@ 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, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 527508af8a31..124fc1d9878e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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,10 +1495,8 @@ 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; } }, (_, _) => {}, @@ -1497,7 +1504,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { } } - 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, From f5b6a0c54da3c9f94ccd53fae77dbf58fa16e680 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Tue, 7 Apr 2020 22:19:20 +0200 Subject: [PATCH 4/4] Format clippy_lints/src/let_underscore.rs --- clippy_lints/src/let_underscore.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/let_underscore.rs b/clippy_lints/src/let_underscore.rs index da83b1f3ce66..f8f84f3d42d0 100644 --- a/clippy_lints/src/let_underscore.rs +++ b/clippy_lints/src/let_underscore.rs @@ -78,7 +78,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnderscore { then { 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::Type(inner_ty) => { + SYNC_GUARD_PATHS.iter().any(|path| match_type(cx, inner_ty, path)) + }, GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, });