Fix cloned_ref_to_slice_refs FN on to_owned()

[profetia]

Closes #16320

changelog: [cloned_ref_to_slice_refs] fix FN on to_owned()

[rustbot]

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[xtqqczze]

There are more cases of implicit clone, see: #16320 (comment).

[profetia]

Added. Thank you for the advice!

[profetia]

This lint also has the macro expansion problem. The fix now is also included in this PR.

[Jarcho]

This needs to check the result type. &[x.to_string()] has a clone only if x is a string. This is the same for every clone-like function.

View changes since this review

[profetia]

Can you explain more? This lint does not check for string originally

[Jarcho]

Lots of types implement ToString, but only <String as ToString>::to_string is actually a clone. A similar issue happens with Path::to_path_buf which is only a clone when the receiver is a derefed PathBuf. Depending on the method you'll have to check either the adjusted or unadjusted type against the result type.

[xtqqczze]

Right, it would be a false positive to trigger in the following case:

#[derive(Clone)]
struct A(i32);

impl std::fmt::Display for A {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

pub fn main() {
    let a = A(42);
    _ = &[a.to_string()];
}

We need to ensure return_type == input_type, like implicit_clone does.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[profetia]

Updated thanks to @xtqqczze's advices.

[Jarcho]

Should just be let Some(parent_def) = since you need to unwrap is later.

[Jarcho]

This will have FNs when custom deref adjusments occur. You will also have a broken suggestion when the initial type is double reference (other than to_owned, that works the same way clone does).

You'll need something like:

let mut ty = expr_ty(recv);
let mut deref_count = 0u32;
let mut has_custom = false;
let mut adjustments = expr_adjustments(recv).iter();
while ty != target_ty {
  if let Some(adjust) = adjustments.next()
    && let Adjust::Deref(kind) = adjust.kind
  {
    ty = adjust.ty;
    deref_count += 1;
    has_custom |= kind.is_some();
  } else {
    return None
  }
}

let needs_borrow = deref_count == 0 || has_custom
deref_count -= (!has_custom) as u32;
return Some((deref_count, needs_borrow));

[profetia]

Emm.. I'm comparing the orignial type (i.e. expr_ty()) not the adjusted one (i.e. expr_ty_adjusted()), so I think custom deref should not be a problem