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

Suggestion to replace std::mem::uninitialized with MaybeUninit is not actionable #65432

Closed
gnzlbg opened this issue Oct 15, 2019 · 5 comments
Closed
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 15, 2019

When Foo below is defined in a crate outside your control, code that currently does this:

struct Foo(&'static i32);
fn main() {
    let _x: Foo = unsafe { std::mem::uninitialized() };
}

(Playground)

gets a warning suggesting to use MaybeUninit instead

   Compiling playground v0.0.1 (/playground)
warning: the type `Foo` does not permit being left uninitialized
 --> src/main.rs:4:28
  |
4 |     let _x: Foo = unsafe { std::mem::uninitialized() };
  |                            ^^^^^^^^^^^^^^^^^^^^^^^^^
  |                            |
  |                            this code causes undefined behavior when executed
  |                            help: use `MaybeUninit<T>` instead
  |
  = note: `#[warn(invalid_value)]` on by default
note: References must be non-null (in this struct field)
 --> src/main.rs:1:12
  |
1 | struct Foo(&'static i32);
  |            ^^^^^^^^^^^^

    Finished dev [unoptimized + debuginfo] target(s) in 0.76s
     Running `target/debug/playground`

This warning is currently not actionable, since Foo cannot be modified, and MaybeUninit<T> does not support T: !Copy.

Instead, the suggestion "help: use MaybeUninit<T> instead" should only be shown if T: Copy.

A different suggestion could be shown, suggesting to change the type definition instead, and maybe reminding the user that their code exhibits undefined behavior, even though there is nothing that the user might be able to do about this.

@Centril
Copy link
Contributor

Centril commented Oct 15, 2019

Instead, the suggestion "help: use MaybeUninit<T> instead" should only be shown if T: Copy.

What I would do is add a note instead if T: !Copy: "note: T is not Copy, perhaps it should be?"
Seems cleaner to implement that way.

@Centril Centril added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 15, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 15, 2019

What I would do is add a note instead if T: !Copy: "note: T is not Copy, perhaps it should be?"
Seems cleaner to implement that way.

So what if T not being Copy is correct? What note would we add for that ?

@Centril
Copy link
Contributor

Centril commented Oct 15, 2019

It's a lint; it doesn't have to be perfect (it says "perhaps") -- I think the user can do some thinking as well to see if the suggestion is appropriate or not. Also, I don't see how your suggestion is materially different... the same situation may arise there.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 15, 2019

This is wrong. MaybeUninit is not a Rust union, but a "magic" type instead.

@gnzlbg gnzlbg closed this as completed Oct 15, 2019
@RalfJung
Copy link
Member

RalfJung commented Oct 15, 2019

To be clear for anyone reading along, MaybeUninit<T> works just fine for any T, no matter whether it is Copy.

MaybeUninit is not a Rust union, but a "magic" type instead.

That's wrong:

pub union MaybeUninit<T> {
    uninit: (),
    value: ManuallyDrop<T>,
}

This is in accordance with an accepted RFC, see #55149. Sure, it uses an unstable feature, but that's not "magic" -- that's the same as Pin<Self> receivers and #[non_exhaustive] and a dozen other things libstd uses. And beyond that there's some true "magic" in libstd when it uses mechanisms that are not even meant to be stabilized in their current form (like the dropck eyepatch, or unsized coercions through types like Rc).

It's just not (and has never been) a valid assumption that whatever holds true for stable Rust, holds true for libstd. Nothing new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants