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

In <&NotClone as Clone>::clone() call, account for bindings #112977

Closed
wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

Address #112857:

error[E0308]: mismatched types
  --> $DIR/explain_clone_autoref.rs:28:5
   |
LL | fn clone_thing3(nc: &NotClone) -> NotClone {
   |                                   -------- expected `NotClone` because of return type
...
LL |     nc
   |     ^^ expected `NotClone`, found `&NotClone`
   |
note: `NotClone` does not implement `Clone`, so `&NotClone` was cloned instead
  --> $DIR/explain_clone_autoref.rs:26:14
   |
LL |     let nc = nc.clone();
   |              ^^
help: consider annotating `NotClone` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct NotClone;
   |

Address rust-lang#112857:

```
error[E0308]: mismatched types
  --> $DIR/explain_clone_autoref.rs:28:5
   |
LL | fn clone_thing3(nc: &NotClone) -> NotClone {
   |                                   -------- expected `NotClone` because of return type
...
LL |     nc
   |     ^^ expected `NotClone`, found `&NotClone`
   |
note: `NotClone` does not implement `Clone`, so `&NotClone` was cloned instead
  --> $DIR/explain_clone_autoref.rs:26:14
   |
LL |     let nc = nc.clone();
   |              ^^
help: consider annotating `NotClone` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct NotClone;
   |
```
@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2023

r? @WaffleLapkin

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 23, 2023
@estebank
Copy link
Contributor Author

@strottos you might want to look at the changes here and see if there are other kind of expressions that we might want to also support, not just single bindings.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'rust-lang/simpleinfra@master' (SHA:3999262f9f9933e66da0e5e168b90798c32a320e)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
  Stored in directory: /tmp/pip-ephem-wheel-cache-91_eedxy/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully tagged rust-ci:latest
Built container sha256:dcb44ec7fdc7167ad8a38f2d3b5109c240f23384b092e5d56672f39e890fdda6
Uploading finished image to https://ci-caches.rust-lang.org/docker/6c79211b759db8881e4e083f32c23f7a9fde027e88ac4e65b52f7f5e0405fbb6bd8d3cd81afe7617920fe598db5be42068b29a40aff8b5cde467364f4587bbd8

<botocore.awsrequest.AWSRequest object at 0x7efc2a9e92d0>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]

@strottos
Copy link
Contributor

This is great, thanks Esteban.

So it can be triggered still further by this kind of (rather contrived) example:

fn f<T>(a: &T) -> T {
    let b = a.clone();
    let c = b;
    c
}

We could I guess keep looping through till no parents found but there maybe should be a limit too and maybe a single round is going to catch enough. This is still an improvement.

On the other hand these are all slightly contrived examples and in the real world something more complex is likely to be the program, maybe wouldn't hurt to loop a little bit further through.

Will have another look over the weekend to see if there's anything else worth thinking about.

@strottos
Copy link
Contributor

strottos commented Jun 26, 2023

So I had another look at this over the weekend. I think there are improvements but I certainly see no issue with this PR so feel free to push it whenever for this issue.

I did put together a separate PR #112995 as an improvement that takes it a step further and recursively trails back up in case we do the above of:

fn f<T>(a: &T) -> T {
    let b = a.clone();
    let c = b;
    c
}

Apologies @estebank as it builds on your work but wasn't sure how best to push suggestions. Not trying to take credit for your work at all and hope it doesn't come across that way.

I'd suggest we either:
(i) merge this in first and look at my PR as a separate improvement (can easily rebase my changes after this goes in)
(ii) pull my changes into your PR here
(iii) merge my PR straight if we think there's no issues with that.

With regards to (ii) and (iii) options my minor concerns are:
(i) Is it possible to trigger a bad suggestion somehow, I tried and haven't managed it yet and I think it's OK, but not 100% certain.
(ii) Are their concerns about heavy recursion? I tried it with 200 let v2 = v1; ... type statements and it seemed to work OK on Linux still, and the likelihood is it would be short-circuited pretty quickly and would also only trigger in the case of errors.
But I do think this other change will pickup more realistic cases in the wild. The cases we do capture are fairly contrived at the moment and even bordering on obvious, this might catch some that are less obvious.

@bors
Copy link
Contributor

bors commented Jul 13, 2023

☔ The latest upstream changes (presumably #113637) made this pull request unmergeable. Please resolve the merge conflicts.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2023
…fee1-dead

Check for `<&NotClone as Clone>::clone()` calls and suggest to add Clone trait appropriately

Added recursive checking back up the HIR to see if a `Clone` suggestion would be helpful.

Addresses rust-lang#112857

Largely based on: rust-lang#112977
@estebank estebank closed this Jul 26, 2023
@WaffleLapkin WaffleLapkin removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants