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

unnecessary_to_owned suggestion changes maybe-cloning to always-cloning #12806

Open
kpreid opened this issue May 15, 2024 · 0 comments
Open

unnecessary_to_owned suggestion changes maybe-cloning to always-cloning #12806

kpreid opened this issue May 15, 2024 · 0 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@kpreid
Copy link
Contributor

kpreid commented May 15, 2024

Summary

When iterating over the contents of a Cow<'a, [T]> by value, clippy::unnecessary_to_owned confidently proposes a change which will cause unnecessary element cloning in the case where the Cow is Cow::Owned. This isn't necessarily worse, but it isn't strictly better and shouldn't be recommended without qualification.

Lint Name

unnecessary_to_owned

Reproducer

I tried this code:

use std::borrow::Cow;

struct NoisyClone<T>(T);

impl<T: Clone + std::fmt::Debug> Clone for NoisyClone<T> {
    fn clone(&self) -> Self {
        let value = &self.0;
        println!("Cloning {value:?}");
        Self(value.clone())
    }
}

fn main() {
    case1(Cow::Owned(vec![NoisyClone(100)]));
    case2(Cow::Owned(vec![NoisyClone(200)]));
    case1(Cow::Borrowed(&[NoisyClone(102)]));
    case2(Cow::Borrowed(&[NoisyClone(202)]));
}

/// Original code
fn case1(input: Cow<'_, [NoisyClone<i32>]>) -> Vec<i32> {
    input.into_owned().into_iter().map(|nc| nc.0).collect()
}

/// Code proposed by Clippy
fn case2(input: Cow<'_, [NoisyClone<i32>]>) -> Vec<i32> {
    input.iter().cloned().map(|nc| nc.0).collect()
}

I saw this happen:

warning: unnecessary use of `into_owned`
  --> src/bin/cow_clippy.rs:20:5
   |
20 |     input.into_owned().into_iter().map(|nc| nc.0).collect()
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `input.iter().cloned()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned
   = note: `#[warn(clippy::unnecessary_to_owned)]` on by default

Both versions of the code have flaws, but neither one is strictly superior to the other, and case1 is likely better, so Clippy should not confidently recommend one.

  • case1, when given Borrowed, will unnecessarily create a Vec (rather than only cloning the elements).
  • case2, when given Owned, will unnecessarily clone all the elements (rather than moving them).

Version

rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: x86_64-apple-darwin
release: 1.78.0
LLVM version: 18.1.2

Additional Labels

No response

@kpreid kpreid added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

1 participant