Carrier trait for testing, DON'T LAND

[nrc]

No description provided.

[rust-highfive]

r? @brson

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

[nrc]

This is a version of the Carrier trait PR with the try! implementation changed to use ? in order to investigate backwards compatibility. There was only one regression in the repo, fixing that is part of the second commit

cc @rust-lang/lang

Can someone do a Crater run please?

[brson]

I've started a crater run.

[durka]

this line got teleported

[brson]

crater results: https://gist.github.com/a2d94906b7f4ac7421e447dffd2fef3d

[nrc]

• There are 31 root regressions
• There are 168 regressions

A random sampling of the root regressions shows they look genuine and all the ones I looked at were the type inference problem

(Thanks @brson!)

[Stebalien]

These into calls don't actually do anything...

[nrc]

@brson (or anyone else with access) could you kick off a crater run please?

[nrc]

@rust-lang/lang as discussed in the meeting the other week, this is a version of the Carrier trait with a dummy impl, to emulate type checking behaviour around multiple impls. Local testing shows that we get the same type errors as with the Option impl, so I think this is a reasonable candidate for stabilising ? with.

[nikomatsakis]

@nrc so does the "DON'T LAND" in the PR header still apply? :)

[nikomatsakis]

r? @nikomatsakis

[nrc]

@nikomatsakis yes, until we get the crater run and make a decision, I guess

[nikomatsakis]

@nrc ok -- I started a crater run just now (sorry, I had some technical difficulties)

[nikomatsakis]

...and something went wrong but I can't figure out what. I accidentally lost the inspector links too, so it's hard to tell what happened, maybe one of the rustc versions failed to build? I guess I can start it again. (Argh.)

[nikomatsakis]

Starting again didn't help. I'm feeling kind of frustrated here. Not sure what's going wrong.

[brson]

I'll look into it.

[brson]

Trying my own crater run.

[brson]

Crater results https://gist.github.com/ca1c789d3c8962d8797eb38bc6dc800a

[Stebalien]

It doesn't like my code 😿^†. In other news, this appears to interact poorly with try!(iterator_of_results.collect()).

_{^†Many (most?) of those regressions are caused by term. I feel so special. Ook.}

[nrc]

So, the two Crater runs are https://gist.github.com/a2d94906b7f4ac7421e447dffd2fef3d and https://gist.github.com/ca1c789d3c8962d8797eb38bc6dc800a. They are not identical, but they are pretty close, given there was 18 days between them.

So, @rust-lang/lang it looks like we can land the Carrier trait here (without the change to try! to use ?) and then we should be able to stabilise ?. Any thoughts?

[Stebalien]

Not being able to write my_iter_of_results.collect()? is a real bummer.

[nrc]

@Stebalien you can write it with a turbofish, or you can continue to use try! without. I don't think there is a solution which wins both ways, there's just too many variables for type inference to give an answer.

[nrc]

@Stebalien to be clear, this PR breaks try!, but the one we land will not.

[eddyb]

@nrc I'm not sure I understand the design of this Carrier trait - it seems to allow, e.g. going between Result<T, ()> and Option<T>, but doesn't interact with From (like try! does) at all?

If the former part is intended, I can see how collecting to Result<Vec<_>, _> would be rightfully impacted - one could just as well collect to Option<_>.

[Stebalien]

you can write it with a turbofish

I know but that turbofish is nasty:

// Two types
let col: Collection<_> = my_iter_of_results.collect::<Result<_, _>>()?;
// or lots of angle brackets smashed together.
let col = my_iter_of_results.collect::<Result<Collection<_>, _>>()?;

// versus
let col: Collection<_> = my_iter_of_results.collect()?;

or you can continue to use try!

try! will eventually fall out of favor and clippy will likely gain a warning lint against it so, IMO, continuing to use the try! macro isn't an option.

I don't think there is a solution which wins both ways, there's just too many variables for type inference to give an answer.

If one doesn't allow converting between carriers (only allowing conversions with a carrier "class") that may not be the case. However, I can't think of a way to do this that allows converting errors and doesn't require some form of HKT.

---

So, yeah. I don't know if there's anything that can be done here...

[nikomatsakis]

to be clear, this change (updating try!) was something we wanted to do just to evaluate whether we could stabilize without the carrier trait. Seems clear we cannot -- and so should revert this change, right?

[nrc]

Yes, that is what I meant in my last comment

[nikomatsakis]

OK, so I want to lay out my thinking on this change.

First off, I thought the idea of introducing this Carrier trait with dummy type was to safeguard -- in particular, we wanted to stabilize ? without having to stabilize its interaction with type inference. This combination of trait plus dummy type seemed like it would be safely conservative.

The idea was (I think) that we would then write-up an RFC discussing Carrier and try to modify the design to match, stabilizing only when we were happy with the overall shape (or possibly removing Carrier altogether, if we can't reach a design we like).

Now, speaking a bit more speculatively, I anticipate that, if we do adopt a Carrier trait, we would want to disallow interconversion between carriers (whereas this trait is basically a way to convert to/from Result). So intuitively if you apply ? to an Option, that's ok if the fn returns Option; and if you apply ? to a Result<T,E>, that's ok if the fn returns Result<U,F> where E: Into<F>; but if you apply ? to an Option and the fn returns Result<..>, that's not ok.

That said, this sort of rule is hard to express in today's type system. The most obvious starting point would be something like HKT (which of course we don't really have, but let's ignore that for now). However, that's not obviously perfect. If we were to use it, one would assume that the Self parameter for Carrier has kind type -> type -> type, since Result can implement Carrier. That would allow us to express things like Self<T,E> -> Self<U,F>. However, it would not necessarily apply to Option, which has kind type -> type (all of this would of course depend on precisely what sort of HKT system we adopted, but I don't think we'll go all the way to "general type lambdas"). Even more extreme might be a type like bool (although I don't want to implement Carrier for bool, I would expect some people might want to implement Carrier for a newtype'd bool).

What I had considered is that the typing rules for ? could themselves be special: for example, we might say that ? can only be applied to a nominal type Foo<..> of some kind, and that it will match the Carrier trait against this type, but it will require that the return type of the enclosing fn is also Foo<..>. So we would basically instantiate Foo with fresh type parameters. The downside of this idea is that if neither the type that ? is being applied to nor the type of the enclosing fn is known, we can't enforce this constraint without adding some new kind of trait obligation. It's also rather ad-hoc. :) But it would work.

Another thought I had is that we might reconsider the Carrier trait. The idea would be to have Expr: Carrier<Return> where Expr is the type ? is applied to and Return is the type of the environment. For example, perhaps it might look like this:

trait Carrier<Target> {
    type Ok;
    fn is_ok(&self) -> bool; // if true, represents the "ok-like" variant
    fn unwrap_into_ok(self) -> Self::Ok; // may panic if not ok
    fn unwrap_into_error(self) -> Target; // may panic if not error
}

Then expr? desugars to:

let val = expr;
if Carrier::is_ok(&val) {
    val.unwrap_into_ok()
} else {
    return val.unwrap_into_error();
}

The key difference here is that Target would not be the error type, but a new Result type. So for example we might add the following impl:

impl<T,U,E,F> Carrier<Result<U,F>> for Result<T,E>
    where E: Into<F>
{
    type Ok = T;
    fn is_ok(&self) -> bool { self.is_ok() }
    fn unwrap_into_ok(self) -> Self::Ok { self.unwrap() }
    fn unwrap_into_error(self) -> { Err(F::from(self.unwrap_err())) }
}

And then we might add:

impl<T> Carrier<Option<T>> for Option<T> {
    type Ok = T;
    fn is_ok(&self) -> bool { self.is_some() }
    fn unwrap_into_ok(self) -> Self::Ok { self.unwrap() }
    fn unwrap_into_error(self) -> { debug_assert!(self.is_none()); None }
}

And finally we could implement for bool like so:

struct MyBool(bool);
impl<T> Carrier<MyBool> for MyBool {
    type Ok = ();
    fn is_ok(&self) -> bool { self.0 }
    fn unwrap_into_ok(self) -> Self::Ok { debug_assert!(self.0); () }
    fn unwrap_into_error(self) -> { debug_assert!(!self.0); self }
}

Now this version is more flexible. For example, we could allow interconversion between Option values to be converted to Result by adding an impl like:

impl<T> Carrier<Result<T,()>> for Option<T> { ... }

But of course we don't have to (and we wouldn't).

[nrc]

@eddyb this does use From, but the calls to from are made in the HIR lowering, rather than in the Carrier trait.

[eddyb]

@nrc That's where the confusion came from, I guess.

[nrc]

Closing this PR, I'll open a new one with the commits we actually want to land

[nrc]

I opened rust-lang/rfcs#1718 to discuss the Carrier trait.