From 040735c110026bbd494a23c86182ebda201d720b Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 10:07:50 +0100 Subject: [PATCH 01/20] First version of noop-lint --- compiler/rustc_lint/src/lib.rs | 3 + compiler/rustc_lint/src/noop_method_call.rs | 71 +++++++++++++++++++++ compiler/rustc_span/src/symbol.rs | 1 + library/core/src/clone.rs | 2 + 4 files changed, 77 insertions(+) create mode 100644 compiler/rustc_lint/src/noop_method_call.rs diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 638b73c27a8d7..b0c22154c0b5b 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -57,6 +57,7 @@ mod methods; mod non_ascii_idents; mod non_fmt_panic; mod nonstandard_style; +mod noop_method_call; mod passes; mod redundant_semicolon; mod traits; @@ -83,6 +84,7 @@ use methods::*; use non_ascii_idents::*; use non_fmt_panic::NonPanicFmt; use nonstandard_style::*; +use noop_method_call::*; use redundant_semicolon::*; use traits::*; use types::*; @@ -170,6 +172,7 @@ macro_rules! late_lint_passes { DropTraitConstraints: DropTraitConstraints, TemporaryCStringAsPtr: TemporaryCStringAsPtr, NonPanicFmt: NonPanicFmt, + NoopMethodCall: NoopMethodCall, ] ); }; diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs new file mode 100644 index 0000000000000..098c50d5abe19 --- /dev/null +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -0,0 +1,71 @@ +use crate::context::LintContext; +use crate::rustc_middle::ty::TypeFoldable; +use crate::LateContext; +use crate::LateLintPass; +use rustc_hir::def::DefKind; +use rustc_hir::{Expr, ExprKind}; +use rustc_middle::ty; +use rustc_span::symbol::sym; + +declare_lint! { + /// The `noop_method_call` lint detects specific calls to noop methods + /// such as a calling `<&T as Clone>::clone` where `T: !Clone`. + /// + /// ### Example + /// + /// ```rust + /// # #![allow(unused)] + /// struct Foo; + /// let foo = &Foo; + /// let clone: &Foo = foo.clone(); + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Some method calls are noops meaning that they do nothing. Usually such methods + /// are the result of blanket implementations that happen to create some method invocations + /// that end up not doing anything. For instance, `Clone` is implemented on all `&T`, but + /// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything + /// as references are copy. This lint detects these calls and warns the user about them. + pub NOOP_METHOD_CALL, + Warn, + "detects the use of well-known noop methods" +} + +declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]); + +impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // We only care about method calls + if let ExprKind::MethodCall(..) = expr.kind { + // Get the `DefId` only when dealing with an `AssocFn` + if let Some((DefKind::AssocFn, did)) = + cx.typeck_results().type_dependent_def(expr.hir_id) + { + // Check that we're dealing with a trait method + if let Some(trait_id) = cx.tcx.trait_of_item(did) { + let substs = cx.typeck_results().node_substs(expr.hir_id); + // We can't resolve on types that recursively require monomorphization, + // so check that we don't need to perfom substitution + if !substs.needs_subst() { + let param_env = cx.tcx.param_env(trait_id); + // Resolve the trait method instance + if let Ok(Some(i)) = ty::Instance::resolve(cx.tcx, param_env, did, substs) { + // Check that it implements the noop diagnostic + if cx.tcx.is_diagnostic_item(sym::ref_clone_method, i.def_id()) { + let span = expr.span; + + cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { + let message = "Call to noop method"; + lint.build(&message).emit() + }); + } + } + } + } + } + } + } +} diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 27bb45bcc8512..2207deb8fb5a0 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -915,6 +915,7 @@ symbols! { receiver, recursion_limit, reexport_test_harness_main, + ref_clone_method, reference, reflect, reg, diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index a953a3a4182bc..12f0f9629a31f 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -104,6 +104,7 @@ /// [impls]: #implementors #[stable(feature = "rust1", since = "1.0.0")] #[lang = "clone"] +#[rustc_diagnostic_item = "Clone"] pub trait Clone: Sized { /// Returns a copy of the value. /// @@ -221,6 +222,7 @@ mod impls { #[stable(feature = "rust1", since = "1.0.0")] impl Clone for &T { #[inline] + #[rustc_diagnostic_item = "ref_clone_method"] fn clone(&self) -> Self { *self } From f49ed7a6b7aa3a44dd0444b508a1d0ddc09b0f15 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 16:14:39 +0100 Subject: [PATCH 02/20] Add tests and support two more noop methods --- compiler/rustc_lint/src/noop_method_call.rs | 12 +++++- compiler/rustc_span/src/symbol.rs | 4 +- library/core/src/borrow.rs | 1 + library/core/src/clone.rs | 3 +- library/core/src/ops/deref.rs | 1 + src/test/ui/lint/noop-method-call.rs | 45 +++++++++++++++++++++ src/test/ui/lint/noop-method-call.stderr | 28 +++++++++++++ 7 files changed, 89 insertions(+), 5 deletions(-) create mode 100644 src/test/ui/lint/noop-method-call.rs create mode 100644 src/test/ui/lint/noop-method-call.stderr diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 098c50d5abe19..3ab3fab4272c7 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -54,11 +54,19 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { // Resolve the trait method instance if let Ok(Some(i)) = ty::Instance::resolve(cx.tcx, param_env, did, substs) { // Check that it implements the noop diagnostic - if cx.tcx.is_diagnostic_item(sym::ref_clone_method, i.def_id()) { + tracing::debug!("Resolves to: {:?}", i.def_id()); + if [ + sym::noop_method_borrow, + sym::noop_method_clone, + sym::noop_method_deref, + ] + .iter() + .any(|s| cx.tcx.is_diagnostic_item(*s, i.def_id())) + { let span = expr.span; cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { - let message = "Call to noop method"; + let message = "call to noop method"; lint.build(&message).emit() }); } diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 2207deb8fb5a0..d39fbd61962e7 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -789,6 +789,9 @@ symbols! { none_error, nontemporal_store, nontrapping_dash_fptoint: "nontrapping-fptoint", + noop_method_borrow, + noop_method_clone, + noop_method_deref, noreturn, nostack, not, @@ -915,7 +918,6 @@ symbols! { receiver, recursion_limit, reexport_test_harness_main, - ref_clone_method, reference, reflect, reg, diff --git a/library/core/src/borrow.rs b/library/core/src/borrow.rs index c9040cd0a1670..af2ad12dddf0a 100644 --- a/library/core/src/borrow.rs +++ b/library/core/src/borrow.rs @@ -219,6 +219,7 @@ impl BorrowMut for T { #[stable(feature = "rust1", since = "1.0.0")] impl Borrow for &T { + #[rustc_diagnostic_item = "noop_method_borrow"] fn borrow(&self) -> &T { &**self } diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index 12f0f9629a31f..12b4feeb14ae3 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -104,7 +104,6 @@ /// [impls]: #implementors #[stable(feature = "rust1", since = "1.0.0")] #[lang = "clone"] -#[rustc_diagnostic_item = "Clone"] pub trait Clone: Sized { /// Returns a copy of the value. /// @@ -222,7 +221,7 @@ mod impls { #[stable(feature = "rust1", since = "1.0.0")] impl Clone for &T { #[inline] - #[rustc_diagnostic_item = "ref_clone_method"] + #[rustc_diagnostic_item = "noop_method_clone"] fn clone(&self) -> Self { *self } diff --git a/library/core/src/ops/deref.rs b/library/core/src/ops/deref.rs index 2419771eae212..d503a30174191 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -78,6 +78,7 @@ pub trait Deref { impl Deref for &T { type Target = T; + #[rustc_diagnostic_item = "noop_method_deref"] fn deref(&self) -> &T { *self } diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs new file mode 100644 index 0000000000000..4b81e04d3f7f9 --- /dev/null +++ b/src/test/ui/lint/noop-method-call.rs @@ -0,0 +1,45 @@ +// check-pass + +#![allow(unused)] + +use std::borrow::Borrow; +use std::ops::Deref; + +struct Foo(T); + +#[derive(Clone)] +struct Bar(T); + +struct DerefExample(T); + +impl Deref for DerefExample { + type Target = T; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +fn main() { + let foo = &Foo(1u32); + let foo_clone: &Foo = foo.clone(); //~ WARNING call to noop method + + let bar = &Bar(1u32); + let bar_clone: Bar = bar.clone(); + + let deref = &&DerefExample(12u32); + let derefed: &DerefExample = deref.deref(); //~ WARNING call to noop method + + let deref = &DerefExample(12u32); + let derefed: &u32 = deref.deref(); + + let a = &&Foo(1u32); + let borrowed: &Foo = a.borrow(); //~ WARNING call to noop method +} + +fn generic(foo: &Foo) { + foo.clone(); +} + +fn non_generic(foo: &Foo) { + foo.clone(); //~ WARNING call to noop method +} diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr new file mode 100644 index 0000000000000..1120adee121b1 --- /dev/null +++ b/src/test/ui/lint/noop-method-call.stderr @@ -0,0 +1,28 @@ +warning: call to noop method + --> $DIR/noop-method-call.rs:24:32 + | +LL | let foo_clone: &Foo = foo.clone(); + | ^^^^^^^^^^^ + | + = note: `#[warn(noop_method_call)]` on by default + +warning: call to noop method + --> $DIR/noop-method-call.rs:30:39 + | +LL | let derefed: &DerefExample = deref.deref(); + | ^^^^^^^^^^^^^ + +warning: call to noop method + --> $DIR/noop-method-call.rs:36:31 + | +LL | let borrowed: &Foo = a.borrow(); + | ^^^^^^^^^^ + +warning: call to noop method + --> $DIR/noop-method-call.rs:44:5 + | +LL | foo.clone(); + | ^^^^^^^^^^^ + +warning: 4 warnings emitted + From a6d926d80db4a52398dea0cf29e6c501eab50170 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 16:46:50 +0100 Subject: [PATCH 03/20] Fix tests --- compiler/rustc_codegen_ssa/src/back/rpath.rs | 2 +- compiler/rustc_lint/src/noop_method_call.rs | 9 ++++++++- compiler/rustc_middle/src/ty/error.rs | 2 -- compiler/rustc_mir/src/borrow_check/invalidation.rs | 8 ++++---- compiler/rustc_mir_build/src/build/matches/test.rs | 2 +- compiler/rustc_span/src/symbol.rs | 2 ++ .../src/traits/error_reporting/mod.rs | 2 +- compiler/rustc_traits/src/chalk/mod.rs | 2 +- compiler/rustc_typeck/src/check/callee.rs | 2 +- compiler/rustc_typeck/src/check/expr.rs | 2 +- library/core/src/borrow.rs | 1 + library/core/src/clone.rs | 1 + library/core/src/ops/deref.rs | 1 + src/test/ui/issues/issue-11820.rs | 10 ++++++---- src/test/ui/underscore-imports/cycle.rs | 1 + src/test/ui/underscore-imports/hygiene-2.rs | 1 + src/test/ui/underscore-imports/macro-expanded.rs | 1 + 17 files changed, 32 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/rpath.rs b/compiler/rustc_codegen_ssa/src/back/rpath.rs index 005d2efdd3b26..5f21046b05e47 100644 --- a/compiler/rustc_codegen_ssa/src/back/rpath.rs +++ b/compiler/rustc_codegen_ssa/src/back/rpath.rs @@ -24,7 +24,7 @@ pub fn get_rpath_flags(config: &mut RPathConfig<'_>) -> Vec { debug!("preparing the RPATH!"); - let libs = config.used_crates.clone(); + let libs = config.used_crates; let libs = libs.iter().filter_map(|&(_, ref l)| l.option()).collect::>(); let rpaths = get_rpaths(config, &libs); let mut flags = rpaths_to_flags(&rpaths); diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 3ab3fab4272c7..dad557128f8b7 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -46,6 +46,14 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { { // Check that we're dealing with a trait method if let Some(trait_id) = cx.tcx.trait_of_item(did) { + // Check we're dealing with one of the traits we care about + if ![sym::Clone, sym::Deref, sym::Borrow] + .iter() + .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) + { + return; + } + let substs = cx.typeck_results().node_substs(expr.hir_id); // We can't resolve on types that recursively require monomorphization, // so check that we don't need to perfom substitution @@ -54,7 +62,6 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { // Resolve the trait method instance if let Ok(Some(i)) = ty::Instance::resolve(cx.tcx, param_env, did, substs) { // Check that it implements the noop diagnostic - tracing::debug!("Resolves to: {:?}", i.def_id()); if [ sym::noop_method_borrow, sym::noop_method_clone, diff --git a/compiler/rustc_middle/src/ty/error.rs b/compiler/rustc_middle/src/ty/error.rs index bf315c81588a9..f19cc99844926 100644 --- a/compiler/rustc_middle/src/ty/error.rs +++ b/compiler/rustc_middle/src/ty/error.rs @@ -12,7 +12,6 @@ use rustc_target::spec::abi; use std::borrow::Cow; use std::fmt; -use std::ops::Deref; #[derive(Clone, Copy, Debug, PartialEq, Eq, TypeFoldable)] pub struct ExpectedFound { @@ -548,7 +547,6 @@ impl Trait for X { TargetFeatureCast(def_id) => { let attrs = self.get_attrs(*def_id); let target_spans = attrs - .deref() .iter() .filter(|attr| attr.has_name(sym::target_feature)) .map(|attr| attr.span); diff --git a/compiler/rustc_mir/src/borrow_check/invalidation.rs b/compiler/rustc_mir/src/borrow_check/invalidation.rs index 8c05e6fd5d0e4..e423e449746fc 100644 --- a/compiler/rustc_mir/src/borrow_check/invalidation.rs +++ b/compiler/rustc_mir/src/borrow_check/invalidation.rs @@ -165,7 +165,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> { self.consume_operand(location, value); // Invalidate all borrows of local places - let borrow_set = self.borrow_set.clone(); + let borrow_set = self.borrow_set; let resume = self.location_table.start_index(resume.start_location()); for (i, data) in borrow_set.iter_enumerated() { if borrow_of_local_data(data.borrowed_place) { @@ -177,7 +177,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> { } TerminatorKind::Resume | TerminatorKind::Return | TerminatorKind::GeneratorDrop => { // Invalidate all borrows of local places - let borrow_set = self.borrow_set.clone(); + let borrow_set = self.borrow_set; let start = self.location_table.start_index(location); for (i, data) in borrow_set.iter_enumerated() { if borrow_of_local_data(data.borrowed_place) { @@ -369,7 +369,7 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> { ); let tcx = self.tcx; let body = self.body; - let borrow_set = self.borrow_set.clone(); + let borrow_set = self.borrow_set; let indices = self.borrow_set.indices(); each_borrow_involving_path( self, @@ -377,7 +377,7 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> { body, location, (sd, place), - &borrow_set.clone(), + borrow_set, indices, |this, borrow_index, borrow| { match (rw, borrow.kind) { diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 126fb957a6a99..4db7debee7e8f 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -51,7 +51,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { PatKind::Constant { value } => Test { span: match_pair.pattern.span, - kind: TestKind::Eq { value, ty: match_pair.pattern.ty.clone() }, + kind: TestKind::Eq { value, ty: match_pair.pattern.ty }, }, PatKind::Range(range) => { diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index d39fbd61962e7..61f9a080a5217 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -129,6 +129,7 @@ symbols! { BTreeMap, BTreeSet, BinaryHeap, + Borrow, C, CString, Center, @@ -141,6 +142,7 @@ symbols! { Decodable, Decoder, Default, + Deref, Encodable, Encoder, Eq, diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index bfb5ebcea58b1..a3faf4cb7d4c1 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -819,7 +819,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { sig.decl .inputs .iter() - .map(|arg| match arg.clone().kind { + .map(|arg| match arg.kind { hir::TyKind::Tup(ref tys) => ArgKind::Tuple( Some(arg.span), vec![("_".to_owned(), "_".to_owned()); tys.len()], diff --git a/compiler/rustc_traits/src/chalk/mod.rs b/compiler/rustc_traits/src/chalk/mod.rs index d98f18182c843..b7275bac19048 100644 --- a/compiler/rustc_traits/src/chalk/mod.rs +++ b/compiler/rustc_traits/src/chalk/mod.rs @@ -165,7 +165,7 @@ crate fn evaluate_goal<'tcx>( // let's just ignore that let sol = Canonical { max_universe: ty::UniverseIndex::from_usize(0), - variables: obligation.variables.clone(), + variables: obligation.variables, value: QueryResponse { var_values: CanonicalVarValues { var_values: IndexVec::new() } .make_identity(tcx), diff --git a/compiler/rustc_typeck/src/check/callee.rs b/compiler/rustc_typeck/src/check/callee.rs index ca1e79fac73e9..6ef63bcbbbf21 100644 --- a/compiler/rustc_typeck/src/check/callee.rs +++ b/compiler/rustc_typeck/src/check/callee.rs @@ -465,7 +465,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let expected_arg_tys = self.expected_inputs_for_expected_output( call_expr.span, expected, - fn_sig.output().clone(), + fn_sig.output(), fn_sig.inputs(), ); diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs index 2faf128c491fd..48740e533da8e 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -711,7 +711,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }); let ret_ty = ret_coercion.borrow().expected_ty(); - let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty.clone()); + let return_expr_ty = self.check_expr_with_hint(return_expr, ret_ty); ret_coercion.borrow_mut().coerce( self, &self.cause(return_expr.span, ObligationCauseCode::ReturnValue(return_expr.hir_id)), diff --git a/library/core/src/borrow.rs b/library/core/src/borrow.rs index af2ad12dddf0a..a0cdf681f67e1 100644 --- a/library/core/src/borrow.rs +++ b/library/core/src/borrow.rs @@ -153,6 +153,7 @@ /// [`HashMap`]: ../../std/collections/struct.HashMap.html /// [`String`]: ../../std/string/struct.String.html #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Borrow"] pub trait Borrow { /// Immutably borrows from an owned value. /// diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index 12b4feeb14ae3..957769cdc5a62 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -104,6 +104,7 @@ /// [impls]: #implementors #[stable(feature = "rust1", since = "1.0.0")] #[lang = "clone"] +#[rustc_diagnostic_item = "Clone"] pub trait Clone: Sized { /// Returns a copy of the value. /// diff --git a/library/core/src/ops/deref.rs b/library/core/src/ops/deref.rs index d503a30174191..10e3ce67448c8 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -60,6 +60,7 @@ #[doc(alias = "*")] #[doc(alias = "&*")] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Deref"] pub trait Deref { /// The resulting type after dereferencing. #[stable(feature = "rust1", since = "1.0.0")] diff --git a/src/test/ui/issues/issue-11820.rs b/src/test/ui/issues/issue-11820.rs index 7ffe9652797cf..8a26624a05dc9 100644 --- a/src/test/ui/issues/issue-11820.rs +++ b/src/test/ui/issues/issue-11820.rs @@ -1,12 +1,14 @@ // run-pass // pretty-expanded FIXME #23616 +#![allow(noop_method_call)] + struct NoClone; fn main() { - let rnc = &NoClone; - let rsnc = &Some(NoClone); + let rnc = &NoClone; + let rsnc = &Some(NoClone); - let _: &NoClone = rnc.clone(); - let _: &Option = rsnc.clone(); + let _: &NoClone = rnc.clone(); + let _: &Option = rsnc.clone(); } diff --git a/src/test/ui/underscore-imports/cycle.rs b/src/test/ui/underscore-imports/cycle.rs index bacf9b2d5a96a..987410fa84b20 100644 --- a/src/test/ui/underscore-imports/cycle.rs +++ b/src/test/ui/underscore-imports/cycle.rs @@ -14,5 +14,6 @@ mod y { pub fn main() { use x::*; + #[allow(noop_method_call)] (&0).deref(); } diff --git a/src/test/ui/underscore-imports/hygiene-2.rs b/src/test/ui/underscore-imports/hygiene-2.rs index bea61eae6b51a..510d91d0d4624 100644 --- a/src/test/ui/underscore-imports/hygiene-2.rs +++ b/src/test/ui/underscore-imports/hygiene-2.rs @@ -29,5 +29,6 @@ m!(y); fn main() { use crate::y::*; + #[allow(noop_method_call)] (&()).deref(); } diff --git a/src/test/ui/underscore-imports/macro-expanded.rs b/src/test/ui/underscore-imports/macro-expanded.rs index 43f527bc9a408..55e86e848558d 100644 --- a/src/test/ui/underscore-imports/macro-expanded.rs +++ b/src/test/ui/underscore-imports/macro-expanded.rs @@ -3,6 +3,7 @@ // check-pass #![feature(decl_macro, rustc_attrs)] +#![allow(noop_method_call)] mod x { pub use std::ops::Not as _; From 618c395a8da1e284a2963b8da02450eb162e0478 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 14:48:06 +0100 Subject: [PATCH 04/20] Bless test where order of error message changed --- src/test/ui/panic-handler/weak-lang-item.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/panic-handler/weak-lang-item.stderr b/src/test/ui/panic-handler/weak-lang-item.stderr index 68e3e21df3e08..b7c040c7a850b 100644 --- a/src/test/ui/panic-handler/weak-lang-item.stderr +++ b/src/test/ui/panic-handler/weak-lang-item.stderr @@ -10,10 +10,10 @@ help: you can use `as` to change the binding name of the import LL | extern crate core as other_core; | -error: language item required, but not found: `eh_personality` - error: `#[panic_handler]` function required, but not found +error: language item required, but not found: `eh_personality` + error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0259`. From 3a86184777e98935bef9b9662d090d6886c3c041 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 17:56:34 +0100 Subject: [PATCH 05/20] Fix ui-full-deps suite --- library/alloc/src/collections/btree/map/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/alloc/src/collections/btree/map/tests.rs b/library/alloc/src/collections/btree/map/tests.rs index 4e48db7f49305..e636e490e1bf4 100644 --- a/library/alloc/src/collections/btree/map/tests.rs +++ b/library/alloc/src/collections/btree/map/tests.rs @@ -1801,11 +1801,11 @@ fn test_occupied_entry_key() { let key = "hello there"; let value = "value goes here"; assert!(a.is_empty()); - a.insert(key.clone(), value.clone()); + a.insert(key, value); assert_eq!(a.len(), 1); assert_eq!(a[key], value); - match a.entry(key.clone()) { + match a.entry(key) { Vacant(_) => panic!(), Occupied(e) => assert_eq!(key, *e.key()), } @@ -1821,11 +1821,11 @@ fn test_vacant_entry_key() { let value = "value goes here"; assert!(a.is_empty()); - match a.entry(key.clone()) { + match a.entry(key) { Occupied(_) => panic!(), Vacant(e) => { assert_eq!(key, *e.key()); - e.insert(value.clone()); + e.insert(value); } } assert_eq!(a.len(), 1); From ee65416f0d50331b5fef4360ce36538028edebf9 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 21:11:08 +0100 Subject: [PATCH 06/20] Fix core tests --- library/core/src/clone.rs | 1 + library/core/tests/clone.rs | 2 ++ library/core/tests/iter/adapters/intersperse.rs | 4 ++-- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/library/core/src/clone.rs b/library/core/src/clone.rs index 957769cdc5a62..51a2dc03de318 100644 --- a/library/core/src/clone.rs +++ b/library/core/src/clone.rs @@ -111,6 +111,7 @@ pub trait Clone: Sized { /// # Examples /// /// ``` + /// # #![allow(noop_method_call)] /// let hello = "Hello"; // &str implements Clone /// /// assert_eq!("Hello", hello.clone()); diff --git a/library/core/tests/clone.rs b/library/core/tests/clone.rs index c97a87aebce41..e5787d79c3226 100644 --- a/library/core/tests/clone.rs +++ b/library/core/tests/clone.rs @@ -1,3 +1,5 @@ +#![allow(noop_method_call)] + #[test] fn test_borrowed_clone() { let x = 5; diff --git a/library/core/tests/iter/adapters/intersperse.rs b/library/core/tests/iter/adapters/intersperse.rs index 9dbe232e4eec8..b336c03b5adbe 100644 --- a/library/core/tests/iter/adapters/intersperse.rs +++ b/library/core/tests/iter/adapters/intersperse.rs @@ -9,7 +9,7 @@ fn test_intersperse() { assert_eq!(v, vec![1]); let xs = ["a", "", "b", "c"]; - let v: Vec<&str> = xs.iter().map(|x| x.clone()).intersperse(", ").collect(); + let v: Vec<&str> = xs.iter().map(|x| *x).intersperse(", ").collect(); let text: String = v.concat(); assert_eq!(text, "a, , b, c".to_string()); @@ -24,7 +24,7 @@ fn test_intersperse_size_hint() { assert_eq!(iter.size_hint(), (0, Some(0))); let xs = ["a", "", "b", "c"]; - let mut iter = xs.iter().map(|x| x.clone()).intersperse(", "); + let mut iter = xs.iter().map(|x| *x).intersperse(", "); assert_eq!(iter.size_hint(), (7, Some(7))); assert_eq!(iter.next(), Some("a")); From d3b49c2ed2c7bf35e6155ea25acbf657d8224eac Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Thu, 7 Jan 2021 11:24:32 +0100 Subject: [PATCH 07/20] Only allow new lint when not bootstrapping - since beta doesn't know about the lint --- library/core/tests/clone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/tests/clone.rs b/library/core/tests/clone.rs index e5787d79c3226..2f2aa9a20f912 100644 --- a/library/core/tests/clone.rs +++ b/library/core/tests/clone.rs @@ -1,4 +1,4 @@ -#![allow(noop_method_call)] +#![cfg_attr(not(bootstrap), allow(noop_method_call))] #[test] fn test_borrowed_clone() { From c5ff54cbdb6b0da742110b416cdbcf0ca0ff0fc4 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Thu, 7 Jan 2021 13:13:25 +0100 Subject: [PATCH 08/20] Fix std tests --- library/std/src/collections/hash/map/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/std/src/collections/hash/map/tests.rs b/library/std/src/collections/hash/map/tests.rs index 467968354e25d..819be14222752 100644 --- a/library/std/src/collections/hash/map/tests.rs +++ b/library/std/src/collections/hash/map/tests.rs @@ -774,11 +774,11 @@ fn test_occupied_entry_key() { let key = "hello there"; let value = "value goes here"; assert!(a.is_empty()); - a.insert(key.clone(), value.clone()); + a.insert(key, value); assert_eq!(a.len(), 1); assert_eq!(a[key], value); - match a.entry(key.clone()) { + match a.entry(key) { Vacant(_) => panic!(), Occupied(e) => assert_eq!(key, *e.key()), } @@ -793,11 +793,11 @@ fn test_vacant_entry_key() { let value = "value goes here"; assert!(a.is_empty()); - match a.entry(key.clone()) { + match a.entry(key) { Occupied(_) => panic!(), Vacant(e) => { assert_eq!(key, *e.key()); - e.insert(value.clone()); + e.insert(value); } } assert_eq!(a.len(), 1); From 217c88655b1155796739edbf415e7ce37d30830b Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 8 Jan 2021 11:37:52 +0100 Subject: [PATCH 09/20] Improve warning --- compiler/rustc_lint/src/noop_method_call.rs | 10 ++++++---- src/test/ui/lint/noop-method-call.rs | 8 ++++---- src/test/ui/lint/noop-method-call.stderr | 16 ++++++++-------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index dad557128f8b7..b9b5009d9dd95 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -70,11 +70,13 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { .iter() .any(|s| cx.tcx.is_diagnostic_item(*s, i.def_id())) { - let span = expr.span; + let expr_span = expr.span; - cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { - let message = "call to noop method"; - lint.build(&message).emit() + cx.struct_span_lint(NOOP_METHOD_CALL, expr_span, |lint| { + let message = "call to method that does nothing"; + lint.build(&message) + .span_label(expr_span, "unnecessary method call") + .emit() }); } } diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index 4b81e04d3f7f9..b8ff75845bd7d 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -21,19 +21,19 @@ impl Deref for DerefExample { fn main() { let foo = &Foo(1u32); - let foo_clone: &Foo = foo.clone(); //~ WARNING call to noop method + let foo_clone: &Foo = foo.clone(); //~ WARNING call to method that does nothing [noop_method_call] let bar = &Bar(1u32); let bar_clone: Bar = bar.clone(); let deref = &&DerefExample(12u32); - let derefed: &DerefExample = deref.deref(); //~ WARNING call to noop method + let derefed: &DerefExample = deref.deref(); //~ WARNING call to method that does nothing [noop_method_call] let deref = &DerefExample(12u32); let derefed: &u32 = deref.deref(); let a = &&Foo(1u32); - let borrowed: &Foo = a.borrow(); //~ WARNING call to noop method + let borrowed: &Foo = a.borrow(); //~ WARNING call to method that does nothing [noop_method_call] } fn generic(foo: &Foo) { @@ -41,5 +41,5 @@ fn generic(foo: &Foo) { } fn non_generic(foo: &Foo) { - foo.clone(); //~ WARNING call to noop method + foo.clone(); //~ WARNING call to method that does nothing [noop_method_call] } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 1120adee121b1..f5b766f42333f 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,28 +1,28 @@ -warning: call to noop method +warning: call to method that does nothing --> $DIR/noop-method-call.rs:24:32 | LL | let foo_clone: &Foo = foo.clone(); - | ^^^^^^^^^^^ + | ^^^^^^^^^^^ unnecessary method call | = note: `#[warn(noop_method_call)]` on by default -warning: call to noop method +warning: call to method that does nothing --> $DIR/noop-method-call.rs:30:39 | LL | let derefed: &DerefExample = deref.deref(); - | ^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^ unnecessary method call -warning: call to noop method +warning: call to method that does nothing --> $DIR/noop-method-call.rs:36:31 | LL | let borrowed: &Foo = a.borrow(); - | ^^^^^^^^^^ + | ^^^^^^^^^^ unnecessary method call -warning: call to noop method +warning: call to method that does nothing --> $DIR/noop-method-call.rs:44:5 | LL | foo.clone(); - | ^^^^^^^^^^^ + | ^^^^^^^^^^^ unnecessary method call warning: 4 warnings emitted From 16c4afbde4bcc0b2471cd48cf29378d557a8f4ab Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 8 Jan 2021 12:09:29 +0100 Subject: [PATCH 10/20] Fix tidy errors --- src/test/ui/lint/noop-method-call.rs | 9 ++++++--- src/test/ui/lint/noop-method-call.stderr | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index b8ff75845bd7d..9f0ab3960f8dd 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -21,19 +21,22 @@ impl Deref for DerefExample { fn main() { let foo = &Foo(1u32); - let foo_clone: &Foo = foo.clone(); //~ WARNING call to method that does nothing [noop_method_call] + let foo_clone: &Foo = foo.clone(); + //~^ WARNING call to method that does nothing [noop_method_call] let bar = &Bar(1u32); let bar_clone: Bar = bar.clone(); let deref = &&DerefExample(12u32); - let derefed: &DerefExample = deref.deref(); //~ WARNING call to method that does nothing [noop_method_call] + let derefed: &DerefExample = deref.deref(); + //~^ WARNING call to method that does nothing [noop_method_call] let deref = &DerefExample(12u32); let derefed: &u32 = deref.deref(); let a = &&Foo(1u32); - let borrowed: &Foo = a.borrow(); //~ WARNING call to method that does nothing [noop_method_call] + let borrowed: &Foo = a.borrow(); + //~^ WARNING call to method that does nothing [noop_method_call] } fn generic(foo: &Foo) { diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index f5b766f42333f..32acf1632336d 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -7,19 +7,19 @@ LL | let foo_clone: &Foo = foo.clone(); = note: `#[warn(noop_method_call)]` on by default warning: call to method that does nothing - --> $DIR/noop-method-call.rs:30:39 + --> $DIR/noop-method-call.rs:31:39 | LL | let derefed: &DerefExample = deref.deref(); | ^^^^^^^^^^^^^ unnecessary method call warning: call to method that does nothing - --> $DIR/noop-method-call.rs:36:31 + --> $DIR/noop-method-call.rs:38:31 | LL | let borrowed: &Foo = a.borrow(); | ^^^^^^^^^^ unnecessary method call warning: call to method that does nothing - --> $DIR/noop-method-call.rs:44:5 + --> $DIR/noop-method-call.rs:47:5 | LL | foo.clone(); | ^^^^^^^^^^^ unnecessary method call From 95e330bd0197a658096dfa1922ec3b3e6a4e6b78 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 11 Jan 2021 11:47:16 +0100 Subject: [PATCH 11/20] Update error message --- compiler/rustc_lint/src/noop_method_call.rs | 9 ++++++--- src/test/ui/issues/issue-11820.rs | 8 ++++---- src/test/ui/lint/noop-method-call.rs | 9 +++++---- src/test/ui/lint/noop-method-call.stderr | 19 +++++++++++++++---- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index b9b5009d9dd95..e91dd37d8aaae 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -39,7 +39,7 @@ declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]); impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // We only care about method calls - if let ExprKind::MethodCall(..) = expr.kind { + if let ExprKind::MethodCall(call, ..) = expr.kind { // Get the `DefId` only when dealing with an `AssocFn` if let Some((DefKind::AssocFn, did)) = cx.typeck_results().type_dependent_def(expr.hir_id) @@ -55,7 +55,7 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { } let substs = cx.typeck_results().node_substs(expr.hir_id); - // We can't resolve on types that recursively require monomorphization, + // We can't resolve on types that require monomorphization, // so check that we don't need to perfom substitution if !substs.needs_subst() { let param_env = cx.tcx.param_env(trait_id); @@ -73,9 +73,12 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { let expr_span = expr.span; cx.struct_span_lint(NOOP_METHOD_CALL, expr_span, |lint| { - let message = "call to method that does nothing"; + let method = &call.ident.name; + let message = format!("call to `.{}()` on a reference in this situation does nothing", &method); lint.build(&message) .span_label(expr_span, "unnecessary method call") + .note("the type the method is being called on and the return type are functionally equivalent.") + .note("therefore, the method call doesn't actually do anything and can be removed.") .emit() }); } diff --git a/src/test/ui/issues/issue-11820.rs b/src/test/ui/issues/issue-11820.rs index 8a26624a05dc9..dc6349b10ee58 100644 --- a/src/test/ui/issues/issue-11820.rs +++ b/src/test/ui/issues/issue-11820.rs @@ -6,9 +6,9 @@ struct NoClone; fn main() { - let rnc = &NoClone; - let rsnc = &Some(NoClone); + let rnc = &NoClone; + let rsnc = &Some(NoClone); - let _: &NoClone = rnc.clone(); - let _: &Option = rsnc.clone(); + let _: &NoClone = rnc.clone(); + let _: &Option = rsnc.clone(); } diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index 9f0ab3960f8dd..b8aa55e1e1da6 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -22,21 +22,21 @@ impl Deref for DerefExample { fn main() { let foo = &Foo(1u32); let foo_clone: &Foo = foo.clone(); - //~^ WARNING call to method that does nothing [noop_method_call] + //~^ WARNING call to `.clone()` on a reference in this situation does nothing [noop_method_call] let bar = &Bar(1u32); let bar_clone: Bar = bar.clone(); let deref = &&DerefExample(12u32); let derefed: &DerefExample = deref.deref(); - //~^ WARNING call to method that does nothing [noop_method_call] + //~^ WARNING call to `.deref()` on a reference in this situation does nothing [noop_method_call] let deref = &DerefExample(12u32); let derefed: &u32 = deref.deref(); let a = &&Foo(1u32); let borrowed: &Foo = a.borrow(); - //~^ WARNING call to method that does nothing [noop_method_call] + //~^ WARNING call to `.borrow()` on a reference in this situation does nothing [noop_method_call] } fn generic(foo: &Foo) { @@ -44,5 +44,6 @@ fn generic(foo: &Foo) { } fn non_generic(foo: &Foo) { - foo.clone(); //~ WARNING call to method that does nothing [noop_method_call] + foo.clone(); + //~^ WARNING call to `.clone()` on a reference in this situation does nothing [noop_method_call] } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 32acf1632336d..f9cc9735d54f6 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,28 +1,39 @@ -warning: call to method that does nothing +warning: call to `.clone()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:24:32 | LL | let foo_clone: &Foo = foo.clone(); | ^^^^^^^^^^^ unnecessary method call | = note: `#[warn(noop_method_call)]` on by default + = note: the type the method is being called on and the return type are functionally equivalent. + = note: therefore, the method call doesn't actually do anything and can be removed. -warning: call to method that does nothing +warning: call to `.deref()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:31:39 | LL | let derefed: &DerefExample = deref.deref(); | ^^^^^^^^^^^^^ unnecessary method call + | + = note: the type the method is being called on and the return type are functionally equivalent. + = note: therefore, the method call doesn't actually do anything and can be removed. -warning: call to method that does nothing +warning: call to `.borrow()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:38:31 | LL | let borrowed: &Foo = a.borrow(); | ^^^^^^^^^^ unnecessary method call + | + = note: the type the method is being called on and the return type are functionally equivalent. + = note: therefore, the method call doesn't actually do anything and can be removed. -warning: call to method that does nothing +warning: call to `.clone()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:47:5 | LL | foo.clone(); | ^^^^^^^^^^^ unnecessary method call + | + = note: the type the method is being called on and the return type are functionally equivalent. + = note: therefore, the method call doesn't actually do anything and can be removed. warning: 4 warnings emitted From 316e9db0cf86852b0150d8f0d475ba9a1dcc4774 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 11 Jan 2021 19:02:19 +0100 Subject: [PATCH 12/20] Fix tidy error --- src/test/ui/lint/noop-method-call.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index b8aa55e1e1da6..bc61d619abe82 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -22,21 +22,21 @@ impl Deref for DerefExample { fn main() { let foo = &Foo(1u32); let foo_clone: &Foo = foo.clone(); - //~^ WARNING call to `.clone()` on a reference in this situation does nothing [noop_method_call] + //~^ WARNING call to `.clone()` on a reference in this situation does nothing let bar = &Bar(1u32); let bar_clone: Bar = bar.clone(); let deref = &&DerefExample(12u32); let derefed: &DerefExample = deref.deref(); - //~^ WARNING call to `.deref()` on a reference in this situation does nothing [noop_method_call] + //~^ WARNING call to `.deref()` on a reference in this situation does nothing let deref = &DerefExample(12u32); let derefed: &u32 = deref.deref(); let a = &&Foo(1u32); let borrowed: &Foo = a.borrow(); - //~^ WARNING call to `.borrow()` on a reference in this situation does nothing [noop_method_call] + //~^ WARNING call to `.borrow()` on a reference in this situation does nothing } fn generic(foo: &Foo) { @@ -45,5 +45,5 @@ fn generic(foo: &Foo) { fn non_generic(foo: &Foo) { foo.clone(); - //~^ WARNING call to `.clone()` on a reference in this situation does nothing [noop_method_call] + //~^ WARNING call to `.clone()` on a reference in this situation does nothing } From 49f32e0c8e78948210654299a8c19b2e0f0cbfa9 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 12 Jan 2021 14:58:51 +0100 Subject: [PATCH 13/20] Improve error messages --- compiler/rustc_lint/src/noop_method_call.rs | 18 +++++++++---- src/test/ui/lint/noop-method-call.stderr | 28 +++++++++------------ 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index e91dd37d8aaae..1aa4a986cd1fe 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -39,7 +39,7 @@ declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]); impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // We only care about method calls - if let ExprKind::MethodCall(call, ..) = expr.kind { + if let ExprKind::MethodCall(call, _, elements, _) = expr.kind { // Get the `DefId` only when dealing with an `AssocFn` if let Some((DefKind::AssocFn, did)) = cx.typeck_results().type_dependent_def(expr.hir_id) @@ -70,15 +70,23 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { .iter() .any(|s| cx.tcx.is_diagnostic_item(*s, i.def_id())) { + let method = &call.ident.name; + let receiver = &elements[0]; + let receiver_ty = cx.typeck_results().expr_ty(receiver); let expr_span = expr.span; + let note = format!( + "the type `{:?}` which `{}` is being called on is the same as the type returned from `{}`, \ + so the method call does not do anything and can be removed.", + receiver_ty, method, method + ); - cx.struct_span_lint(NOOP_METHOD_CALL, expr_span, |lint| { + let span = expr_span.with_lo(receiver.span.hi()); + cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { let method = &call.ident.name; let message = format!("call to `.{}()` on a reference in this situation does nothing", &method); lint.build(&message) - .span_label(expr_span, "unnecessary method call") - .note("the type the method is being called on and the return type are functionally equivalent.") - .note("therefore, the method call doesn't actually do anything and can be removed.") + .span_label(span, "unnecessary method call") + .note(¬e) .emit() }); } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index f9cc9735d54f6..7e27bf3abf9f3 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,39 +1,35 @@ warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:24:32 + --> $DIR/noop-method-call.rs:24:35 | LL | let foo_clone: &Foo = foo.clone(); - | ^^^^^^^^^^^ unnecessary method call + | ^^^^^^^^ unnecessary method call | = note: `#[warn(noop_method_call)]` on by default - = note: the type the method is being called on and the return type are functionally equivalent. - = note: therefore, the method call doesn't actually do anything and can be removed. + = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed. warning: call to `.deref()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:31:39 + --> $DIR/noop-method-call.rs:31:44 | LL | let derefed: &DerefExample = deref.deref(); - | ^^^^^^^^^^^^^ unnecessary method call + | ^^^^^^^^ unnecessary method call | - = note: the type the method is being called on and the return type are functionally equivalent. - = note: therefore, the method call doesn't actually do anything and can be removed. + = note: the type `&&DerefExample` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed. warning: call to `.borrow()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:38:31 + --> $DIR/noop-method-call.rs:38:32 | LL | let borrowed: &Foo = a.borrow(); - | ^^^^^^^^^^ unnecessary method call + | ^^^^^^^^^ unnecessary method call | - = note: the type the method is being called on and the return type are functionally equivalent. - = note: therefore, the method call doesn't actually do anything and can be removed. + = note: the type `&&Foo` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed. warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:47:5 + --> $DIR/noop-method-call.rs:47:8 | LL | foo.clone(); - | ^^^^^^^^^^^ unnecessary method call + | ^^^^^^^^ unnecessary method call | - = note: the type the method is being called on and the return type are functionally equivalent. - = note: therefore, the method call doesn't actually do anything and can be removed. + = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed. warning: 4 warnings emitted From e48670c34a3f5ab3fe9defcc61861999f3f14e93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 12 Jan 2021 18:37:32 -0800 Subject: [PATCH 14/20] Increase accuracy of lint trigger --- compiler/rustc_lint/src/noop_method_call.rs | 58 +++++++++++++-------- src/test/ui/lint/noop-method-call.rs | 3 ++ src/test/ui/lint/noop-method-call.stderr | 10 ++-- 3 files changed, 43 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 1aa4a986cd1fe..04d87dc959273 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -62,33 +62,45 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { // Resolve the trait method instance if let Ok(Some(i)) = ty::Instance::resolve(cx.tcx, param_env, did, substs) { // Check that it implements the noop diagnostic - if [ - sym::noop_method_borrow, - sym::noop_method_clone, - sym::noop_method_deref, + for (s, peel_ref) in [ + (sym::noop_method_borrow, true), + (sym::noop_method_clone, false), + (sym::noop_method_deref, true), ] .iter() - .any(|s| cx.tcx.is_diagnostic_item(*s, i.def_id())) { - let method = &call.ident.name; - let receiver = &elements[0]; - let receiver_ty = cx.typeck_results().expr_ty(receiver); - let expr_span = expr.span; - let note = format!( - "the type `{:?}` which `{}` is being called on is the same as the type returned from `{}`, \ - so the method call does not do anything and can be removed.", - receiver_ty, method, method - ); - - let span = expr_span.with_lo(receiver.span.hi()); - cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { + if cx.tcx.is_diagnostic_item(*s, i.def_id()) { let method = &call.ident.name; - let message = format!("call to `.{}()` on a reference in this situation does nothing", &method); - lint.build(&message) - .span_label(span, "unnecessary method call") - .note(¬e) - .emit() - }); + let receiver = &elements[0]; + let receiver_ty = cx.typeck_results().expr_ty(receiver); + let receiver_ty = match receiver_ty.kind() { + // Remove one borrow from the receiver as all the trait methods + // we care about here have a `&self` receiver. + ty::Ref(_, ty, _) if *peel_ref => ty, + _ => receiver_ty, + }; + let expr_ty = cx.typeck_results().expr_ty_adjusted(expr); + if receiver_ty != expr_ty { + return; + } + let expr_span = expr.span; + let note = format!( + "the type `{:?}` which `{}` is being called on is the same as \ + the type returned from `{}`, so the method call does not do \ + anything and can be removed", + receiver_ty, method, method, + ); + + let span = expr_span.with_lo(receiver.span.hi()); + cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { + let method = &call.ident.name; + let message = format!("call to `.{}()` on a reference in this situation does nothing", &method); + lint.build(&message) + .span_label(span, "unnecessary method call") + .note(¬e) + .emit() + }); + } } } } diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index bc61d619abe82..70363a191e9e9 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -37,6 +37,9 @@ fn main() { let a = &&Foo(1u32); let borrowed: &Foo = a.borrow(); //~^ WARNING call to `.borrow()` on a reference in this situation does nothing + + let xs = ["a", "b", "c"]; + let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead } fn generic(foo: &Foo) { diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 7e27bf3abf9f3..316c47975e6de 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -5,7 +5,7 @@ LL | let foo_clone: &Foo = foo.clone(); | ^^^^^^^^ unnecessary method call | = note: `#[warn(noop_method_call)]` on by default - = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed. + = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed warning: call to `.deref()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:31:44 @@ -13,7 +13,7 @@ warning: call to `.deref()` on a reference in this situation does nothing LL | let derefed: &DerefExample = deref.deref(); | ^^^^^^^^ unnecessary method call | - = note: the type `&&DerefExample` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed. + = note: the type `&DerefExample` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed warning: call to `.borrow()` on a reference in this situation does nothing --> $DIR/noop-method-call.rs:38:32 @@ -21,15 +21,15 @@ warning: call to `.borrow()` on a reference in this situation does nothing LL | let borrowed: &Foo = a.borrow(); | ^^^^^^^^^ unnecessary method call | - = note: the type `&&Foo` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed. + = note: the type `&Foo` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:47:8 + --> $DIR/noop-method-call.rs:50:8 | LL | foo.clone(); | ^^^^^^^^ unnecessary method call | - = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed. + = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed warning: 4 warnings emitted From 055db16479937d0a567b2318c775a25ab6cb7d99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 13 Jan 2021 10:39:25 -0800 Subject: [PATCH 15/20] Clean up code rightward drift --- compiler/rustc_lint/src/noop_method_call.rs | 144 +++++++++++--------- 1 file changed, 79 insertions(+), 65 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 04d87dc959273..b4ee1f9157c82 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -38,73 +38,87 @@ declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]); impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - // We only care about method calls - if let ExprKind::MethodCall(call, _, elements, _) = expr.kind { - // Get the `DefId` only when dealing with an `AssocFn` - if let Some((DefKind::AssocFn, did)) = - cx.typeck_results().type_dependent_def(expr.hir_id) - { - // Check that we're dealing with a trait method - if let Some(trait_id) = cx.tcx.trait_of_item(did) { - // Check we're dealing with one of the traits we care about - if ![sym::Clone, sym::Deref, sym::Borrow] + // We only care about method calls. + let (call, elements) = match expr.kind { + ExprKind::MethodCall(call, _, elements, _) => (call, elements), + _ => return, + }; + // We only care about method calls corresponding to the `Clone`, `Deref` and `Borrow` + // traits and ignore any other method call. + let (trait_id, did) = match cx.typeck_results().type_dependent_def(expr.hir_id) { + // Verify we are dealing with a method/associated function. + Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) { + // Check that we're dealing with a trait method for one of the traits we care about. + Some(trait_id) + if [sym::Clone, sym::Deref, sym::Borrow] .iter() - .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) - { - return; - } - - let substs = cx.typeck_results().node_substs(expr.hir_id); - // We can't resolve on types that require monomorphization, - // so check that we don't need to perfom substitution - if !substs.needs_subst() { - let param_env = cx.tcx.param_env(trait_id); - // Resolve the trait method instance - if let Ok(Some(i)) = ty::Instance::resolve(cx.tcx, param_env, did, substs) { - // Check that it implements the noop diagnostic - for (s, peel_ref) in [ - (sym::noop_method_borrow, true), - (sym::noop_method_clone, false), - (sym::noop_method_deref, true), - ] - .iter() - { - if cx.tcx.is_diagnostic_item(*s, i.def_id()) { - let method = &call.ident.name; - let receiver = &elements[0]; - let receiver_ty = cx.typeck_results().expr_ty(receiver); - let receiver_ty = match receiver_ty.kind() { - // Remove one borrow from the receiver as all the trait methods - // we care about here have a `&self` receiver. - ty::Ref(_, ty, _) if *peel_ref => ty, - _ => receiver_ty, - }; - let expr_ty = cx.typeck_results().expr_ty_adjusted(expr); - if receiver_ty != expr_ty { - return; - } - let expr_span = expr.span; - let note = format!( - "the type `{:?}` which `{}` is being called on is the same as \ - the type returned from `{}`, so the method call does not do \ - anything and can be removed", - receiver_ty, method, method, - ); - - let span = expr_span.with_lo(receiver.span.hi()); - cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { - let method = &call.ident.name; - let message = format!("call to `.{}()` on a reference in this situation does nothing", &method); - lint.build(&message) - .span_label(span, "unnecessary method call") - .note(¬e) - .emit() - }); - } - } - } - } + .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => + { + (trait_id, did) } + _ => return, + }, + _ => return, + }; + let substs = cx.typeck_results().node_substs(expr.hir_id); + if substs.needs_subst() { + // We can't resolve on types that require monomorphization, so we don't handle them if + // we need to perfom substitution. + return; + } + let param_env = cx.tcx.param_env(trait_id); + // Resolve the trait method instance. + let i = match ty::Instance::resolve(cx.tcx, param_env, did, substs) { + Ok(Some(i)) => i, + _ => return, + }; + // (Re)check that it implements the noop diagnostic. + for (s, peel_ref) in [ + (sym::noop_method_borrow, true), + (sym::noop_method_clone, false), + (sym::noop_method_deref, true), + ] + .iter() + { + if cx.tcx.is_diagnostic_item(*s, i.def_id()) { + let method = &call.ident.name; + let receiver = &elements[0]; + let receiver_ty = cx.typeck_results().expr_ty(receiver); + let receiver_ty = match receiver_ty.kind() { + // Remove one borrow from the receiver if appropriate to positively verify that + // the receiver `&self` type and the return type are the same, depending on the + // involved trait being checked. + ty::Ref(_, ty, _) if *peel_ref => ty, + // When it comes to `Clone` we need to check the `receiver_ty` directly. + // FIXME: we must come up with a better strategy for this. + _ => receiver_ty, + }; + let expr_ty = cx.typeck_results().expr_ty_adjusted(expr); + if receiver_ty != expr_ty { + // This lint will only trigger if the receiver type and resulting expression \ + // type are the same, implying that the method call is unnecessary. + return; + } + let expr_span = expr.span; + let note = format!( + "the type `{:?}` which `{}` is being called on is the same as \ + the type returned from `{}`, so the method call does not do \ + anything and can be removed", + receiver_ty, method, method, + ); + + let span = expr_span.with_lo(receiver.span.hi()); + cx.struct_span_lint(NOOP_METHOD_CALL, span, |lint| { + let method = &call.ident.name; + let message = format!( + "call to `.{}()` on a reference in this situation does nothing", + &method, + ); + lint.build(&message) + .span_label(span, "unnecessary method call") + .note(¬e) + .emit() + }); } } } From 4be7052b1a81f8f39d9ad23781e19f30d071f572 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 18 Jan 2021 14:15:19 +0100 Subject: [PATCH 16/20] Allow noop_method_call in clippy ui test --- src/tools/clippy/tests/ui/unnecessary_clone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/clippy/tests/ui/unnecessary_clone.rs b/src/tools/clippy/tests/ui/unnecessary_clone.rs index 6770a7fac90fd..ce26634a995d9 100644 --- a/src/tools/clippy/tests/ui/unnecessary_clone.rs +++ b/src/tools/clippy/tests/ui/unnecessary_clone.rs @@ -1,7 +1,7 @@ // does not test any rustfixable lints #![warn(clippy::clone_on_ref_ptr)] -#![allow(unused, clippy::redundant_clone, clippy::unnecessary_wraps)] +#![allow(unused, noop_method_call, clippy::redundant_clone, clippy::unnecessary_wraps)] use std::cell::RefCell; use std::rc::{self, Rc}; From 6bf6652616ca75dde30cbdd021942433ae519730 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 10 Feb 2021 11:08:00 +0100 Subject: [PATCH 17/20] Move unrelated ui test back to what it was before --- src/test/ui/panic-handler/weak-lang-item.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/panic-handler/weak-lang-item.stderr b/src/test/ui/panic-handler/weak-lang-item.stderr index b7c040c7a850b..68e3e21df3e08 100644 --- a/src/test/ui/panic-handler/weak-lang-item.stderr +++ b/src/test/ui/panic-handler/weak-lang-item.stderr @@ -10,10 +10,10 @@ help: you can use `as` to change the binding name of the import LL | extern crate core as other_core; | -error: `#[panic_handler]` function required, but not found - error: language item required, but not found: `eh_personality` +error: `#[panic_handler]` function required, but not found + error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0259`. From da3995f0ec3085de42dcce9e91dbb5662b2c99d3 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 16 Feb 2021 15:12:19 +0100 Subject: [PATCH 18/20] Remove lint pass on borrow and deref --- compiler/rustc_lint/src/noop_method_call.rs | 12 +----- compiler/rustc_span/src/symbol.rs | 3 -- library/core/src/borrow.rs | 2 - library/core/src/ops/deref.rs | 2 - src/test/ui/lint/noop-method-call.rs | 46 +++++++-------------- src/test/ui/lint/noop-method-call.stderr | 34 ++++----------- 6 files changed, 25 insertions(+), 74 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index b4ee1f9157c82..335c3c575e766 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -50,9 +50,7 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) { // Check that we're dealing with a trait method for one of the traits we care about. Some(trait_id) - if [sym::Clone, sym::Deref, sym::Borrow] - .iter() - .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => + if [sym::Clone].iter().any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => { (trait_id, did) } @@ -73,13 +71,7 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { _ => return, }; // (Re)check that it implements the noop diagnostic. - for (s, peel_ref) in [ - (sym::noop_method_borrow, true), - (sym::noop_method_clone, false), - (sym::noop_method_deref, true), - ] - .iter() - { + for (s, peel_ref) in [(sym::noop_method_clone, false)].iter() { if cx.tcx.is_diagnostic_item(*s, i.def_id()) { let method = &call.ident.name; let receiver = &elements[0]; diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 61f9a080a5217..f43b180e06321 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -142,7 +142,6 @@ symbols! { Decodable, Decoder, Default, - Deref, Encodable, Encoder, Eq, @@ -791,9 +790,7 @@ symbols! { none_error, nontemporal_store, nontrapping_dash_fptoint: "nontrapping-fptoint", - noop_method_borrow, noop_method_clone, - noop_method_deref, noreturn, nostack, not, diff --git a/library/core/src/borrow.rs b/library/core/src/borrow.rs index a0cdf681f67e1..c9040cd0a1670 100644 --- a/library/core/src/borrow.rs +++ b/library/core/src/borrow.rs @@ -153,7 +153,6 @@ /// [`HashMap`]: ../../std/collections/struct.HashMap.html /// [`String`]: ../../std/string/struct.String.html #[stable(feature = "rust1", since = "1.0.0")] -#[rustc_diagnostic_item = "Borrow"] pub trait Borrow { /// Immutably borrows from an owned value. /// @@ -220,7 +219,6 @@ impl BorrowMut for T { #[stable(feature = "rust1", since = "1.0.0")] impl Borrow for &T { - #[rustc_diagnostic_item = "noop_method_borrow"] fn borrow(&self) -> &T { &**self } diff --git a/library/core/src/ops/deref.rs b/library/core/src/ops/deref.rs index 10e3ce67448c8..2419771eae212 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -60,7 +60,6 @@ #[doc(alias = "*")] #[doc(alias = "&*")] #[stable(feature = "rust1", since = "1.0.0")] -#[rustc_diagnostic_item = "Deref"] pub trait Deref { /// The resulting type after dereferencing. #[stable(feature = "rust1", since = "1.0.0")] @@ -79,7 +78,6 @@ pub trait Deref { impl Deref for &T { type Target = T; - #[rustc_diagnostic_item = "noop_method_deref"] fn deref(&self) -> &T { *self } diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index 70363a191e9e9..8e4b5bf4d12c7 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -2,51 +2,33 @@ #![allow(unused)] -use std::borrow::Borrow; -use std::ops::Deref; - -struct Foo(T); +struct NonCloneType(T); #[derive(Clone)] -struct Bar(T); - -struct DerefExample(T); - -impl Deref for DerefExample { - type Target = T; - fn deref(&self) -> &Self::Target { - &self.0 - } -} +struct CloneType(T); fn main() { - let foo = &Foo(1u32); - let foo_clone: &Foo = foo.clone(); + let non_clone_type_ref = &NonCloneType(1u32); + let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing - let bar = &Bar(1u32); - let bar_clone: Bar = bar.clone(); - - let deref = &&DerefExample(12u32); - let derefed: &DerefExample = deref.deref(); - //~^ WARNING call to `.deref()` on a reference in this situation does nothing - - let deref = &DerefExample(12u32); - let derefed: &u32 = deref.deref(); + let clone_type_ref = &CloneType(1u32); + let clone_type_ref_clone: CloneType = clone_type_ref.clone(); - let a = &&Foo(1u32); - let borrowed: &Foo = a.borrow(); - //~^ WARNING call to `.borrow()` on a reference in this situation does nothing + // Calling clone on a double reference doesn't warn since the method call itself + // peels the outer reference off + let clone_type_ref = &&CloneType(1u32); + let clone_type_ref_clone: &CloneType = clone_type_ref.clone(); let xs = ["a", "b", "c"]; let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead } -fn generic(foo: &Foo) { - foo.clone(); +fn generic(non_clone_type: &NonCloneType) { + non_clone_type.clone(); } -fn non_generic(foo: &Foo) { - foo.clone(); +fn non_generic(non_clone_type: &NonCloneType) { + non_clone_type.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 316c47975e6de..85a67f538407c 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,35 +1,19 @@ warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:24:35 + --> $DIR/noop-method-call.rs:12:74 | -LL | let foo_clone: &Foo = foo.clone(); - | ^^^^^^^^ unnecessary method call +LL | let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); + | ^^^^^^^^ unnecessary method call | = note: `#[warn(noop_method_call)]` on by default - = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed - -warning: call to `.deref()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:31:44 - | -LL | let derefed: &DerefExample = deref.deref(); - | ^^^^^^^^ unnecessary method call - | - = note: the type `&DerefExample` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed - -warning: call to `.borrow()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:38:32 - | -LL | let borrowed: &Foo = a.borrow(); - | ^^^^^^^^^ unnecessary method call - | - = note: the type `&Foo` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed + = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:50:8 + --> $DIR/noop-method-call.rs:32:19 | -LL | foo.clone(); - | ^^^^^^^^ unnecessary method call +LL | non_clone_type.clone(); + | ^^^^^^^^ unnecessary method call | - = note: the type `&Foo` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed + = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed -warning: 4 warnings emitted +warning: 2 warnings emitted From 1999a3147f5ab65cd556d45e631be5c18fbaebf4 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 16 Feb 2021 22:39:05 +0100 Subject: [PATCH 19/20] Fix borrow and deref --- compiler/rustc_lint/src/noop_method_call.rs | 18 ++++------ compiler/rustc_span/src/symbol.rs | 3 ++ library/core/src/borrow.rs | 2 ++ library/core/src/ops/deref.rs | 2 ++ library/core/tests/clone.rs | 2 -- src/test/ui/issues/issue-11820.rs | 2 -- src/test/ui/lint/noop-method-call.rs | 30 +++++++++++++--- src/test/ui/lint/noop-method-call.stderr | 36 ++++++++++++++----- src/test/ui/underscore-imports/cycle.rs | 1 - .../ui/underscore-imports/macro-expanded.rs | 1 - .../clippy/tests/ui/unnecessary_clone.rs | 2 +- 11 files changed, 67 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 335c3c575e766..3a1b9c85fd1bd 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -15,6 +15,7 @@ declare_lint! { /// /// ```rust /// # #![allow(unused)] + /// #![deny(noop_method_call)] /// struct Foo; /// let foo = &Foo; /// let clone: &Foo = foo.clone(); @@ -30,7 +31,7 @@ declare_lint! { /// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything /// as references are copy. This lint detects these calls and warns the user about them. pub NOOP_METHOD_CALL, - Warn, + Allow, "detects the use of well-known noop methods" } @@ -50,7 +51,9 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { Some((DefKind::AssocFn, did)) => match cx.tcx.trait_of_item(did) { // Check that we're dealing with a trait method for one of the traits we care about. Some(trait_id) - if [sym::Clone].iter().any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => + if [sym::Clone, sym::Deref, sym::Borrow] + .iter() + .any(|s| cx.tcx.is_diagnostic_item(*s, trait_id)) => { (trait_id, did) } @@ -71,20 +74,11 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall { _ => return, }; // (Re)check that it implements the noop diagnostic. - for (s, peel_ref) in [(sym::noop_method_clone, false)].iter() { + for s in [sym::noop_method_clone, sym::noop_method_deref, sym::noop_method_borrow].iter() { if cx.tcx.is_diagnostic_item(*s, i.def_id()) { let method = &call.ident.name; let receiver = &elements[0]; let receiver_ty = cx.typeck_results().expr_ty(receiver); - let receiver_ty = match receiver_ty.kind() { - // Remove one borrow from the receiver if appropriate to positively verify that - // the receiver `&self` type and the return type are the same, depending on the - // involved trait being checked. - ty::Ref(_, ty, _) if *peel_ref => ty, - // When it comes to `Clone` we need to check the `receiver_ty` directly. - // FIXME: we must come up with a better strategy for this. - _ => receiver_ty, - }; let expr_ty = cx.typeck_results().expr_ty_adjusted(expr); if receiver_ty != expr_ty { // This lint will only trigger if the receiver type and resulting expression \ diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index f43b180e06321..61f9a080a5217 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -142,6 +142,7 @@ symbols! { Decodable, Decoder, Default, + Deref, Encodable, Encoder, Eq, @@ -790,7 +791,9 @@ symbols! { none_error, nontemporal_store, nontrapping_dash_fptoint: "nontrapping-fptoint", + noop_method_borrow, noop_method_clone, + noop_method_deref, noreturn, nostack, not, diff --git a/library/core/src/borrow.rs b/library/core/src/borrow.rs index c9040cd0a1670..f28be20aaa1e6 100644 --- a/library/core/src/borrow.rs +++ b/library/core/src/borrow.rs @@ -153,6 +153,7 @@ /// [`HashMap`]: ../../std/collections/struct.HashMap.html /// [`String`]: ../../std/string/struct.String.html #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Borrow"] pub trait Borrow { /// Immutably borrows from an owned value. /// @@ -205,6 +206,7 @@ pub trait BorrowMut: Borrow { #[stable(feature = "rust1", since = "1.0.0")] impl Borrow for T { + #[rustc_diagnostic_item = "noop_method_borrow"] fn borrow(&self) -> &T { self } diff --git a/library/core/src/ops/deref.rs b/library/core/src/ops/deref.rs index 2419771eae212..10e3ce67448c8 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -60,6 +60,7 @@ #[doc(alias = "*")] #[doc(alias = "&*")] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_diagnostic_item = "Deref"] pub trait Deref { /// The resulting type after dereferencing. #[stable(feature = "rust1", since = "1.0.0")] @@ -78,6 +79,7 @@ pub trait Deref { impl Deref for &T { type Target = T; + #[rustc_diagnostic_item = "noop_method_deref"] fn deref(&self) -> &T { *self } diff --git a/library/core/tests/clone.rs b/library/core/tests/clone.rs index 2f2aa9a20f912..c97a87aebce41 100644 --- a/library/core/tests/clone.rs +++ b/library/core/tests/clone.rs @@ -1,5 +1,3 @@ -#![cfg_attr(not(bootstrap), allow(noop_method_call))] - #[test] fn test_borrowed_clone() { let x = 5; diff --git a/src/test/ui/issues/issue-11820.rs b/src/test/ui/issues/issue-11820.rs index dc6349b10ee58..7ffe9652797cf 100644 --- a/src/test/ui/issues/issue-11820.rs +++ b/src/test/ui/issues/issue-11820.rs @@ -1,8 +1,6 @@ // run-pass // pretty-expanded FIXME #23616 -#![allow(noop_method_call)] - struct NoClone; fn main() { diff --git a/src/test/ui/lint/noop-method-call.rs b/src/test/ui/lint/noop-method-call.rs index 8e4b5bf4d12c7..9870c813572e3 100644 --- a/src/test/ui/lint/noop-method-call.rs +++ b/src/test/ui/lint/noop-method-call.rs @@ -1,15 +1,19 @@ // check-pass #![allow(unused)] +#![warn(noop_method_call)] -struct NonCloneType(T); +use std::borrow::Borrow; +use std::ops::Deref; + +struct PlainType(T); #[derive(Clone)] struct CloneType(T); fn main() { - let non_clone_type_ref = &NonCloneType(1u32); - let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); + let non_clone_type_ref = &PlainType(1u32); + let non_clone_type_ref_clone: &PlainType = non_clone_type_ref.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing let clone_type_ref = &CloneType(1u32); @@ -20,15 +24,31 @@ fn main() { let clone_type_ref = &&CloneType(1u32); let clone_type_ref_clone: &CloneType = clone_type_ref.clone(); + let non_deref_type = &PlainType(1u32); + let non_deref_type_deref: &PlainType = non_deref_type.deref(); + //~^ WARNING call to `.deref()` on a reference in this situation does nothing + + // Dereferencing a &&T does not warn since it has collapsed the double reference + let non_deref_type = &&PlainType(1u32); + let non_deref_type_deref: &PlainType = non_deref_type.deref(); + + let non_borrow_type = &PlainType(1u32); + let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + //~^ WARNING call to `.borrow()` on a reference in this situation does nothing + + // Borrowing a &&T does not warn since it has collapsed the double reference + let non_borrow_type = &&PlainType(1u32); + let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + let xs = ["a", "b", "c"]; let _v: Vec<&str> = xs.iter().map(|x| x.clone()).collect(); // ok, but could use `*x` instead } -fn generic(non_clone_type: &NonCloneType) { +fn generic(non_clone_type: &PlainType) { non_clone_type.clone(); } -fn non_generic(non_clone_type: &NonCloneType) { +fn non_generic(non_clone_type: &PlainType) { non_clone_type.clone(); //~^ WARNING call to `.clone()` on a reference in this situation does nothing } diff --git a/src/test/ui/lint/noop-method-call.stderr b/src/test/ui/lint/noop-method-call.stderr index 85a67f538407c..7f6f96bf1d142 100644 --- a/src/test/ui/lint/noop-method-call.stderr +++ b/src/test/ui/lint/noop-method-call.stderr @@ -1,19 +1,39 @@ warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:12:74 + --> $DIR/noop-method-call.rs:16:71 | -LL | let non_clone_type_ref_clone: &NonCloneType = non_clone_type_ref.clone(); - | ^^^^^^^^ unnecessary method call +LL | let non_clone_type_ref_clone: &PlainType = non_clone_type_ref.clone(); + | ^^^^^^^^ unnecessary method call | - = note: `#[warn(noop_method_call)]` on by default - = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed +note: the lint level is defined here + --> $DIR/noop-method-call.rs:4:9 + | +LL | #![warn(noop_method_call)] + | ^^^^^^^^^^^^^^^^ + = note: the type `&PlainType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed + +warning: call to `.deref()` on a reference in this situation does nothing + --> $DIR/noop-method-call.rs:28:63 + | +LL | let non_deref_type_deref: &PlainType = non_deref_type.deref(); + | ^^^^^^^^ unnecessary method call + | + = note: the type `&PlainType` which `deref` is being called on is the same as the type returned from `deref`, so the method call does not do anything and can be removed + +warning: call to `.borrow()` on a reference in this situation does nothing + --> $DIR/noop-method-call.rs:36:66 + | +LL | let non_borrow_type_borrow: &PlainType = non_borrow_type.borrow(); + | ^^^^^^^^^ unnecessary method call + | + = note: the type `&PlainType` which `borrow` is being called on is the same as the type returned from `borrow`, so the method call does not do anything and can be removed warning: call to `.clone()` on a reference in this situation does nothing - --> $DIR/noop-method-call.rs:32:19 + --> $DIR/noop-method-call.rs:52:19 | LL | non_clone_type.clone(); | ^^^^^^^^ unnecessary method call | - = note: the type `&NonCloneType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed + = note: the type `&PlainType` which `clone` is being called on is the same as the type returned from `clone`, so the method call does not do anything and can be removed -warning: 2 warnings emitted +warning: 4 warnings emitted diff --git a/src/test/ui/underscore-imports/cycle.rs b/src/test/ui/underscore-imports/cycle.rs index 987410fa84b20..bacf9b2d5a96a 100644 --- a/src/test/ui/underscore-imports/cycle.rs +++ b/src/test/ui/underscore-imports/cycle.rs @@ -14,6 +14,5 @@ mod y { pub fn main() { use x::*; - #[allow(noop_method_call)] (&0).deref(); } diff --git a/src/test/ui/underscore-imports/macro-expanded.rs b/src/test/ui/underscore-imports/macro-expanded.rs index 55e86e848558d..43f527bc9a408 100644 --- a/src/test/ui/underscore-imports/macro-expanded.rs +++ b/src/test/ui/underscore-imports/macro-expanded.rs @@ -3,7 +3,6 @@ // check-pass #![feature(decl_macro, rustc_attrs)] -#![allow(noop_method_call)] mod x { pub use std::ops::Not as _; diff --git a/src/tools/clippy/tests/ui/unnecessary_clone.rs b/src/tools/clippy/tests/ui/unnecessary_clone.rs index ce26634a995d9..6770a7fac90fd 100644 --- a/src/tools/clippy/tests/ui/unnecessary_clone.rs +++ b/src/tools/clippy/tests/ui/unnecessary_clone.rs @@ -1,7 +1,7 @@ // does not test any rustfixable lints #![warn(clippy::clone_on_ref_ptr)] -#![allow(unused, noop_method_call, clippy::redundant_clone, clippy::unnecessary_wraps)] +#![allow(unused, clippy::redundant_clone, clippy::unnecessary_wraps)] use std::cell::RefCell; use std::rc::{self, Rc}; From 25637b228d68e9bdf2d3ce1ba421cbd115fcb81e Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 17 Feb 2021 10:06:23 +0100 Subject: [PATCH 20/20] Warn in doc test --- compiler/rustc_lint/src/noop_method_call.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/noop_method_call.rs b/compiler/rustc_lint/src/noop_method_call.rs index 3a1b9c85fd1bd..479cc00199f6a 100644 --- a/compiler/rustc_lint/src/noop_method_call.rs +++ b/compiler/rustc_lint/src/noop_method_call.rs @@ -15,7 +15,7 @@ declare_lint! { /// /// ```rust /// # #![allow(unused)] - /// #![deny(noop_method_call)] + /// #![warn(noop_method_call)] /// struct Foo; /// let foo = &Foo; /// let clone: &Foo = foo.clone();