impl Error for !

[canndrew]

impl Error for ! { ... }

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

@bors: r+ 7df8271

Thanks!

[eddyb]

@bors r-

Travis results in "Aborted (core dumped)" while running std tests.

[canndrew]

Not sure what's causing this. The test which is aborting is here. It aborts at the very start of the spawned thread. Specifically, I can see execution get to thread_start, then start_thread, then call_box starts unwinding (but without executing the closure that was passed to spawn). After that, _Unwind_Resume gets called from thread_start and aborts.

I'm guessing the problem has something to do with the closure returning ! with feature(never_type) enabled by this PR. I haven't been able to reproduce the bug in isolation though.

Can anyone suggest a good next step for debugging this? I'd like to know why it starts unwinding to begin with.

[alexcrichton]

I suspect that's bad but not this PR bad, let's see what bors has to say:

@bors: r+

[bors]

📌 Commit 7df8271 has been approved by alexcrichton

[bors]

⌛️ Testing commit 7df8271 with merge f380135...

[bors]

💔 Test failed - auto-win-gnu-32-opt-rustbuild

[canndrew]

Damn. I dunno what to make of that log though, both the panics that it mentions at the bottom are panics that were supposed to occur.

[alexcrichton]

Hm that failed at about the same location as travis, so maybe not spurious?

[canndrew]

Yeah I'd say it's getting to this test again and aborting. Any idea why? Is there any reason why a closure would start panicking as soon as it's called?

[alexcrichton]

Maybe? Not sure. Best next option is likely to reproduce locally so we're not just poking in the dark.

[canndrew]

Is there an easy way to get the make system to spit out MIR and LLVM code for the libstd tests? I don't know how to reproduce this locally.

By the way, it is caused by #![feature(never_type)] being enabled on the crate, not by the impl.

[alexcrichton]

LLVM IR can be emitted with --emit llvm-ir, as for MIR though I'm not entirely sure (cc @eddyb)

[eddyb]

MIR is --unpretty=mir -Zunstable-options or -Zdump-mir=PreTrans.
The latter dumps it all in the current dir, one file per function. You can change PreTrans to any pass name you want to get before and after MIR.

[canndrew]

I just had another look at this. It looks like it crashes here when it tries to use mem::uninitialized to create an uninitialised !.

[canndrew]

Here's how I fixed this: In the old code, panicking::try took a type argument R and created an R using mem::uninitialized():

let r: R = mem::uninitialized();

Later, it either reads the value out of r or calls mem::forget on it depending on whether a value was written there.

When R is ! though, this could cause the code to crash. This is fair enough since having an invalid value around is undefined behavior and R doesn't have any valid values. So instead I wrapped it in a union:

union MaybeUninitialized<R> {
    data: R,
    _uninitialized: (),
}

let r: MaybeUninitialized<R> = mem::uninitialized();

This is fine since a MaybeUnitialized<R> is always valid since it's _uninitialized field always contains a valid value.

However this does raise a problem: until now we've always just assumed that mem::uninitialized is okay to use so long as the uninitialized data is never read and is always overwritten or forgotten. But in the presence of ! this doesn't seem to be true, at least not with the current implementation.

[alexcrichton]

Hm in theory we shouldn't have to change the code for something like this, doesn't that mean a bug in the compiler?

[eddyb]

@alexcrichton Sadly, no. Unsafe code everywhere using closures can have subtle dependencies on the closures returning an inhabited type, before the closure has a chance to stop the control-flow.
This is one of those things that is hard to track down and we need to build tools to analyze the ecosystem.

cc @rust-lang/compiler

[canndrew]

I didn't even have to use uninitialized in the final code, I could have initialized the union as MaybeUninitialized { _uninitialized: () }. This seems like a better way of writing this sort of code since uninitialized::<!> is a diverging function (has type fn() -> !).

Edit: Thinking about this a little more I can't see any situation where it would be better to use mem::uninitialized rather than this MaybeUninitialized contraption. After all, MaybeUninitialized reflects what's actually going on - either you have a value here or you have nothing (ie. ()), and you're keeping track of which state you're in somewhere else. Plus it doesn't cause problems with uninhabited types. Maybe we should just add it to the standard library and deprecate mem::uninitialized.

[canndrew]

An alternative would be to change the way divergence works for unsafe functions and say that just because it returns an uninhabited type doesn't mean it never returns because unsafe functions can return invalid instances of a type. That seems like a much messier solution though.

[alexcrichton]

@eddyb it seems... bad though if we add ! and suddenly all unsafe code ever written now needs to be re-audited to be correct? Surely there's somewhere else in the standard library this applies to as well at least?

[eddyb]

@alexcrichton Yeah it's not ideal, we'll have to discuss it. @rust-lang/lang is probably more appropriate.

EDIT: tagged both for good measure.

[canndrew]

One way to avoid an audit would be to put a T: Inhabited bound on uninitialized.

It's a pretty fundamental problem though. Functions that return an uninhabited type don't return at all. Except uninitialized does return because it doesn't necessarily return a sane value. It's kind of a weird, type-system-defying intrinsic.

This doesn't just effect ! either. The only difference between ! and any other uninhabited type should be the invisible coercions except right now there are some places in the compiler that check for ! specifically when they could check for uninhabited types generally. If those places ever get fixed this bug will probably spread to Void as well (if it doesn't effect it all ready).

[aturon]

cc https://internals.rust-lang.org/t/mem-uninitialized-and-trap-representations/4167/1

[alexcrichton]

Ok, I'm going to close this for now in favor of that internals thread (in the interest of clearing out the queue), but we can certainly reopen/resubmit once that's handled!