Skip to content
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.

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

When suggesting borrow, remove useless clones #61143

Merged
merged 4 commits into from
Jun 15, 2019

Conversation

estebank
Copy link
Contributor

Fix #61106.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 25, 2019
@estebank estebank changed the title When suggesting to borrow, remove useless clones When suggesting borrow, remove useless clones May 25, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 8ce063a has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2019
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me with trim_end_matches(".clone()") removed

@eddyb
Copy link
Member

eddyb commented Jun 14, 2019

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 14, 2019
@estebank
Copy link
Contributor Author

r? @eddyb @bors r=eddyb

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 34c4117 has been approved by eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned oli-obk Jun 14, 2019
@bors
Copy link
Contributor

bors commented Jun 14, 2019

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
When suggesting borrow, remove useless clones

Fix rust-lang#61106.
Centril added a commit to Centril/rust that referenced this pull request Jun 14, 2019
When suggesting borrow, remove useless clones

Fix rust-lang#61106.
@bors
Copy link
Contributor

bors commented Jun 15, 2019

⌛ Testing commit 34c4117 with merge 9f06855...

bors added a commit that referenced this pull request Jun 15, 2019
When suggesting borrow, remove useless clones

Fix #61106.
@bors
Copy link
Contributor

bors commented Jun 15, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing 9f06855 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2019
@bors bors merged commit 34c4117 into rust-lang:master Jun 15, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #61143!

Tested on commit 9f06855.
Direct link to PR: #61143

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 15, 2019
Tested on commit rust-lang/rust@9f06855.
Direct link to PR: <rust-lang/rust#61143>

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
) {
// 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect AString.clone() when &str is needed and emit appropriate suggestion
6 participants