Regression in Error conversion from Infallible

[alexcrichton]

The following code:

use std::convert::{TryFrom, Infallible};

struct E;

impl From<Infallible> for E {
    fn from(_: Infallible) -> E {
        E
    }
}

fn foo() -> Result<(), E> {
    u32::try_from(1u32)?;
    Ok(())
}

compiles on stable but fails to compile on nightly with:

error[E0277]: `?` couldn't convert the error to `E`
  --> src/lib.rs:13:24
   |
13 |     u32::try_from(1u32)?;
   |                        ^ the trait `std::convert::From<()>` is not implemented for `E`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following implementations were found:
             <E as std::convert::From<!>>
   = note: required by `std::convert::From::from`

This was discovered through bytecodealliance/wasmtime#633

[alexcrichton]

Bisection reveals #66607 as the cause, likely #65355

[SimonSapin]

Reduced:

struct E;

impl From<!> for E {
    fn from(_: !) -> E {
        E
    }
}

#[allow(unreachable_code)]
fn foo(never: !) {
    <E as From<!>>::from(never);  // Ok
    <E as From<_>>::from(never);  // Inference fails here
}

(Playground)

[Centril]

(Adding #![feature(never_type_fallback)] resolves the issue.)

[SimonSapin]

The above went through this doc-comment of the desugaring of foo?, which wasn’t easy to find:

rust/src/librustc/hir/lowering/expr.rs

Lines 1199 to 1209 in 91642e3

	/// Desugar `ExprKind::Try` from: `<expr>?` into:
	/// ```rust
	/// match Try::into_result(<expr>) {
	/// Ok(val) => #[allow(unreachable_code)] val,
	/// Err(err) => #[allow(unreachable_code)]
	/// // If there is an enclosing `try {...}`:
	/// break 'catch_target Try::from_error(From::from(err)),
	/// // Otherwise:
	/// return Try::from_error(From::from(err)),
	/// }
	/// ```

[SimonSapin]

(Adding #![feature(never_type_fallback)] resolves the issue.)

That explains where the () comes from, thanks. But it’s not clear to me why fallback should happen in the first place in this code.

[Centril]

Unclear to me also; cc @nikomatsakis and @arielb1 -- also cc #65992

[acfoltzer]

The same result also occurs when the desired error type is transitively From<!>, which seems to come up pretty commonly in the wild since std errors like TryFromIntError are From<!>:

use std::convert::{TryFrom};

struct E;

impl From<!> for E {
    fn from(_: !) -> E {
        E
    }
}

struct F;

impl From<E> for F {
    fn from(_: E) -> F {
        F
    }
}

fn foo() -> Result<(), F> {
    u32::try_from(1u32)?;
    Ok(())
}

error[E0277]: `?` couldn't convert the error to `F`
  --> src/lib.rs:20:24
   |
20 |     u32::try_from(1u32)?;
   |                        ^ the trait `std::convert::From<()>` is not implemented for `F`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following implementations were found:
             <F as std::convert::From<E>>
   = note: required by `std::convert::From::from`

[SimonSapin]

@acfoltzer Your example legitimately does not compile, although the error message is wrong in Nightly because of this bug. From conversions have never worked transitively. The impl From<!> for E does not affect your example, you get the same results without it. The ! here comes from <u32 as TryFrom<u32>>::Error

[acfoltzer]

@SimonSapin Hmm, that wasn't the change I was expecting would make the example compile, but the fact that it doesn't change the behavior is a surprise.

What I thought was the telling change is whether it compiles after removing the impl From<E> for F rather than impl From<!> for E. But it looks like that happens regardless of the From<!>:

use std::convert::TryFrom;

struct E;

struct F;

impl From<E> for F {
    fn from(_: E) -> F {
        F
    }
}

fn foo() -> Result<(), F> {
    u32::try_from(1u32)?;
    Ok(())
}

error[E0277]: `?` couldn't convert the error to `F`
  --> src/lib.rs:14:24
   |
14 |     u32::try_from(1u32)?;
   |                        ^ the trait `std::convert::From<()>` is not implemented for `F`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following implementations were found:
             <F as std::convert::From<E>>
   = note: required by `std::convert::From::from`

vs the following, which compiles fine:

use std::convert::TryFrom;

struct F;

fn foo() -> Result<(), F> {
    u32::try_from(1u32)?;
    Ok(())
}

Maybe this is a distinct issue?

[SimonSapin]

Oooh that’s interesting! Adapting this to the reduced failure from #66757 (comment): removing the impl From<!> for E makes that code compile:

pub struct E;

#[allow(unreachable_code)]
pub fn foo(never: !) {
    <E as From<_>>::from(never);
}

(Playground)

I think what happens in the code that compiles is that the only From<_> trait implemented by E is From<E>, the identity conversion. So the call is <E as From<E>>::from which expects an argument of type E. It works out because ! auto-coerces to E (or anything).

If we add impl From<!> for E (back), now E as From<_> becomes ambiguous. We have a type variable, not fully resolved in type inference, which is then unified with the argument expression passed to from(). That argument has type !, so the type variable is unified with the type !. This unification apparently happens before any coercion has a chance to happen.

Presumably, the compiler then ends up here somehow:

rust/src/librustc/ty/context.rs

Lines 2442 to 2449 in 797fd92

	#[inline]
	pub fn mk_diverging_default(self) -> Ty<'tcx> {
	if self.features().never_type_fallback {
	self.types.never
	} else {
	self.types.unit
	}
	}

[dtolnay]

heim-rs/heim#182 is hitting a variation of this which minimizes to:

fn from(e: std::convert::Infallible) -> Box<dyn std::error::Error> {
    Box::new(e)
}

Fine on stable/beta, fails on nightly with:

error[E0277]: the trait bound `(): std::error::Error` is not satisfied
 --> src/main.rs:2:5
  |
2 |     Box::new(e)
  |     ^^^^^^^^^^^ the trait `std::error::Error` is not implemented for `()`
  |
  = note: required for the cast to the object type `dyn std::error::Error`

[SimonSapin]

So ideally we’d like the type variable _ to unify with ! in cases like this:

struct E;
impl From<!> for E {
    fn from(x: !) -> E { x }
}
fn foo(never: !) {
    <E as From<_>>::from(never);
}

And:

fn foo(e: !) -> Box<dyn std::error::Error> {
    Box::<_>::new(e)
}

But not in the case of older https://crates.io/crates/objc versions where that causes UB:

fn unconstrained_return<T>() -> Result<T, String> {
    let ffi: fn() -> T = transmute(some_pointer);
    Ok(ffi())
}
fn foo() {
    match unconstrained_return::<_>() {
        Ok(x) => x,  // `x` has type `_`, which is unconstrained
        Err(s) => panic!(s),  // … except for unifying with the type of `panic!()`
        // so that both `match` arms have the same type.
        // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value.
    };
}

But in a world where ! is a proper type, is there any fundamental difference between these examples? Or are they all cases of #65992?

I’m afraid that the answer is no, there’s no real difference. Which would mean that we can’t have all three of:

• Make Infallible an alias for !
• Not yet make the inference fallback change Tracking issue for fallback of diverging expressions to the never_type (!) #65992 (because objc mitigations are not ready yet)
• Not have the regressions in this issue.

… at the same time. And as we know, if Infallible is to ever be an alias for ! we need to do it in the same release cycle as stabilizing the never type.

[pnkfelix]

triage: P-high. Leaving nomination tag on this, since it seems like we'll need to discuss what action to take on it either this week or next week.

[pnkfelix]

also, tagging this as T-lang: my interpretation of the analysis above is that where it says "... [we cannot have all three of ] 1. Make Infallible an alias for ! ...", Simon mean more generally the idea that empty enums are all aliases for !. And deciding whether or not to keep that as a feature of never_type seems like a definite T-lang issue.

[SimonSapin]

No, I changing pub type Infallible = !; to pub enum Infallible {} in the standard library. This change landed in the same PR as stabilization of the never type:

089229a#diff-da707dc4d90c85e7074dbdecebfe8785R643

I don’t think we should make all empty enums aliases with each other. Rust chose nominal typing over structural typing a long time ago. The name std::convert::Infallible only exists so we could stabilize TryFrom before the never type.

[est31]

Why should Infallible be ever made an alias for !? Why not just deprecate it without doing that? Everyone can switch to ! instead.

[SimonSapin]

@est31 impl<T, U> TryFrom<U> for T where U: Into<T> and impl FromStr for String use Infallible for their associated error type and are #[stable].

[faern]

That's what happens when things are rushed before they are done. We'll either have to 1) wait with things like TryFrom until they are ready. Or 2) be ready to make somewhat breaking changes later, or 3) settle for a forever suboptimal language. I'm in the break everything for the sake of a bright future camp.

[SimonSapin]

“Rushed” is an interesting choice of word for a feature that was stabilized almost three years after it was formally proposed.

[faern]

If it had already waited three years, a few more releases would not have made a large difference, relatively.

[pnkfelix]

also, tagging this as T-lang: my interpretation of the analysis above is that where it says "... [we cannot have all three of ] 1. Make Infallible an alias for ! ...", Simon mean more generally the idea that empty enums are all aliases for !. And deciding whether or not to keep that as a feature of never_type seems like a definite T-lang issue.

Oh, whoops, I totally misinterpreted the situation about what is going on with Infallible then. Sorry!

So ... its possible this is not a T-lang issue then? Is the T-libs team considering "just" reverting the definition of Infallible to be an empty enum again?

[SimonSapin]

T-libs has not collectively discussed this issue yet. Every comment on mine in this thread so far (including this one) is from me individually.

I think we (the Rust project) have a number of possible ways to proceed here, and some of them do involve T-lang. As far as I can tell this is all options including bad ones:

1. We do nothing and ship 1.41.0 with the regressions in this issue. This is likely not acceptable under our stability promise.

2. We complete the mitigations for UB in old objc versions and land the inference fallback change Tracking issue for fallback of diverging expressions to the never_type (!) #65992, all in time for Rust 1.41.0. This fixes the regressions here.

3. We land the fallback change in 1.41 (fixing the regressions) without blocking on mitigations being complete, and either try to add mitigations later or give up on them.

4. We find some other way to fix the regressions while keeping type Infallible = !; and without introducing UB in old objc versions. I don’t know if this is possible. Regression in Error conversion from Infallible #66757 (comment) is saying I’m afraid it’s not. I’d appreciate input from T-lang or T-compiler on this. In particular: what does “fallback” mean exactly? Is it possible and would it help to tweak that definition so that it happens in fewer cases?

5. We revert Infallible to an empty enum and give up on ever making it an alias of !. This would be unfortunate. Infallible was introduced in the first place on the basis that we could avoid this scenario. This fixes the regressions literally as reported here, but new code would still be affected if it uses similar patterns with ! instead of Infallible. IMO it seems too easy to write reasonable-looking code that would be affected for ! to be considered truly first-class in the language.

6. We revert Infallible to an empty enum in 1.41 and plan to make it an alias of ! in a later release. T-libs discussed this before and considers it not acceptable under the stability promise since it would break stable code that separately implements a given trait for both ! and Infallible.

7. We revert Infallible to an empty enum and the stabilization of the never type, effectively going back to the state of 1.40 as far as this issue is concerned, to give ourselves more time to complete one of the other options (ideally 2 or 4).

That’s it. I can’t think of anything else.

[nikomatsakis]

A few quick thoughts:

@SimonSapin asked in this comment whether there was a difference between the fallback used in a case like <T as From<_>>::from(never) (where never: !) and the fallback from the objc example. It seems to me that there is -- the fallback in the former case arises from a reference to a value of type !, whereas the fallback in the latter case arises from the result of an "abrupt" expression (e.g., return etc).

I've not gone digging into the code but I believe that at first glance it should be possible to separate the two, so that let x: _ = never winds up with x: ! but let x: _ = return; winds up with x: (). In fact, it seems quite surprising to have let x: _ = never wind up with x: ().

This would basically mean that the type of an abrupt expression like return is not ! but rather a fresh type variable that falls back to ().

[dtolnay]

From #66757 (comment) I agree that 1 (shipping as is) is not acceptable. With holidays and ~2 weeks remaining in 1.41 we likely wouldn't have time for 2 (mitigations for the objc style inference change).

If Niko's suggestion of differentiating between references to an existing value of type ! vs results of "abrupt" expressions would be possible to implement, that seems way better than option 5 (giving up on ever making Infallible = !). So I would recommend option 7 -- revert back to the state as of 1.40 except now knowing what still needs to be fixed, implementing Niko's approach as soon as we get to it, and then moving forward with stabilization of ! and Infallible=! in the same future release.

[SimonSapin]

@dtolnay That plan sounds good to me.

@nikomatsakis

In fact, it seems quite surprising to have let x: _ = never wind up with x: ().

I agree! Though right now that seems to cause an inference error:

#[allow(unreachable_code)]
pub fn foo(never: !) {
    let x: _ = never;
    // Access non-existent field to make the compiler print
    // an error message with the type of `x`
    x.bar;
}

(Playground) Errors:

   Compiling playground v0.0.1 (/playground)
error[E0282]: type annotations needed
 --> src/lib.rs:6:5
  |
3 |     let x: _ = never;
  |         - consider giving `x` a type
...
6 |     x.bar;
  |     ^ cannot infer type
  |
  = note: type must be known at this point

[nikomatsakis]

I agree with @dtolnay as well -- I think a revert is probably the most prudent step right now.

[nikomatsakis]

@SimonSapin in your example, the problem has to do with when fallback occurs. When you see x.bar, fallback has not yet occurred, and so the type is not (yet) known. Field access (and the . operator in general) requires that the type be known, so you get an error.

However, code like this does compile, and it shows that x is getting a type of either u32 or () -- we know it's not u32, so it must be (). Interestingly, if you enable fallback-to-!, the code no longer builds. (playground):

#![allow(unreachable_code)]

// Uncomment and you will get an error:
// #![feature(never_type_fallback)]

fn foo(never: !) {
    let x: _ = never;
    bar(x);
}

fn bar<T: Bar>(t: T) { }
trait Bar { }
impl Bar for u32 { }
impl Bar for () { }

fn main() { }

Trait resolution can wait until the end, after fallback has occurred.

[nikomatsakis]

We discussed in today's lang-team meeting and decided that the most prudent step is to revert. @Centril asked if someone else could prepare the PR -- any volunteers?

We would like after that to pursue the mitigations a bit more actively, since it seems better to have a uniform fallback to ! than to introduce the "split fallback" that I proposed in my previous comments. @llogiq you had previously expressed an interest, not sure if others are interested, but we may want to up the priority on that work (I can try to dedicate a bit more time to getting it started, too).

[llogiq]

@nikomatsakis Sorry, I was swamped with other work. Also some pointers to the infer functions I need to use – how do I find out if a type comes from fallback? Or how do I set the fallback? – would be really helpful.

[nikomatsakis]

@llogiq Not a problem at all! Happy to discuss further the fallback question on Zulip.

[pnkfelix]

visiting for T-compiler triage. I think we can remove the nomination label; the immediate fire is being put out via PR #67224