Skip to content

When suggesting borrow, remove useless clones #61143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 15, 2019
Merged

When suggesting borrow, remove useless clones #61143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bdb05a8
When suggesting to borrow, remove useless clones
estebank May 25, 2019
8ce063a
Verify that the clone method call actually corresponds to std::clone:…
estebank Jun 14, 2019
ae8d6a8
review comments: avoid string modification
estebank Jun 14, 2019
34c4117
review comment: do not rely on path str to identify std::clone::Clone
estebank Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/librustc_typeck/check/demand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,23 @@ 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_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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also change expr (maybe better named sugg_expr, inside the if let below?) to arg, so that the needs_parens logic, and everything else, works correctly.
(e.g. I suspect you print &1 + 2 instead of &(1 + 2) for (1 + 2).clone())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it works just fine:

error[E0308]: mismatched types
 --> src/main.rs:2:19
  |
2 |     let x: &i32 = (1i32 + 2).clone();
  |                   ^^^^^^^^^^^^^^^^^^
  |                   |
  |                   expected &i32, found i32
  |                   help: consider borrowing here: `&(1i32 + 2)`
  |
  = note: expected type `&i32`
             found type `i32`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's mildly disturbing - the code is technically wrong, since it's looking at the whole method call when determining whether it needs parens, but happens to produce the correct result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because the logic is to add parentheses to the code when it is not already int he code. In this case the parentheses are already there so the expression doesn't need to change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExprKind::Paren doesn't exist in the HIR, the first argument of the clone method call is 1i32 + 2 (which I would've assumed doesn't include parens in its own 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(_, _) |
Expand Down Expand Up @@ -425,6 +441,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}
}
}

return Some(match mutability {
hir::Mutability::MutMutable => (
sp,
Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/issues/issue-61106.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn main() {
let x = String::new();
foo(x.clone()); //~ ERROR mismatched types
}

fn foo(_: &str) {}
15 changes: 15 additions & 0 deletions src/test/ui/issues/issue-61106.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`.