rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.34k stars 1.53k forks source link

rc_buffer lint is dangerous as it changes runtime behaviour #6170

Open sdroege opened 3 years ago

sdroege commented 3 years ago

clippy warns if one tries to use a type like Rc<Vec<T>>, e.g.

Arc<Vec<u8>>
^^^^^^^^^^^^ help: try: `Arc<[u8]>`

This is dangerous as creating a Rc<[T]> causes a memcpy of the whole memory area with all the elements, while creating an Rc<Vec<T>> simply copies over the 3 pointers inside the Vec without copying all the elements.

A better suggestion would probably be Rc<Box<[T]>> as that has the same runtime behaviour, but even independent of that it can be useful to have a Vec inside an Rc / Arc in combination with the make_mut() / get_mut() API.

The lint should probably be at least disabled by default.


The same also applies to Rc<String> vs Rc<str>.

Kixunil commented 3 years ago

In the specific case of [u8], one might want to use bytes::BytesMut instead of Vec<u8>.

Turns out I misunderstood how BytesMut works. I thought it's stored inline, but it isn't.

cmyr commented 3 years ago

Will second this, I think there are very reasonable cases to use Rc<Vec<T>> along with Rc::make_mut.

GuillaumeGomez commented 8 months ago

Just tried:

use std::sync::Arc;

struct Bar {
    inner: Arc<Vec<u8>>
}

fn foo(x: Arc<Vec<u8>>) {
    let x = Bar { inner: x };
}

fn main() {
    let x: Arc<Vec<u8>> = Arc::new(Vec::new());
    foo(x);
}

The lint was not emitted so I suppose we can close this issue?

sdroege commented 8 months ago

Would be good to understand which change caused this, but yes looks like this can be closed then :)

y21 commented 8 months ago

It doesn't lint on the code anymore because it was moved to restriction in #6128, so it's opt in now. With -Wclippy::rc_buffer it still emits a warning. Which I guess is enough to close this? It's not really a false positive that can be fixed in the lint I don't think, so restriction seems like a reasonable category.

A better suggestion would probably be Rc<Box<[T]>>

IMO this is worse than Rc<[T]> as a general suggestion. Converting a Vec<T> to Box<[T]> is also not free and involves memcpy-ing the data into a new buffer when capacity != len. When this happens, you have double indirection on every element access and you still need to copy the data