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

Uplift MaybeUninit::uninit().assume_init() lint from clippy #75968

Closed
Lokathor opened this issue Aug 27, 2020 · 8 comments
Closed

Uplift MaybeUninit::uninit().assume_init() lint from clippy #75968

Lokathor opened this issue Aug 27, 2020 · 8 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Lokathor
Copy link
Contributor

Lokathor commented Aug 27, 2020

Using MaybeUninit::uninit().assume_init() is instant UB unless the target type is itself composed entirely of MaybeUninit (eg: [MaybeUninit<u8>; 10], or similar).

It's also the most common thing for a person to try when using MaybeUninit for the first time. I don't think that Rust has any footgun bigger than this single expression.

Clippy has a lint against this, and the lint needs to be uplifted into the compiler.

Essentially the lint is: if the target type of this expression isn't itself some sort of MaybeUninit, then deny by default.

@jyn514 jyn514 added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Aug 27, 2020
@scottmcm
Copy link
Member

Even the [MaybeUninit<u8>; 10] example wouldn't need to use this pattern if uninit_array were to stabilize.

@Lokathor
Copy link
Contributor Author

Ideally once there's a complete confidence that there's no false positives, we upgrade the deny by default into either a hard error, or maybe into an instant runtime panic.

But that might be a controversial step?

@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 27, 2020
@RalfJung
Copy link
Member

We already have a lint for this: invalid_values.

Using MaybeUninit::uninit().assume_init() is instant UB unless the target type is itself composed entirely of MaybeUninit (eg: [MaybeUninit; 10], or similar).

That's not true. Any union instead of MaybeUninit will also work. And ZST are not insta-UB either.

@RalfJung

This comment has been minimized.

@Lokathor
Copy link
Contributor Author

Fair point on unions and zsts

@RalfJung
Copy link
Member

I am working on strengthening that existing lint in #71274. But there's a lot of crater regressions, so help would be appreciated.

Ouch, that was silly of me... we actually have two things that try to guard against such bugs, a lint and runtime panics. However, the runtime panics only apply to mem::uninitialized(), not to MaybeUninit::uninit().assume_init(), as the latter case cannot easily be detected at runtime. The PR is about the latter.


So regarding the lint, I think it is in a pretty good shape already. The one thing it does not do is warn about uninitialized integers, raw pointers and floats. The reason for this is that rust-lang/unsafe-code-guidelines#71 is still open, so I didn't feel like we should already assert that uninit ints are UB when the lang team has not firmly made that decision yet.

If there are any other cases of MaybeUninit::uninit().assume_init() that rustc does not warn about, please let me know. Some enums are tricky to handle, but with concrete examples we can make the lint properly understand more of them.

@RalfJung
Copy link
Member

RalfJung commented Sep 27, 2020

@Lokathor what is the actionable part of this issue?

As I said above, we already have such a lint in rustc, and I'd be happy to improve it given examples where it should fire but does not.
But I checked the clippy lint, and I think it is way too aggressive for a rustc lint -- it warns about many cases that are definitely fine (like an uninitialized repr(transparent) newtype around MaybeUninit), and in cases where it cannot know if things are actually problematic (like when there are generics involved).

It also warns about some cases that are okay with the current rustc layout strategy but not if that ever changes, like

enum Univariant {
  V(MaybeUninit<bool>)
}

I wanted to be conservative when adding the lint and not turn these into errors, but if T-lang is on board I would be fine with triggering the lint for all enums.

@Lokathor
Copy link
Contributor Author

Well, alright then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants