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

clippy::uninit_vec (probable) false positive for Vec<UnsafeCell<MaybeUninit<T>>> #13364

Closed
schuetzm opened this issue Sep 7, 2024 · 3 comments · Fixed by #13367
Closed

clippy::uninit_vec (probable) false positive for Vec<UnsafeCell<MaybeUninit<T>>> #13364

schuetzm opened this issue Sep 7, 2024 · 3 comments · Fixed by #13367
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

@schuetzm
Copy link

schuetzm commented Sep 7, 2024

Summary

AFAIU an UnsafeCell<MaybeUninit<T>> doesn't need to be initialized, although I couldn't find anything definite about this. clippy::uninit_vec should therefore not trigger on a vector with this element type.

Lint Name

clippy::uninit_vec

Reproducer

I tried this code:

fn main<T>() {
    let mut data: Vec<std::cell::UnsafeCell<std::mem::MaybeUninit<T>>> = Vec::with_capacity(1);
    unsafe {
        data.set_len(1);
    }
}

I saw this happen:

error: calling `set_len()` immediately after reserving a buffer creates uninitialized values
 --> src/xxx.rs:2:5
  |
2 |     let mut data: Vec<std::cell::UnsafeCell<std::mem::MaybeUninit<T>>> = Vec::with_capacity(1);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3 |     unsafe {
4 |         data.set_len(1);
  |         ^^^^^^^^^^^^^^^
  |
  = help: initialize the buffer or wrap the content in `MaybeUninit`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
  = note: `#[deny(clippy::uninit_vec)]` on by default

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

No response

@schuetzm schuetzm 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 Sep 7, 2024
@y21
Copy link
Member

y21 commented Sep 7, 2024

You mention UnsafeCell<MaybeUninit<T>> but your reproducer uses u8 as the element type. Is that intentional?

The lint already checks if the element type is valid to be left in an uninitalized state and won't lint, so your repro wouldn't emit a warning for UnsafeCell<MaybeUninit<u8>> as the element type

@schuetzm
Copy link
Author

schuetzm commented Sep 7, 2024

@y21 Thanks, that was stupid. I've updated the example.

Interestingly enough, this version is accepted:

let mut data: Vec<std::cell::UnsafeCell<std::mem::MaybeUninit<u8>>> = Vec::with_capacity(1);
unsafe {
    data.set_len(1);
}

Obviously the innermost type should make no difference here, right?

What I'm unsure about is whether UnsafeCell<MaybeUninit<T>> needs to be initialized. Intuitively, I'd say it doesn't, as it has the same memory layout as just MaybeUninit<T>.

@y21
Copy link
Member

y21 commented Sep 7, 2024

Yeah, I realized after some testing that the reproducer was probably just a mistake and that there's indeed a false positive if you use a generic instead of a concrete type like u8.

The difference between the two is that the UnsafeCell<MaybeUninit<u8>> case is handled by rustc's uninit check which is more precise, however it fails when generic types are involved like UnsafeCell<MaybeUninit<T>>, in which case clippy uses a less precise fallback that would previously not allow structs. I've put up a PR that handles that case in #13367.

To my knowledge that's fine, structs that entirely consist of types that can be left uninitialized can themselves be left uninitialized as well (which applies to UnsafeCell<MaybeUninit<T>>). Regardless of memory layout.

@bors bors closed this as completed in 938f8ba Sep 9, 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

Successfully merging a pull request may close this issue.

2 participants