From 988e2f9b2f654442ec1157e431e4c26d311e0446 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 10:07:50 +0100 Subject: [PATCH 01/15] 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 2336b52619ab8..3fff2303ac681 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -55,6 +55,7 @@ mod levels; mod methods; mod non_ascii_idents; mod nonstandard_style; +mod noop_method_call; mod panic_fmt; mod passes; mod redundant_semicolon; @@ -81,6 +82,7 @@ use internal::*; use methods::*; use non_ascii_idents::*; use nonstandard_style::*; +use noop_method_call::*; use panic_fmt::PanicFmt; use redundant_semicolon::*; use traits::*; @@ -168,6 +170,7 @@ macro_rules! late_lint_passes { ClashingExternDeclarations: ClashingExternDeclarations::new(), DropTraitConstraints: DropTraitConstraints, TemporaryCStringAsPtr: TemporaryCStringAsPtr, + NoopMethodCall: NoopMethodCall, PanicFmt: PanicFmt, ] ); 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 b6cf584d875f1..0f7b4d2efc775 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -887,6 +887,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 58e3a4a24a4e5345ac0b095ecdb80bd9b476fb0c Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 16:14:39 +0100 Subject: [PATCH 02/15] 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 0f7b4d2efc775..4ca5a8f392c15 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -765,6 +765,9 @@ symbols! { none_error, nontemporal_store, nontrapping_dash_fptoint: "nontrapping-fptoint", + noop_method_borrow, + noop_method_clone, + noop_method_deref, noreturn, nostack, not, @@ -887,7 +890,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 245152e5490d8..9c3d66696c890 100644 --- a/library/core/src/ops/deref.rs +++ b/library/core/src/ops/deref.rs @@ -77,6 +77,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 e29cefdfd4c8f34428641626014fad883c458f1b Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Jan 2021 16:46:50 +0100 Subject: [PATCH 03/15] 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 | 4 ++-- 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, 33 insertions(+), 18 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 fe20925b38798..b6a3a15cfbebe 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 { @@ -535,7 +534,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 07173f41cd6db..a81f93729381d 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 4ca5a8f392c15..7c2626a31e269 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -126,6 +126,7 @@ symbols! { Argument, ArgumentV1, Arguments, + Borrow, C, CString, Center, @@ -136,6 +137,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 a439bb892f8e7..d83641afe72ca 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -828,7 +828,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 bd2f87f70a2f1..23a2baf2496f6 100644 --- a/compiler/rustc_traits/src/chalk/mod.rs +++ b/compiler/rustc_traits/src/chalk/mod.rs @@ -112,7 +112,7 @@ crate fn evaluate_goal<'tcx>( }); let sol = Canonical { max_universe: ty::UniverseIndex::from_usize(0), - variables: obligation.variables.clone(), + variables: obligation.variables, value: QueryResponse { var_values: CanonicalVarValues { var_values }, region_constraints: QueryRegionConstraints::default(), @@ -137,7 +137,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 116b079e7425a..b30fa71aa755b 100644 --- a/compiler/rustc_typeck/src/check/callee.rs +++ b/compiler/rustc_typeck/src/check/callee.rs @@ -446,7 +446,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 d76a80d5a3990..15004138afb1e 100644 --- a/compiler/rustc_typeck/src/check/expr.rs +++ b/compiler/rustc_typeck/src/check/expr.rs @@ -710,7 +710,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 9c3d66696c890..8911772604aaf 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 9777e0c0e45ff20f6ef55e03d091276b10af0d99 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 14:48:06 +0100 Subject: [PATCH 04/15] 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 f3bbb980a9765d32a891438157cc455b64d4d650 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 17:56:34 +0100 Subject: [PATCH 05/15] 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 f92aed8ce15bf..5a4eae165a50d 100644 --- a/library/alloc/src/collections/btree/map/tests.rs +++ b/library/alloc/src/collections/btree/map/tests.rs @@ -1633,11 +1633,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()), } @@ -1653,11 +1653,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 ac50400aade9f835875bc38d4d017b7036f6cde9 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Jan 2021 21:11:08 +0100 Subject: [PATCH 06/15] Fix core tests --- library/core/src/clone.rs | 1 + library/core/tests/clone.rs | 2 ++ library/core/tests/iter.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.rs b/library/core/tests/iter.rs index 7376e7848eff5..3dff2ebf8f0e9 100644 --- a/library/core/tests/iter.rs +++ b/library/core/tests/iter.rs @@ -3509,7 +3509,7 @@ pub fn extend_for_unit() { #[test] fn test_intersperse() { 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()); @@ -3521,7 +3521,7 @@ fn test_intersperse() { #[test] fn test_intersperse_size_hint() { 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 8ac6481ef8beab758018c97c47bfaa4abffd297e Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Thu, 7 Jan 2021 11:24:32 +0100 Subject: [PATCH 07/15] 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 cdcb3b5160f3d6b44f77e5b077ca22c22a7b62c9 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Thu, 7 Jan 2021 13:13:25 +0100 Subject: [PATCH 08/15] 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 fd81c874f0e7e28a96429416dbfdcdbf2d0e396c Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 8 Jan 2021 11:37:52 +0100 Subject: [PATCH 09/15] 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 a35655621a7a7c0d41093e177855f46bce75fa30 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Fri, 8 Jan 2021 12:09:29 +0100 Subject: [PATCH 10/15] 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 8ded7852edcdabfe630fe18fdc5e8e3cbcae0e28 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 11 Jan 2021 11:47:16 +0100 Subject: [PATCH 11/15] 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 7cf66b8214b8aaca9dea5722f7991d008ec8a301 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Mon, 11 Jan 2021 19:02:19 +0100 Subject: [PATCH 12/15] 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 7b7708e663969859e1b549cf1191037450391457 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 12 Jan 2021 14:58:51 +0100 Subject: [PATCH 13/15] 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 59577b89a6bc4188854c170398f2499fe4e51beb 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/15] 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 af88b7311def8ad39ae958360749e97f1a4aaec5 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/15] 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() + }); } } }