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

Unions with uninhabited variants are considered uninhabited #46845

Closed
gereeter opened this issue Dec 19, 2017 · 5 comments
Closed

Unions with uninhabited variants are considered uninhabited #46845

gereeter opened this issue Dec 19, 2017 · 5 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta.

Comments

@gereeter
Copy link
Contributor

Uninhabited variants should ideally be irrelevant to the representation, just like in enums.

As a test, this prints out 0 instead of 8:

#![feature(never_type)]
use std::mem;

union Foo {
    a: u64,
    b: !
}

fn main() {
    println!("{}", mem::size_of::<Foo>());
}

This happens because rustc considers unions more like structs than enums, and the test for uninhabitedness sees a single variant with two fields, one of which is uninhabited.

I have a fix for this, but it's slightly easier to bundle it with some other changes I'm working on.

@durka
Copy link
Contributor

durka commented Dec 19, 2017

This becomes a problem when making an array of unions:

#![allow(unused)]
use std::mem;

#[derive(Copy, Clone)]
enum Never {}

union Foo {
    a: u64,
    b: Never
}

fn main() {
    println!("{}", mem::size_of::<Foo>());
    
    let f = [Foo { a: 42 }, Foo { a: 10 }];
    println!("{:?}", unsafe { f[0].a });
}

The above code represents a regression (prints 42 on stable, garbage on beta/nightly).

@gereeter
Copy link
Contributor Author

I didn't realize it was a regression to beta - in that case I'll put up an independent fix so that it can be backported if desired.

@gereeter
Copy link
Contributor Author

gereeter commented Dec 19, 2017

Although I can't trigger it on the playground, you code is actually triggering an ICE for me:

error: internal compiler error: /checkout/src/librustc_trans/mir/place.rs:418: using operand local _19 as place

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.24.0-nightly (dc39c3169 2017-12-17) running on x86_64-unknown-linux-gnu

EDIT: I bisected the ICE to somewhere between "rust 1.23.0-nightly (e97ba83 2017-11-25)" and "rust 1.23.0-nightly (827cb0d 2017-11-26)".

@arielb1 arielb1 added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 19, 2017
@arielb1
Copy link
Contributor

arielb1 commented Dec 19, 2017

cc @eddyb

@gereeter
Copy link
Contributor Author

I put up a PR to fix this, but I'm still confused about the ICE (which I moved to #46855).

bors added a commit that referenced this issue Dec 22, 2017
Bounce out the layout refactor from beta

@eddyb's #45225 was supposed to get into get into 1.24, but due to an ordering mistake, it had snuck into 1.23.

That wide-effect translation-changing PR had poked LLVM's weak corners and caused many regressions (3 of them have fixes I include here, but also #46897, #46845, #46449, #46371). I don't think it is a good idea to land it in the beta (1.23) because there are bound to be some regressions we didn't patch.

Therefore, I am reverting it in time for stable, along with its related regression fixes.

r? @michaelwoerister (I think)
bors added a commit that referenced this issue Dec 24, 2017
Only mark unions as uninhabited if all of their fields are uninhabited

Fixes #46845.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta.
Projects
None yet
Development

No branches or pull requests

3 participants