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

Lint against MaybeUninit::uninit().assume_init() #4272

Closed
ExpHP opened this issue Jul 14, 2019 · 7 comments · Fixed by #4479
Closed

Lint against MaybeUninit::uninit().assume_init() #4272

ExpHP opened this issue Jul 14, 2019 · 7 comments · Fixed by #4479
Labels
A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@ExpHP
Copy link

ExpHP commented Jul 14, 2019

I spotted this in PyO3: (PyO3/pyo3#536)

if next_idx == 0 {
    self.inner
        .push_back(unsafe { mem::MaybeUninit::uninit().assume_init() });
}

GitHub search brings up a total of 62 matches even for a fairly restrictive search query: https://github.com/search?q=%22uninit%28%29%3A%3Aassume_init%28%29%22&type=Code

Problem: It seems people are using this as an easy way out of the deprecation warnings for std::mem::uninitialized, failing to understand the reason why it was deprecated (it is, to my current understanding, universally UB, always!). The correct solution is to use MaybeUninit<T> where you were formerly using T until the value is known to be initialized. The MaybeUninit<T> docs even give examples for what are by far the two most common use cases (out pointers and partially-initialized arrays).

Proposal: There should either be a warn-by-default or an error-by-default lint against immediate calls to assume_init() on MaybeUninit::uninit() with no other intervening operations.

Possible extension: We can try going a step further and adjust our suggestions based on the type:

  • In the case where T is uninhabited, we can instead recommend the use of std::hint::unreachable_unchecked.
  • If T = [A; N], we can specifically recommend that the user constructs a [MaybeUninit<A>; N]. (as this is often a key piece of the correct solution that the user might not think of on their own, and without it they may simply invent some other way to silence the warning improperly)
@ExpHP ExpHP changed the title MaybeUninit::uninit().assume_init() Lint against MaybeUninit::uninit().assume_init() Jul 14, 2019
@flip1995 flip1995 added L-correctness Lint: Belongs in the correctness lint group L-suggestion Lint: Improving, adding or fixing lint suggestions A-lint Area: New lints labels Jul 15, 2019
@ExpHP
Copy link
Author

ExpHP commented Jul 22, 2019

Sigh. Just noticed this isn't always UB. The docs even use this pattern to construct a [MaybeUnit<T>; n]. (The whole array, not the individual elements!)

I suppose more generally one can say that if all fields of a type are themselves MaybeUnit, then MaybeUninit::uninit().assume_init() at that type is sound. ZST fields are probably also okay.

@llogiq
Copy link
Contributor

llogiq commented Jul 23, 2019

ZST fields may be absolutely non-OK – for example if they are uninhabited (the poster child here is the ! type), it's unsound to instantiate them.

@ExpHP
Copy link
Author

ExpHP commented Jul 23, 2019

Even though ! is technically implemented as zero-sized, I wouldn't call it a ZST. The term is much more useful as a category if it only includes inhabited types.

(I reckon the line between valid and invalid types of zero-size is pretty clear, with no gray area in-between)

@llogiq
Copy link
Contributor

llogiq commented Jul 23, 2019

For now I think it'd be enough to lint if the target type isn't uninit-compatible, which I define as MaybeUninit<T> where T is inhabitable, or a tuple or area, of uninit-compatible types.

@llogiq
Copy link
Contributor

llogiq commented Jul 23, 2019

Having a MaybeUninit of an uninhabitable type should be a forbidden lint, btw.

@llogiq
Copy link
Contributor

llogiq commented Aug 31, 2019

I'm taking this. It's been far too long since I wrote my last lint.

@Shnatsel
Copy link
Member

Shnatsel commented Nov 5, 2019

There is an open pull request to warn against those in rustc itself: rust-lang/rust#66044

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints L-correctness Lint: Belongs in the correctness lint group L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants