Warn if 'nonstandard Clone' used in vec!

[ralfbiedert]

Background

Here is the essence of a bug that took me several hours to find in a larger code base. The code originally came from a much larger, multi-threaded function that fed data into a machine learning system. Instead of asserting, it just produced inputs that subtly changed other outputs.

use std::sync::{Arc, Mutex};

#[derive(Default, Clone)]
struct Entry<T> {
    id: usize,
    data: Arc<Mutex<T>>,
}

pub fn main() {
    let mut entries = vec![Entry::default(); 10];

    for (i, e) in entries.iter_mut().enumerate() {
        e.id = i;
        *e.data.lock().unwrap() = i;
    }

    let entry_0 = &entries[0];

    assert_eq!(entry_0.id, *entry_0.data.lock().unwrap());
}

In our project we had the following lints and warning enabled:

#![forbid(unsafe_code)]
#![warn(clippy::all)]
#![warn(clippy::nursery)]
#![warn(clippy::pedantic)]
#![allow(clippy::inline_always)] // Globally allow "inline" because we do it too much.
#![allow(clippy::module_name_repetitions)] // We have too many of these

Analysis

The bug comes from combining a struct S {} that contains an Arc with the vec![T; N] macro. The official documentation even warns against this sort of behavior:

This will use clone to duplicate an expression, so one should be careful using this with types having a nonstandard Clone implementation. For example, vec![Rc::new(1); 5] will create a vector of five references to the same boxed integer value, not five references pointing to independently boxed integers.

However, my intuition is that most people are so used to vec![S; 10] meaning "give me 10 independent instances of S" that this 'nonstandard case' will be somewhat surprising.

Suggestion

This bug could have been avoided if Clippy warned against known 'nonstandard' Clone implementations used in vec!. For starters, that could be any call to vec![T; N], where T contains an Arc or Rc.

[basil-cow]

I think we wouldn't want to fire the lint if some type on the path to the Arc has a custom Clone implementation, or should we lint that anyway?

[ralfbiedert]

I'm not sure.

If the lint isn't 100% accurate I'd probably rather have a false positive I can silence, than having subtle errors caused by the combination of two innocent constructs possibly far apart.

On the other hand, I wouldn't want too many false positives, and I don't know how prevalent those constructs are.

[basil-cow]

Ok, I'll write a draft sometime soon and feed it some codebases to see how common it is

[camsteffen]

Same as #3377, except the Arc is buried in a struct.