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

The "rc_buffer" lint is probably wrong for inner Vec types which implement Clone or Copy #6359

Open
Fishrock123 opened this issue Nov 21, 2020 · 3 comments
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@Fishrock123
Copy link
Contributor

I previously landed a clarification that this lint is not always correct, depending on the use-case: #6090

Today I discovered that Rc::make_mut() and Arc::make_mut() exist for Rc<T: Clone> and Arc<T: Clone> respectively.

The rc_buffer lint says that you can only get a mutable reference if there are no mutable references or else panic, which is not true due to these functions. Copy-on-write is perfectly reasonable for types which are Clone. See also http-rs/tide#747.

I'm willing to make a change to only have this lint apply when T is not Clone if someone can point me in the direction of how to do that. It looks like there are 4 lint sites, two of which are for slices. The lint should also be updated to mention make_mut, which could be done either in the same PR or separately.

@Fishrock123 Fishrock123 added the C-bug Category: Clippy is not doing the correct thing label Nov 21, 2020
@Fishrock123
Copy link
Contributor Author

Ok I have formulated some additional thoughts on this lint:

For slices Rc<&[T]> is always perfectly valid and should not warn. It can't be mutated anyways. Rc<&mut [T]> on the other hand should warn unless somehow impl<T: SomeBounds + Clone> Clone for &mut [T] which I think is probably rare enough to ignore, unless it specifically comes up.

For Vecs, Rc<Vec<T: Clone>> and Rc<Vec<T: Copy>> should be fine and should not warn - clone-on-write via make_mut is a perfectly reasonable for these.

Given a lot of the cases of this lint may be valid, maybe it should be allow by default if implementing that isn't easy?

@Fishrock123 Fishrock123 changed the title The "rc_buffer" lint is probably wrong for inner types which implement Clone The "rc_buffer" lint is probably wrong for inner Vec types which implement Clone or Copy Nov 24, 2020
@ecton
Copy link

ecton commented Nov 24, 2020

I feel like Arc<T> shouldn't trip this warning, regardless of Clone, due to the pattern mentioned in Known Problems on the lint description. This lint triggered in multiple of my source-bases due to me using Arc to share pre-built data structures between threads. It seems like a common use case, but the reason it didn't trip in most of my Arc's is that it's rare for me to use Arc<Vec<T>> instead of Arc<SomeType> where SomeType contains a Vec instead. For example:

struct Wrapper<T> {
    items: Vec<T>,
}

struct Test {
    // no warning
    items: Arc<Wrapper<i8>>,
    // warning
    bad_items: Arc<Vec<i8>>,
}

In terms of potential waste/overhead that the lint is aiming to warn against, both usages of Arc here pose the same potential waste of space. Ultimately the waste is dependant upon whether the Vec was constructed in such a way that the capacity equals the length, and fixed-size arrays aren't viable for most of my data structures.

I'll add a caveat here that I don't think I'm an expert on Rust, and if I'm misusing Arc or misunderstanding the overhead issues, I'd love to know.

@ChrisJefferson
Copy link

I just want to say I'm also using make_mut extensively to do reference-counted copy-on-write, so this warning is quite annoyed. Would it be possible/reasonable to check if there are any make_mut / get_mut in the program (I realise this is very general), and if so not warn about this?

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
Projects
None yet
Development

No branches or pull requests

3 participants