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

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fee1-dead (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@estebank Forgive if this is stealing thunder as I wouldn't have done this so quickly without your work here, please feel free to pull this into your branch or whatever works best for protocol.

This covers more cases as per the tests though there are still potentially further improvements I may look into at some point as I notice this issue is probably in the same area: #76643

I think this is OK, but there is a danger of heavy recursion in big code bases but I think they'll always terminate fairly quickly or get caught by stack limits (which are probably quite high in compiler world having said that). Would welcome your thoughts on this one?

Note that there is already the noop_method_call lint, which is being made warn by default in #111916. There is also an MCP for running lints even if errors have occured at compiler-team#633. If both of these were to be merged then I don't think this change would be necessary?

@fee1-dead Seems fair, the only thing would be this would put it inline with the error itself making it clear this fixes the error where the other would put out a seemingly unrelated warning the user would have to link together, could even get lost in lots of other warnings. At least that's my understanding of it.

This would potentially be helpful to beginners and the like, more so than experienced Rust devs certainly.

But yeah, if we decide to not go with this for that reason that's cool. I'm happy to close if so.

If both of these were to be merged then I don't think this change would be necessary?

The issue with lints is that they don't trigger if other errors are present, which is why I find it reasonable to have some duplication between targeted lints and suggestions in errors, to reduce the number of back and forth that users have when fixing an issue.

Setting to waiting on author per my review.

@rustbot author

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

Pushed some more changes, there's still some patterns not covered but it's more inclusive as can be seen from the new tests.

Examples of a pattern not covered is the following, this seems harder:

fn clone_thing(nc: &NotClone) -> Box<NotClone> {
    let ret = Box::new(nc.clone());
    ret
}

where conversely this is covered:

fn clone_thing(nc: &NotClone) -> Box<NotClone> {
    let ret: Box<NotClone> = Box::new(nc.clone());
    ret
}

Not opposed to looking at the above, but I suspect this will take longer, would be interested in what people thought for feedback as to whether the above is worth it @fee1-dead @estebank ?

I do think it's an improvement from what it was but I think to cover every pattern will be a lot of work and not sure how much value.

@rustbot review

Examples of a pattern not covered is the following, this seems harder:

fn clone_thing(nc: &NotClone) -> Box<NotClone> {
    let ret = Box::new(nc.clone());
    ret
}

It is fine to not support every case, particularly when there are generics in place. I think we could eventually look for any arguments in an fn expression for .clone() calls, recursively, but I'd be very concerned with false positives. Maybe if we found all .clone() calls in sub expressions of the pointed to expression, changed their type from borrow to owned and re-ran inference on the whole expression to see if that would fix the issue, but that's more complex than what we set out to do here.

I do think it's an improvement from what it was but I think to cover every pattern will be a lot of work and not sure how much value.

I am of the position of "handle 80% of the cases with 20% of the effort". If we can expand on the coverage after the fact, that's fine.

That was the conclusion I came to as well, I did try exactly that funny enough using the (walk|visit)_<foo> methods but I managed to trigger false positives myself pretty easily that were harder to get rid of than just keep exhaustively check like we've ended up with.

Thanks @fee1-dead , think I’ve addressed all but the last one which I don’t think is relevant any more. Thanks for your time.

@bors r+

📌 Commit 25db1fa has been approved by fee1-dead

It is now in the queue for this repository.

LGTM, thanks!

Thanks for your time again and no need to apologise, learned loads from doing this and been a great experience

Testing commit 25db1fa with merge 0c0090f...

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

Test failed - checks-actions

Had a quick look and looks kind of like a spurious failure. It failed because the backtrack on a Windows PC stopped spitting out function names and just wrote out {unknown} in place. Currently unsure why this would happen but my guess is a retry would work. Certainly this isn’t an area I’ve directly touched:

2023-07-25T11:57:15.0014906Z stack backtrace:
2023-07-25T11:57:15.0015160Z    0: 0x662c2d42 - core::fmt::write::h1147d65d38424eec
2023-07-25T11:57:15.0015437Z    1: 0x66256614 - std::io::Write::write_fmt::hb78e5874c67659c6
2023-07-25T11:57:15.0015721Z    2: 0x662603c0 - std::sys_common::backtrace::print::hd10d5c20761406e1
2023-07-25T11:57:15.0016060Z    3: 0x662636cd - std::panicking::panic_hook_with_disk_dump::{{closure}}::h00d490f5d4e50606
2023-07-25T11:57:15.0016511Z    4: 0x662634b3 - std::panicking::panic_hook_with_disk_dump::hebe5fc45befd04f3
2023-07-25T11:57:15.0016828Z    5: 0x66d1f227 - rustc_driver_impl[a5419d7a2f24870a]::install_ice_hook::{closure#0}
2023-07-25T11:57:15.0017144Z    6: 0x66263f19 - std::panicking::rust_panic_with_hook::h4ff6de34a70869f0
2023-07-25T11:57:15.0017457Z    7: 0x66263cc8 - std::panicking::begin_panic_handler::{{closure}}::haf05a0f3d7198621
2023-07-25T11:57:15.0017837Z    8: 0x66261158 - std::sys_common::backtrace::__rust_end_short_backtrace::ha215c523c59793aa
2023-07-25T11:57:15.0018107Z    9: 0x66263a55 - _rust_begin_unwind
2023-07-25T11:57:15.0018363Z   10: 0x662bea6d - core::panicking::panic_fmt::hff163fd7d423cd54
2023-07-25T11:57:15.0018675Z   11: 0x69d40fc7 - <rustc_errors[37c6311b894514b8]::HandlerInner>::panic_if_treat_err_as_bug
2023-07-25T11:57:15.0019003Z   12: 0x69d40126 - <rustc_errors[37c6311b894514b8]::HandlerInner>::emit_diagnostic::{closure#2}
2023-07-25T11:57:15.0019318Z   13: 0x66f6bf79 - rustc_interface[90485b44df649e4f]::callbacks::track_diagnostic
2023-07-25T11:57:15.0019626Z   14: 0x69d3f48f - <rustc_errors[37c6311b894514b8]::HandlerInner>::emit_diagnostic
2023-07-25T11:57:15.0020027Z   15: 0x69d8a538 - <rustc_span[816e203749c7a62]::ErrorGuaranteed as rustc_errors[37c6311b894514b8]::diagnostic_builder::EmissionGuarantee>::diagnostic_builder_emit_producing_guarantee
2023-07-25T11:57:15.0020428Z   16: 0x67edbf3f - <rustc_resolve[9998538d989d25aa]::Resolver>::report_errors
2023-07-25T11:57:15.0020785Z   17: 0x67fe959a - <rustc_session[7093658e48485c60]::session::Session>::time::<(), <rustc_resolve[9998538d989d25aa]::Resolver>::resolve_crate::{closure#0}>
2023-07-25T11:57:15.0021070Z   18:    0x3d4d0 - <unknown>
2023-07-25T11:57:15.0021258Z   19:    0x3d4d0 - <unknown>
2023-07-25T11:57:15.0021371Z 
2023-07-25T11:57:15.0021541Z error: the compiler unexpectedly panicked. this is a bug

When tested on a Linux PC (all I have access to right now, tomorrow can try on a Windows PC tomorrow evening) we get much more output than this that would match the test. Is there a way we can queue a retry @fee1-dead ? No stress on this one, it’s not a biggie.

Would be curious to know under what circumstances we get this… could be some obscure Windows internals maybe but would need to look how backtrack gets this detail but it’s bound to be a Windows syscall equivalent (forget the name).

Ran that test 1k times on my Linux box and 1k correct outputs. Wonder if the same will be true on the Windows box tomorrow evening. Either way whatever happens to this I’ll look into that again there tomorrow so even if it is something I’ve done hopefully I’ll be able to see what (though I can’t think what that could have been at present).

Let's just retry and see what happens.

@bors retry