Fix handling of Deref in assigning_clones

[Kobzol]

The assigning_clones lint had a special case for producing a bit nicer code for mutable references:

fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
    *mut_thing = Clone::clone(ref_thing);
}
//v
fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
    Clone::clone_from(mut_thing, ref_thing);
}

However, this could break when combined with Deref.

This PR removes the special case, so that the generated code should work more generally. Later we can improve the detection of Deref and put the special case back in a way that does not break code.

Fixes: #12437

r? @blyxyas

changelog: [assigning_clones]: change applicability to Unspecified and fix a problem with Deref.

[blyxyas]

Hmmmm... What if instead of outright removing it, we could check for the Deref lang item?
It would just add a few lines. Here's the code that you can copy and here's LangItem::Deref

[Kobzol]

I mostly wanted a hotfix so that this doesn't break so often for people, and my experience from writing IDE plugins is that this stuff around Deref handling etc. can be incredibly tricky to get right, so it might take some time :)

I will take a look at what you sent me, thanks.

[bors]

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

[Kobzol]

It's a bit more complicated than I thought. This works fine:

// #[derive(Clone)]
struct DerefWrapper(DerefInner);

impl Deref for DerefWrapper {
    type Target = DerefInner;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for DerefWrapper {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}

#[derive(Clone)]
struct DerefInner;

fn clone_into_deref_method(mut a: DerefWrapper, b: DerefInner) {
    *a = b.clone();
    a.clone_from(&b);
}

But it doesn't work when we uncomment the Clone derive on DerefWrapper. So the issue is not Deref per-se, but the problem is if both the Deref wrapper and the Deref target implement Clone. Maybe it is unnecessary to also check for this, as just keeping * should be enough. I'll try.

[Kobzol]

So, some of the stuff works, but I'm having some trouble with recognizing if Deref was actually applied. Using this:

let ty = cx.typeck_results().expr_ty(ref_expr);
let impls_deref = cx.tcx.lang_items().deref_mut_trait().map_or(false, |id| implements_trait(cx, ty, id, &[]));

also fires here:

fn clone_function_lhs_mut_ref(mut_thing: &mut HasCloneFrom, ref_thing: &HasCloneFrom) {
    *mut_thing = Clone::clone(ref_thing);
}

because the LHS has type &ReErased mut HasCloneFrom, which apparently implements Deref, even though HasCloneFrom doesn't.

I tried using typeck_results().expr_adjustments(), but that doesn't actually contain a Deref adjustment even if LHS goes through a Deref type...

[Kobzol]

I pushed my code. The second commit shows that some unnecessary &mut * is being generated.

[gibfahn]

Hmmmm... What if instead of outright removing it, we could check for the Deref lang item?

Could we remove the special-case first, and then continue this PR to add it back later? Having clippy autofix working code to broken code (even on nightly) feels like a "revert first, improve later" situation to me (as a user).

[Kobzol]

Could we remove the special-case first, and then continue this PR to add it back later?

I agree, that's why I changed the applicability here. Therefore this lint should no longer autofix.

[bors]

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

[xFrednet]

Hey @Kobzol, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[Kobzol]

Well, it was waiting for a review :) I'll try to rebase it.

[xFrednet]

Ahh okay, I guessed that but wasn't sure. Then let's pick a new reviewer:

r? clippy

[xFrednet]

r? clippy

[xFrednet]

Hey @Alexendoo, rustbot has picked you as a new reviewer. If you don't have the time, you can give the PR to me. Just comment r? @xFrednet

[Kobzol]

Rebased. It's still WIP, but I'd appreciate some feedback on the approach that I have used here.

[Alexendoo]

Great, thank you!

@bors r+

[bors]

📌 Commit e9852cc has been approved by Alexendoo

It is now in the queue for this repository.

[bors]

⌛ Testing commit e9852cc with merge 533c377...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 533c377 to master...