From bdb05a84f3b9d0c7d39d2bbbe92d2695d33d79fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 24 May 2019 18:24:20 -0700 Subject: [PATCH 1/4] When suggesting to borrow, remove useless clones --- src/librustc_typeck/check/demand.rs | 3 +++ src/test/ui/issues/issue-61106.rs | 6 ++++++ src/test/ui/issues/issue-61106.stderr | 15 +++++++++++++++ 3 files changed, 24 insertions(+) create mode 100644 src/test/ui/issues/issue-61106.rs create mode 100644 src/test/ui/issues/issue-61106.stderr diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 87fc90f53abca..bc36162e02f6a 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -425,6 +425,9 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } } } + // If this expression had a clone call, when suggesting borrowing, we + // want to suggest removing it + let sugg_expr = sugg_expr.trim_end_matches(".clone()"); return Some(match mutability { hir::Mutability::MutMutable => ( sp, diff --git a/src/test/ui/issues/issue-61106.rs b/src/test/ui/issues/issue-61106.rs new file mode 100644 index 0000000000000..308ef1de3ccc3 --- /dev/null +++ b/src/test/ui/issues/issue-61106.rs @@ -0,0 +1,6 @@ +fn main() { + let x = String::new(); + foo(x.clone()); //~ ERROR mismatched types +} + +fn foo(_: &str) {} diff --git a/src/test/ui/issues/issue-61106.stderr b/src/test/ui/issues/issue-61106.stderr new file mode 100644 index 0000000000000..ca67d51492845 --- /dev/null +++ b/src/test/ui/issues/issue-61106.stderr @@ -0,0 +1,15 @@ +error[E0308]: mismatched types + --> $DIR/issue-61106.rs:3:9 + | +LL | foo(x.clone()); + | ^^^^^^^^^ + | | + | expected &str, found struct `std::string::String` + | help: consider borrowing here: `&x` + | + = note: expected type `&str` + found type `std::string::String` + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. From 8ce063a216768ed125eb4245a027fef431d7d78c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Thu, 13 Jun 2019 17:58:59 -0700 Subject: [PATCH 2/4] Verify that the clone method call actually corresponds to std::clone::Clone::clone --- src/librustc_typeck/check/demand.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index bc36162e02f6a..b26e6ccb559a6 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -425,19 +425,29 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } } } - // If this expression had a clone call, when suggesting borrowing, we - // want to suggest removing it - let sugg_expr = sugg_expr.trim_end_matches(".clone()"); + + let mut sugg = sugg_expr.as_str(); + if let hir::ExprKind::MethodCall(_segment, _sp, _args) = &expr.node { + let clone_path = "std::clone::Clone::clone"; + if let Some(true) = self.tables.borrow() + .type_dependent_def_id(expr.hir_id) + .map(|did| self.tcx.def_path_str(did).as_str() == clone_path) + { + // If this expression had a clone call when suggesting borrowing + // we want to suggest removing it because it'd now be unecessary. + sugg = sugg_expr.trim_end_matches(".clone()"); + } + } return Some(match mutability { hir::Mutability::MutMutable => ( sp, "consider mutably borrowing here", - format!("{}&mut {}", field_name, sugg_expr), + format!("{}&mut {}", field_name, sugg), ), hir::Mutability::MutImmutable => ( sp, "consider borrowing here", - format!("{}&{}", field_name, sugg_expr), + format!("{}&{}", field_name, sugg), ), }); } From ae8d6a82feb184d73a43149fccdeb6aca5b6c14a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 14 Jun 2019 10:45:13 -0700 Subject: [PATCH 3/4] review comments: avoid string modification --- src/librustc_typeck/check/demand.rs | 30 ++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index b26e6ccb559a6..30411225519c7 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -379,7 +379,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } }; if self.can_coerce(ref_ty, expected) { - if let Ok(src) = cm.span_to_snippet(sp) { + let mut sugg_sp = sp; + if let hir::ExprKind::MethodCall(_segment, _sp, args) = &expr.node { + let clone_path = "std::clone::Clone::clone"; + if let ([arg], Some(true)) = (&args[..], self.tables.borrow() + .type_dependent_def_id(expr.hir_id) + .map(|did| self.tcx.def_path_str(did).as_str() == clone_path)) + { + // If this expression had a clone call when suggesting borrowing + // we want to suggest removing it because it'd now be unecessary. + sugg_sp = arg.span; + } + } + if let Ok(src) = cm.span_to_snippet(sugg_sp) { let needs_parens = match expr.node { // parenthesize if needed (Issue #46756) hir::ExprKind::Cast(_, _) | @@ -426,28 +438,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { } } - let mut sugg = sugg_expr.as_str(); - if let hir::ExprKind::MethodCall(_segment, _sp, _args) = &expr.node { - let clone_path = "std::clone::Clone::clone"; - if let Some(true) = self.tables.borrow() - .type_dependent_def_id(expr.hir_id) - .map(|did| self.tcx.def_path_str(did).as_str() == clone_path) - { - // If this expression had a clone call when suggesting borrowing - // we want to suggest removing it because it'd now be unecessary. - sugg = sugg_expr.trim_end_matches(".clone()"); - } - } return Some(match mutability { hir::Mutability::MutMutable => ( sp, "consider mutably borrowing here", - format!("{}&mut {}", field_name, sugg), + format!("{}&mut {}", field_name, sugg_expr), ), hir::Mutability::MutImmutable => ( sp, "consider borrowing here", - format!("{}&{}", field_name, sugg), + format!("{}&{}", field_name, sugg_expr), ), }); } From 34c4117f5f28c8eb8dd61600df1611fd29c8b66e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Fri, 14 Jun 2019 11:44:20 -0700 Subject: [PATCH 4/4] review comment: do not rely on path str to identify std::clone::Clone --- src/librustc_typeck/check/demand.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 30411225519c7..2d7ee2a73f6c8 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -380,12 +380,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> { }; if self.can_coerce(ref_ty, expected) { let mut sugg_sp = sp; - if let hir::ExprKind::MethodCall(_segment, _sp, args) = &expr.node { - let clone_path = "std::clone::Clone::clone"; - if let ([arg], Some(true)) = (&args[..], self.tables.borrow() - .type_dependent_def_id(expr.hir_id) - .map(|did| self.tcx.def_path_str(did).as_str() == clone_path)) - { + if let hir::ExprKind::MethodCall(segment, _sp, args) = &expr.node { + let clone_trait = self.tcx.lang_items().clone_trait().unwrap(); + if let ([arg], Some(true), "clone") = ( + &args[..], + self.tables.borrow().type_dependent_def_id(expr.hir_id).map(|did| { + let ai = self.tcx.associated_item(did); + ai.container == ty::TraitContainer(clone_trait) + }), + &segment.ident.as_str()[..], + ) { // If this expression had a clone call when suggesting borrowing // we want to suggest removing it because it'd now be unecessary. sugg_sp = arg.span;