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

[!; 0] incorrectly considered uninhabited #47563

Closed
dtolnay opened this issue Jan 19, 2018 · 13 comments
Closed

[!; 0] incorrectly considered uninhabited #47563

dtolnay opened this issue Jan 19, 2018 · 13 comments
Labels
A-type-system Area: Type system C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Jan 19, 2018

#![feature(never_type)]

enum Helper<T, U> {
    T(T, [!; 0]),
    #[allow(dead_code)]
    U(U),
}

fn transmute<T, U>(t: T) -> U {
    let Helper::U(u) = Helper::T(t, []); // ??
    u
}

fn main() {
    println!("{:?}", transmute::<&str, (*const u8, u64)>("type safety"));
}

Output on my machine as of rustc 1.25.0-nightly (3bd4af8 2018-01-18):

(0x55fd17861801, 11)

Mentioning @eddyb and @arielb1 who were involved with #45225.
Mentioning the never_type tracking issue #35121.

@dtolnay dtolnay added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Jan 19, 2018
@eddyb
Copy link
Member

eddyb commented Jan 19, 2018

#45225 always considers arrays inhabited, so it's not from there, but rather match checking.
cc @rust-lang/compiler @canndrew

@canndrew
Copy link
Contributor

That's weird. The inhabitedness-checking function explicitly checks whether arrays have size zero or not: https://github.com/rust-lang/rust/blob/master/src/librustc/ty/inhabitedness/mod.rs#L265

@kennytm kennytm added A-type-system Area: Type system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jan 19, 2018
@eddyb
Copy link
Member

eddyb commented Jan 19, 2018

@canndrew That check is incorrect, it should default to inhabited if to_const_int or to_u64 return None.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 19, 2018

That's a bug but it doesn't fix this bug right? Here it seems like len would be Some(0).

@eddyb
Copy link
Member

eddyb commented Jan 19, 2018

@dtolnay The length might be unevaluated. If it's possible to evaluate, sure, this code should trigger that, but it's incorrect to assume that the type is uninhabited if the length isn't known yet.

@varkor
Copy link
Member

varkor commented Jan 19, 2018

The issue is exactly as @eddyb suggests: the length is unevaluated in the program at the point uninhabited_from is called, so it then incorrectly looks at the inhabitedness of the array type because len.val.to_const_int() == None.

@arielb1 arielb1 added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Jan 19, 2018
@canndrew
Copy link
Contributor

@varkor A problem with your fix is that @dtolnay's code now fails to compile if you make the array have length 1. eg. I would expect this to compile fine:

#![feature(never_type)]

enum Helper<T, U> {
    T(T, [!; 1]),
    #[allow(dead_code)]
    U(U),
}

fn make_the_array() -> [!; 1] {
    panic!("whoops!");
}

fn transmute<T, U>(t: T) -> U {
    let Helper::U(u) = Helper::T(t, make_the_array()); // ??
    u
}

fn main() {
    println!("{:?}", transmute::<&str, (*const u8, u64)>("type safety"));
}

I think we should still merge it to fix the soundness bug, but why don't we know the value of the constant expression there?

@varkor
Copy link
Member

varkor commented Jan 20, 2018

This is an issue with the constant evaluation rather than the fix itself. For some reason, the sizes do not seem to have been evaluated by the time the inhabitedness checks are called. This seems like a bug, and I want to look into when I get some time.

@eddyb
Copy link
Member

eddyb commented Jan 20, 2018

This isn't really about ordering, but rather a lack of normalization. <&! as Deref>::Target, for example, would exhibit the same problem.

@canndrew
Copy link
Contributor

There seems to be a lot of bugs related to things not getting normalized. I know I've encountered a few before. Are they all the same bug?

@eddyb
Copy link
Member

eddyb commented Jan 21, 2018

@canndrew Depends. I've been trying to get @nikomatsakis to upstream his lazy normalization work, which would at least make it clear who's responsibility is to normalize anything (if you are traversing types and hit a TyProjection or TyAnon, or want the length of TyArray).

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 25, 2018
@nikomatsakis
Copy link
Contributor

triage: P-high

Regression, though only with never_type feature -- but we'd like to stabilize that!

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Jan 25, 2018
@varkor
Copy link
Member

varkor commented Jan 25, 2018

The fix is just waiting on bors. Should be in soon!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 25, 2018
Fix type inhabitedness check for arrays

Arrays of uninhabited types were considered to also be uninhabited if
their length had not been evaluated, causing unsoundness.

Fixes rust-lang#47563.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

8 participants