rust icon indicating copy to clipboard operation
rust copied to clipboard

Tracking Issue for `try_trait_v2`, A new design for the `?` desugaring (RFC#3058)

Open scottmcm opened this issue 4 years ago • 142 comments

This is a tracking issue for the RFC "try_trait_v2: A new design for the ? desugaring" (rust-lang/rfcs#3058). The feature gate for the issue is #![feature(try_trait_v2)].

This obviates https://github.com/rust-lang/rfcs/pull/1859, tracked in https://github.com/rust-lang/rust/issues/42327.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

  • [ ] Implement the RFC
    • [x] Add the new traits and impls
    • [x] Update the desugar in ast lowering
    • [x] Fixup all the tests
    • [ ] Add nice error messages in inference
    • [ ] Improve perf with enough MIR optimizations
    • [x] ~~Delete the old way after a bootstrap update~~ https://github.com/rust-lang/rust/pull/88223
  • [x] Not strictly needed, but a mir-opt to simplify the matches would really help: https://github.com/rust-lang/rust/issues/85133
  • [ ] Add more detailed documentation about how to implement and use the traits
  • [ ] Decide whether to block return types that are FromResidual but not Try
  • [x] Fix rustdoc to show the default type parameter on FromResidual better (Issue https://github.com/rust-lang/rust/issues/85454)
  • [ ] Before stabilizing, ensure that all uses of Infallible are either fine that way or have been replaced by !
  • [ ] Stabilizing this will allow people to implement Iterator::try_fold
    • [ ] As part of stabilizing, document implementing try_fold for iterators (perhaps reopen https://github.com/rust-lang/rust/pull/62606)
    • [ ] Ensure that the default implementations of other things have the desired long-term DAG, since changing them is essentially impossible later. (Specifically, it would be nice to have fold be implemented in terms of try_fold, so that both don't need to be overridden.)
  • [ ] Adjust documentation (see instructions on rustc-dev-guide)
  • [ ] Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

From RFC:

  • [ ] What vocabulary should Try use in the associated types/traits? Output+residual, continue+break, or something else entirely?
  • [ ] Is it ok for the two traits to be tied together closely, as outlined here, or should they be split up further to allow types that can be only-created or only-destructured?

From experience in nightly:

  • [ ] Should there be a trait requirement on residuals of any kind? It's currently possible to accidentally be FromResidual from a type that's never actually produced as a residual (https://github.com/SergioBenitez/Rocket/pull/1645). But that would add more friction for cases not using the Foo<!> pattern, so may not be worth it.
    • Given the trait in #91286, that might look like changing the associated type Residual; to type Residual: Residual<Self::Output>;.

Implementation history

  • [x] Basic traits and impls added, https://github.com/rust-lang/rust/pull/84092
  • [x] Removing try_trait from stdarch, https://github.com/rust-lang/stdarch/pull/1142
  • [x] Implementing the desugaring, https://github.com/rust-lang/rust/pull/84767

scottmcm avatar Apr 17 '21 18:04 scottmcm

We have a problem in our project related to the new question mark desugaring. We use the track_caller feature in From::from implementation of the error types to collect stack traces with generics and auto and negative impl traits magic implemented by @sergeyboyko0791 (https://github.com/KomodoPlatform/atomicDEX-API/blob/mm2.1/mm2src/common/mm_error/mm_error.rs).

After updating to the latest nightly toolchain this stack trace collection started to work differently. I've created a small project for the demo: https://github.com/artemii235/questionmark_track_caller_try_trait_v2

cargo +nightly-2021-05-17 run outputs Location { file: "src/main.rs", line: 18, col: 23 } as we expect.

cargo +nightly-2021-07-18 run outputs Location { file: "/rustc/c7331d65bdbab1187f5a9b8f5b918248678ebdb9/library/core/src/result.rs", line: 1897, col: 27 } - the from_residual implementation that is now used for ? desugaring.

Is there a way to make the track caller work the same way as it was before? Maybe we can use some workaround in our code?

Thanks in advance for any help!

artemii235 avatar Jul 22 '21 11:07 artemii235

That's interesting -- maybe Result::from_residual could also have #[track_caller]? But that may bloat a lot of callers in cases that won't ever use the data.

cuviper avatar Jul 23 '21 00:07 cuviper

From the description:

A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.

@artemii235 Do you mind opening a separate issue?

thomaseizinger avatar Jul 23 '21 08:07 thomaseizinger

Do you mind opening a separate issue?

No objections at all :slightly_smiling_face: I've just created it https://github.com/rust-lang/rust/issues/87401.

artemii235 avatar Jul 23 '21 11:07 artemii235

May i suggest changing branch method's name to something else? When searching for methods, it's a little not obvious to see Option::branch or Result::branch is not the method one should usually call...

crlf0710 avatar Jul 28 '21 04:07 crlf0710

How do I use ? with Option -> Result now? Before it was only necessary to implement From<NoneError> for my error type.

huntiep avatar Aug 03 '21 09:08 huntiep

Use .ok_or(MyError)?

tmccombs avatar Aug 03 '21 19:08 tmccombs

Why the implementation of FromResidual for Result uses trait From in stead of Into. According to the documentation of trait Into and From, we should

Prefer using Into over From when specifying trait bounds on a generic function to ensure that types that only implement Into can be used as well.

Clarification is welcome as an error type implementing only Into trait arises with associated error type on traits and associated types cannot bind on From for lack of GATs.

RagibHasin avatar Aug 23 '21 16:08 RagibHasin

@RagibHasin

see https://github.com/rust-lang/rust/issues/31436#issuecomment-299482914 and https://github.com/rust-lang/rust/issues/31436#issuecomment-619427209 (and the following discusion, respectively)

steffahn avatar Aug 23 '21 17:08 steffahn

Hi, I'm keen to see this stabilized. Is there any work that can be contributed to push this forward? It would be my first Rust contribution, but I have a little experience working on compiler code (little bit of LLVM and KLEE in college).

BGR360 avatar Aug 23 '21 22:08 BGR360

@BGR360 Unfortunately the main blockers here are unknowns, not concrete-work-needing-to-be-done, so it's difficult to push forward. It's hard to ever confirm for sure that people don't need the trait split into parts, for example.

Have you perhaps been trying it out on nightly? It's be great to get experience reports -- good or bad -- about how things went. (For example, https://github.com/rust-lang/rust/issues/42327#issuecomment-366840247 was a big help in moving to this design from the previous one.) If it was good, how did you use it? If it was bad, what went wrong? In either case, was there anything it kept you from doing which you would have liked to, even if you didn't need it?

scottmcm avatar Aug 24 '21 21:08 scottmcm

Experience Report

@scottmcm I have tried #[feature(try_trait_v2)] in its current form. I'll give an experience report:

Overall my experience with this feature is positive. It may end up being critical for my professional work in Rust.

My use case is very similar to @artemii235: https://github.com/rust-lang/rust/issues/84277#issuecomment-884838560. At my work, we need a way to capture the sequence of code locations that an error propagates through after it is created. We aren't able to simply use std::backtrace to capture the backtrace at the time of creation, because errors can propagate between multiple threads as they bubble up to their final consumer. The way we do this in our C code is to manually wrap every returned error value in a special forward_error macro which appends the current __file__, __line__, and __func__ to the error's backtrace.

We would love to be able to do this in our Rust code using just the ? operator, no macros or boilerplate required. So I experimented with implementing my own replacement for std::result::Result (call it MyResult). I implemented std::ops::Try on MyResult in a very similar manner to std::result::Result, but I annotated FromResidual::from_residual with #[track_caller] so that I could append the location of the ? invocation to the error's backtrace. The experiment was successful and relatively straightforward.

To get this to work, I made express use of the fact that you can implement multiple different FromResidual on a type (I think that might be what you're referring to when you say "splitting the trait into parts"?). I have one FromResidual to coerce from std::result::Result to my MyResult, and another one to coerce from MyResult to MyResult.

I'd be happy to give more specifics on how I achieved my use case either here or on Zulip, just let me know :)

Pros:

  • Allows me to implement multiple FromResidual for my Try type. This was critical for my use case.

Cons:

  • Documentation is a little weak, but I was able to learn by example by reading the source code for std::result::Result.
  • It'd be great to be able to achieve my use case without having to rewrite Result. See my other comment below.

BGR360 avatar Aug 25 '21 19:08 BGR360

Experience report

I was using try_trait on an app of mine and upgraded to try_trait_v2 because the build started failing on the latest nightly. My use case was a bit weird as I am using the same type of Ok and Err variants as it is a generic Value type for a programming language. However the try operator is still incredibly helpful in the implementation.

Pros:

  • The conversion was localized.

Cons:

  • More code to get it to work.
  • Many more new concepts than try_trait. For example I now need to use:
    • ControlFlow which is fairly straight forward (although I don't know why the arguments are backwards compared to Result.
    • Residual which I still barely understand and the name is incredibly perplexing. "Residue" is something left over but it isn't clear what is being left over in this case.
  • The docs are not very helpful. I had to guess the impl<E: Into<Val>> std::ops::FromResidual<Result<std::convert::Infallible, E>> for Val incantation from the error messages and it still isn't completely clear to me how this type comes to be.

Overall this v2 is a clear downgrade for this particular use case however the end result isn't too bad. If this is making other use cases possible it is likely worth it with better names and docs.

The full change: https://gitlab.com/kevincox/ecl/-/commit/a1f348633afd2c8dd269f95820f95f008b461c9e

kevincox avatar Aug 27 '21 14:08 kevincox

So I experimented with implementing my own replacement for std::result::Result (call it MyResult).

This is actually a little bit unfortunate, in retrospect. It would be much better if I could just make use of std::result::Result as it already exists. That would require two things that are missing:

  • <std::result::Result as FromResidual>::from_residual would need to have #[track_caller]
  • I would need to be able to intercept invocations of From<T>::from() -> T so I can push to the stack even when the ? operator does not coerce the result to a different error type.

To illustrate, here's how things work in my experiment:

pub struct ErrorStack<E> {
    stack: ..,
    inner: E,
}

impl<E> ErrorStack<E> {
    /// Construst new ErrorStack with the caller location on top.
    #[track_caller]
    fn new(e: E) -> Self { ... }

    /// Push location of caller to self.stack
    #[track_caller]
    fn push_caller(&mut self) { ... }

    /// Return a new ErrorStack with the wrapped error converted to F
    fn convert_inner<F: From<E>>(f: F) -> ErrorStack<F> { ... }
}

pub enum MyResult<T, E> {
    Ok(T),
    Err(ErrorStack<E>),
}

pub use MyResult::Ok;
pub use MyResult::Err;

impl<T, E> Try for MyResult<T, E> {
    type Output = T;
    type Residual = MyResult<Infallible, E>;

    /* equivalent to std::result::Result's Try impl */
}

/// Pushes an entry to the stack when one [`MyResult`] is coerced to another using the `?` operator.
impl<T, E, F: From<E>> FromResidual<MyResult<Infallible, E>> for MyResult<T, F> {
    #[inline]
    #[track_caller]
    fn from_residual(residual: MyResult<Infallible, E>) -> Self {
        match residual {
            // seems like this match arm shouldn't be needed, but idk the compiler complained
            Ok(_) => unreachable!(),
            Err(mut e) => {
                e.push_caller();
                Err(e.convert_inner())
            }
        }
    }
}

/// Starts a new stack when a [`std::result::Result`] is coerced to a [`Result`] using `?`.
impl<T, E> FromResidual<std::result::Result<Infallible, E>> for Result<T, E> {
    #[inline]
    #[track_caller]
    fn from_residual(residual: std::result::Result<Infallible, E>) -> Self {
        match residual {
            // seems like this match arm shouldn't be needed, but idk the compiler complained
            std::result::Result::Ok(_) => unreachable!(),
            std::result::Result::Err(e) => Err(StackError::new(e)),
        }
    }
}

If std::result::Result had #[track_caller] on its FromResidual::from_residual, then I could avoid everything above by just pushing to the stack inside an impl From:

impl<E, F: From<E>> From<ErrorStack<E>> for ErrorStack<F> {
    #[track_caller]
    fn from(mut e: ErrorStack<E>) -> Self {
        e.push_caller();
        e.convert_inner()
    }
}

However, this does not work because it conflicts with the blanket From<T> for T implementation.

I could limit my From to types E, F such that E != F, but I need functions to show up in my error trace even if the residual from ? does not change types. For example:

fn foo() -> MyResult<(), io::Error> {
    fs::File::open("foo.txt")?;
}

fn bar() -> MyResult<(), io::Error> {
    // I need bar to show up in error traces, so I wrap with Ok(..?).
    // Without my custom MyResult, I am unable to intercept this invocation of the `?` operator, because
    // the return type is the same as that of `foo`.
    Ok(foo()?)
}

BGR360 avatar Aug 27 '21 21:08 BGR360

Why the Option<Infallible> is the Option's Residual type? Why not the option itself: Option<T> ?

This would let me do:


impl FromResidual<Option<Viewport>> for MyResult {
    fn from_residual(_: Option<Viewport>) -> Self {
        Self(Err(SomeError::ViewportNotFound))
    }
}
impl FromResidual<Option<Item>> for MyResult {
    fn from_residual(_: Option<Item>) -> Self {
        Self(Err(SomeError::ItemNotFound))
    }
}

This ok_or(Error)? is bugging me, and I really want a solution that converts Option<T> to MyResult. If I would convert Option<Infallible> to a MyNoneError it wouldn't help me at all. Even .expect() would add more info about the place.

For workaround, I created a module that contains the error handling like:

pub fn viewport<'p>(
    viewports: &'p HashMap<ViewportId, Viewport>,
    viewport_id: &ViewportId,
) -> Result<&'p Viewport, BindingError> {
    viewports
        .get(viewport_id)
        .ok_or(BindingError::ViewportDoesNotExist)
}
pub fn viewport_mut<'p, 'msg>(
    viewports: &'p mut HashMap<ViewportId, Viewport>,
    viewport_id: &ViewportId,
) -> Result<&'p mut Viewport, BindingError> {
    viewports
        .get_mut(viewport_id)
        .ok_or(BindingError::ViewportDoesNotExist)
}
...

It looks really bad. But the usage is only 1 line compared to 3.

So will this be improved?

fxdave avatar Sep 01 '21 22:09 fxdave

Why the Option<Infallible> is the Option's Residual type?

Because from_residual should always be called with None. Option<!> would also work, if ! is stabilized before the Try trait. ! or Infallible communicates that through the type system.

If/once RFC-1210 is stabilized, then I think the Option's Try trait could be implemented like:

impl<T> ops::Try for Option<T> {
    type Output = T;
    default type Residual = Option<convert::Infallible>;

    #[inline]
    fn from_output(output: Self::Output) -> Self {
        Some(output)
    }

    #[inline]
    default fn branch(self) -> ControlFlow<Self::Residual, Self::Output> {
        match self {
            Some(v) => ControlFlow::Continue(v),
            None => ControlFlow::Break(None),
        }
    }
}

And you could change the Residual type for your specific Option type.

tmccombs avatar Sep 02 '21 00:09 tmccombs

Meanwhile, I found an even better workaround. I put it here. This may be useful for somebody else too:

  1. DefineSomeError and MyResult. MyResult is needed because I'm not allowed to impl the std's Result, so I applied the new type pattern.
  2. Implement From<FromResidual<PhantomData<T>>> for MyResult (where T is from the Option<T> that you want to use.)
#[derive(Debug)]
pub enum SomeError {
    NoStringError,
    NoIntError,
}
struct MyResult(Result<i32, SomeError>);

// allow convert Option<T> i
impl FromResidual<PhantomData<String>> for MyResult {
    fn from_residual(_: PhantomData<String>) -> Self {
        Self(Err(SomeError::NoStringError))
    }
}
impl FromResidual<PhantomData<i32>> for MyResult {
    fn from_residual(_: PhantomData<i32>) -> Self {
        Self(Err(SomeError::NoIntError))
    }
}
  1. Define MyOption (An Option<T> wrapper for the same reasons as for MyResult).
  2. impl From<Option<T>> to let rust convert any option to this MyOption.
  3. Implement FromResidual and Try for this MyOption
struct MyOption<T>(Result<T, PhantomData<T>>);

// let any Option<T> be MyOption<T>
impl<T> From<Option<T>> for MyOption<T> {
    fn from(option: Option<T>) -> Self {
        match option {
            Some(val) => Self(Ok(val)),
            None => Self(Err(PhantomData)),
        }
    }
}

// Allow '?' operator for MyOption
impl<T> FromResidual<PhantomData<T>> for MyOption<T> {
    fn from_residual(o: PhantomData<T>) -> Self {
        Self(Err(o))
    }
}
impl<T> Try for MyOption<T> {
    type Output = T;
    type Residual = PhantomData<T>;
    fn from_output(output: Self::Output) -> Self {
        MyOption(Ok(output))
    }
    fn branch(self) -> ControlFlow<Self::Residual, Self::Output> {
        match self.0 {
            Ok(val) => ControlFlow::Continue(val),
            Err(err) => ControlFlow::Break(err),
        }
    }
}
  1. define a macro for convenience.
macro_rules! e {
    ($($token:tt)+) => {
        MyOption::<_>::from($($token)+)?
    };
}

And now I can use this e! macro for any Option<T> in a method that returns MyResult.


fn get_some_string() -> Option<String> { Some(String::from("foo")) }
fn get_some_int() -> Option<i32> { Some(42) }

fn foo() -> MyResult {
    let some_string = e!(get_some_string());
    let some_int = e!(get_some_int());

    MyResult(Ok(42))
}

fxdave avatar Sep 02 '21 00:09 fxdave

@fxdave IMO not a great idea to ditch the whole Result API -- try this or newtype that hashmap or something. But you're onto something with this and eventually when you turned it into PhantomData<T>:

Why the Option<Infallible> is the Option's Residual type? Why not the option itself: Option<T>?

The problem: you want to convert Option::<T>::None to Err for specific error types using only ?. I think this is pretty common. To be clear I think it has somewhat limited use, I wouldn't implement it unless I were sure absence was always an error for that type, lest a single character ? be the cause of mistakes. Constraining the implicit conversion to specific error types you only use when this statement holds is a good idea, like InViewportContextError.

The current Option::Residual is indeed annoying in that it erases the type it could have held, so that information can't be used for any conversions like the one you want. As I understand it the whole point of FromResidual is that it's where you glue together your own Try types with other people's.

Re @tmccombs' solution, I don't think making people implement a specialised Try for Option with a custom Residual is ideal. The specialised Try isn't even enough -- you'd need to implement FromResidual<MyResidual> on both Option<T> and Result<T, MyError> generically as well. Can those even be done outside std? I don't think it can for Option<T>. Maybe you'd just have Residual = Result<!, MyError>. I don't know. But it sounds way too much effort and a steep learning curve for a common thing.

Given this is kinda common, why not bring back good old NoneError? But this time, carry information about what type produced it. And given the v2 RFC is all about removing references to "errors", give it a new name accordingly.

// std
struct Absent<T>(PhantomData<T>);
impl Try for Option<T> {
    type Residual = Absent<T>;
    type Output = T;
    ...
}
impl<A, T> FromResidual<Absent<A>> for Option<T> { ... }
impl<A, T, E> FromResidual<Absent<A>> for Result<T, E> where E: From<Absent<A>> { ... }

// userland
struct Foo;
enum MyError { MissingFoo, MissingOtherType }
impl From<Absent<Foo>> for MyError { ... }
impl From<Absent<OtherType>> for MyError { ... }

fn get_foo() -> Option<Foo> { ... }
fn bar() -> Result<i32, MyError> {
    let foo = get_foo()?;
    Ok(42)
}

This is basically your workaround but in std where it should be. This isn't possible with Residual = Option<!> because the the T is erased and unavailable in Result::from_residual.

Benefits:

  • No need to wait for impl specialization to land.
  • Exactly the same API as people use now to make error types composable via ?.
  • When you write this in a codebase, it pretty directly communicates the idea that absence of the type is an error. Full-on specialised Try/FromResidual impls don't.

Problems:

  • Absent<&'_ T>and similarly &mut are a bit annoying. You can't add a second impl i.e. impl FromResidual<Absent<&'_ A>> for result wherever E: From<Absent<A>>, because E could implement From<Absent<&'_ A>> as well. So as it stands people would have implement From<Absent<&'a Foo>> on their error types to make .as_ref()? work.

I understand the RFC is also trying to avoid having to create residual types, because implementing Try on large enums was previously really annoying. That doesn't mean std has to use !/Infallible everywhere. There is nothing preventing std from using a neat little residual type to make life with Option and Result easier.

Try it: old, edit: more complete

cormacrelf avatar Sep 02 '21 07:09 cormacrelf

One addition for completeness is that if the Enum variant types RFC ever comes out of postponement hibernation, it might cover some (not all) of these residual types-with-holes-in-them problems. Thinking about this also surfaced a usability problem that might have gone unnoticed due to the rustc features enabled in libcore so far.

  1. It is not specifically contemplated by that RFC, but if you could impl FromResidual<Option<T>::None> for MyTryType (noting that's different from impl Trait for Option<T>::None that it does contemplate forbidding) then that would be a much more easily understood way to define your residual types.

  2. With enum variant types, the Absent<T> idea could be replaced by Option<T>::None. Deprecate Absent<T> (a struct with no public fields) and alias it to the variant, and everything would still work. It would be very easy to do this kind of thing in your own code, too. So if you're worried about usability of the residual pattern for user-defined types, there's at least something on the distant horizon to ameliorate that.

  3. Then consider Result<T, E>::Err. This one is more of a worry. First, note that the try_trait_v2 RFC's example implementation (and the real one in libcore) of FromResidual<Result<!, E>> does not compile outside libcore with its many rustc features activated. On stable Rust, you have to do this: (playground)

let r: Result<core::convert::Infallible, i32> = Err(5);
match r {
    Err(five) => ...,
    // rustc demands an Ok match arm, even though it only contains an Infallible.
    // you must add:
    Ok(never) => match never {},
}

This also happens with #![feature(never_type)] and Result<!, i32>. So as it stands now using Result<Infallible, E>, the main use case for try_trait_v2, namely adding FromResidual implementations for custom types that interoperate with Result APIs, requires this weird workaround for infallible types. It's not as clean as it has been made out.

But also, if you ever simply swapped out Result<!, E> for Result<T, E>::Err, you'd mess up everyone's match residual { Ok(never) => match never {}, ... } arms, since they wouldn't compile with the variant type.

  1. You could do an Absent<T>-style solution for Result, by defining struct ResultErr<T, E>(PhantomData<T>, E) and only providing a single fn into_err(self) -> E method, so that nobody is relying on the infallible match arm behaviour. (No name is going to be as good as Absent 😞). That would also eliminate the usability problem with infallible matches identified above. It would require choosing an API that will eventually be present on Result<T, E>::Err, i.e. match up with https://github.com/rust-lang/rfcs/issues/1723 or something.

In summary, if you stabilise the impl with type Residual = Result<!, E> there's no going back, everyone's going to have to wrap their head around the use of the infallible/never type in there forever. As I said in my last post, while it's nice that the pattern can be used to create ad hoc residual types for a decent class of enums with the current compiler, std doesn't have to use !. I would consider not using the pattern for Result either, rather using a dedicated type as above.

cormacrelf avatar Nov 11 '21 09:11 cormacrelf

Also, if std contained no implementations of Try with a ! in the associated residual, it would become even more difficult to explain why it's called the residue / the residual.

I would suggest naming it Failure. We don't need to describe it in terms of abstract splits between outputs and anti-outputs, the Try trait is named Try and the operator is a question mark. If ? returns from the function, the answer is that we tried but did not succeed. When you ask yourself, "if you try a Result, what constitutes failure?" you must admit the answer is Err(E). You would not additionally rename Output to Success, because "what constitutes success" is Ok(T), not T.

pub trait Try: FromFailure<Self::Failure> {
    type Output;
    /// A type representing a short-circuit produced by this specific `Try` implementation.
    ///
    /// Each `Try` implementation should have its own `Failure` type, so that a custom
    /// conversion can be defined for every combination of the type `?` is used on
    /// (implementing `Try<Failure = F>`), and the return type of the function it is used
    /// within (implementing `FromFailure<F>`).
    ///
    /// (Docs can give an example of using ! if they like)
    type Failure;
    fn from_output(x: Self::Output) -> Self;
    fn branch(self) -> ControlFlow<Self::Failure, Self::Output>;
}

pub trait FromFailure<F = <Self as Try>::Failure> {
    /// Construct Self from a failure type, 
    fn from_failure(failure: F) -> Self;
}

This might have been discussed /dismissed somewhere already, but I don't really see any downsides. You've already got the perfectly abstract ControlFlow in there, no need to pretend that Try isn't about success/failure.

cormacrelf avatar Nov 11 '21 10:11 cormacrelf

@cormacrelf as far as I remember, counter-points were e.g.: https://github.com/rust-lang/rust/issues/42327#issuecomment-318923393 https://github.com/rust-lang/rust/issues/42327#issuecomment-376772143

e.g. in some cases we want to short-circuit on success or short-circuit in both success and error conditions, and the ControlFlow terminology matches this more closely than some Failure/Success distinction; this is also afaik basically the underlying motivation to do this trait-juggling at all, because otherwise we could just continue to use Result and Option, which would suffice in that case, but unfortunately, doesn't adequately cover other cases that should be covered.

Another possibility which might be interesting, would be replacing all of this just with ControlFlow as the primary building block, and defining adequate conversions for Result and Option from/into that. Another alternative might be some kind of PhTaggedControlFlow, e.g.

pub struct PhTaggedControlFlow<Tag, B, C = ()> {
    tag: PhantomData<Tag>,
    inner: ControlFlow<B, C>,
}

with appropriate conversions (including conversion into ControlFlow). This would have the downside that functionality to decide whether to Break or Continue would be more ad-hoc (although it could be wrapped mostly properly). Another disadvantage of that would be that it might be easier to accidentially end up with some ControlFlow->Result conversion which we overall would want to avoid (hence some tagged ControlFlow, to have more control about potential conversions, especially if we also need to deal with Results (and similar types) from other functions, and might want to handle them different for every such case, this would be more some kind of "stop-gap" thing to avoid some unintended "pollution by conversion possiblities", which might make code more unreadable (or nudge the user into sprinkling of .into() or such, which could quickly lead to fragile code, in case any of the possible conversions break)). I don't know exactly how justified that concern might be.

fogti avatar Nov 11 '21 14:11 fogti

What about implement From trait when the two generic of ControlFlow are identical as impl From<ControlFlow<T, T>> for T. Would allow to use into() instead of doing a match "at hand".

Inspired by https://github.com/rust-lang/rust/issues/45222#issuecomment-1002432515

Stargateur avatar Dec 29 '21 07:12 Stargateur

I wonder if it would be feasible to have an optimization where for in loops are replaced with .try_for_each call. Currently it's not possible to implement this method manually, and the standard library implementations are well behaved, so this wouldn't be a breaking change. This couldn't be done after Try trait gets stabilized.

Of course, I guess one issue with that is .await within for in loop.

sugar700 avatar Jan 26 '22 09:01 sugar700

@xfix the compiler could easily scan for awaits in the code, tho, and decide based on that, I don't think it would be a big problem.

fogti avatar Jan 26 '22 11:01 fogti

Await is not the only problem, there is also the problem of named loop labels, which are allowed on for _ in loops. Code inside nested loops can break out of outer ones. You would need to put the information necessary to replicate this in the Try-implementing types used by the generated desugaring. The challenge is to convert break/continue/return statements into return <expr>; in such a way as to gettry_for_each to emulate them and hopefully compile efficiently. For reference, here is the current desugaring of for _ in.

Here's an example desugaring using ControlFlow<ControlFlow<integer, integer>, ()>, where the integers represent loop nesting depth, and e.g. return; from the whole function desugars as return Break(Break(0));: playground, plus a println-less version with a silly benchmark that probably doesn't tell us anything since there's nothing to inline.

cormacrelf avatar Jan 26 '22 15:01 cormacrelf

What is the benefit of "optimising" to a call of try_foreach?

tmccombs avatar Jan 26 '22 15:01 tmccombs

This discussion about for loop desugaring seems very off-topic for this tracking issue. For in-depth discussion on that, please open a topic on https://internals.rust-lang.org, or a new issue on https://github.com/rust-lang/rust.

steffahn avatar Jan 26 '22 15:01 steffahn

@tmccombs It gives you internal iteration for more complicated iterators like Chain, rather than calling the outermost next() each time. But while I think that's a useful transformation under user control, I'm skeptical about having the compiler do it.

cuviper avatar Jan 26 '22 16:01 cuviper

It took me a really long time for me to wrap my head around this. I was really tripped up on the word "residual", and now I still think that word is unhelpful. It all clicked for me when I realized that Output maps to ControlFlow::Continue and Residual maps to ControlFlow::Break. And Try is, in essence, just Into<ControlFlow>. So I think we should capitalize on the cohesion with ControlFlow by just using the same names.

trait Try: FromBreak<Self::Break> {
    type Continue;
    type Break;
    
    fn from_continue(c: Self::Continue) -> Self;
    fn branch(self) -> ControlFlow<Self::Break, Self::Continue>;
}

camsteffen avatar Feb 11 '22 06:02 camsteffen

Commenting on the bullet of "Decide whether to block return types that are FromResidual but not Try", I have a use case in an error handling system for a parser:

impl<V, C: catch::Catchable> std::ops::Try for GuardedResult<V, C, join::Joined> {
    type Output = V;

    type Residual = GuardedResult<V, catch::Uncaught, join::Unjoined>;

    fn from_output(output: Self::Output) -> Self {
        //
    }

    fn branch(self) -> std::ops::ControlFlow<Self::Residual, Self::Output> {
        todo!()
    }
}

impl<V> std::ops::Residual<V> for GuardedResult<V, catch::Uncaught, join::Unjoined> {
    fn from_residual(r: GuardedResult<Infallible, catch::Uncaught, join::Unjoined>) -> GuardedResult<V, catch::Uncaught, join::Unjoined> {
        GuardedResult { _c: PhantomData::default(), _j: PhantomData::default(), ..r }
    }
}

/// the struct in question
pub struct GuardedResult<V, C: catch::Catchable, J: join::Joinable> {
    value: Option<V>,
    root_error: Option<ParseResultError>,
    cascading_errors: ErrorSet,

    solution: SolutionClass,

    /// If this error has been caught and is planned to be locally handled, this is true
    caught: bool,

    _c: PhantomData<C>,
    _j: PhantomData<J>,
}

I want to be able to force the user to "deal with" an error by using functions implemented on GuardedResult<V, C, join::Unjoined> and GuardedResult<V, catch::Uncaught, J> in order for them to be able to create a GuardedResult<V, Caught, Joined>. I only want to allow the ? operation on GuardedResult<V, Caught, Joined>, but as soon as the error has been bubbled have it revert to GuardedResult<V, Uncaught, Unjoined> so that the next function up is forced to also try to deal with it. Unfortunately having Try: FromResidual forces me to have the type returned by the function be both Joined and Caught, which I don't want to add for the reasons above.

szbergeron avatar Feb 18 '22 23:02 szbergeron