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

rc_buffer lint is dangerous as it changes runtime behaviour #6170

Open
sdroege opened this issue Oct 13, 2020 · 5 comments
Open

rc_buffer lint is dangerous as it changes runtime behaviour #6170

sdroege opened this issue Oct 13, 2020 · 5 comments
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@sdroege
Copy link

sdroege commented Oct 13, 2020

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>.

@sdroege sdroege added the C-bug Category: Clippy is not doing the correct thing label Oct 13, 2020
@Kixunil
Copy link

Kixunil commented Oct 18, 2020

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
Copy link

cmyr commented Nov 19, 2020

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

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy L-suggestion Lint: Improving, adding or fixing lint suggestions labels Dec 6, 2020
@GuillaumeGomez
Copy link
Member

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
Copy link
Author

sdroege commented Jan 19, 2024

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

@y21
Copy link
Member

y21 commented Jan 19, 2024

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

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 good-first-issue These issues are a good way to get started with Clippy L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

6 participants