search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.08k stars 12.26k forks source link

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

Open scottmcm opened 3 years ago

scottmcm commented 3 years ago

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

Unresolved Questions

From RFC:

From experience in nightly:

Implementation history

artemii235 commented 2 years ago

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!

cuviper commented 2 years ago

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.

thomaseizinger commented 2 years ago

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?

artemii235 commented 2 years ago

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.

crlf0710 commented 2 years ago

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...

huntiep commented 2 years ago

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

tmccombs commented 2 years ago

Use .ok_or(MyError)?

RagibHasin commented 2 years ago

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.

steffahn commented 2 years ago

@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)

BGR360 commented 2 years ago

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).

scottmcm commented 2 years ago

@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?

BGR360 commented 2 years ago

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:

Cons:

kevincox commented 2 years ago

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:

Cons:

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

BGR360 commented 2 years ago

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:

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

```rust pub struct ErrorStack { stack: .., inner: E, } impl ErrorStack { /// 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: F) -> ErrorStack { ... } } pub enum MyResult { Ok(T), Err(ErrorStack), } pub use MyResult::Ok; pub use MyResult::Err; impl Try for MyResult { type Output = T; type Residual = MyResult; /* 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> FromResidual> for MyResult { #[inline] #[track_caller] fn from_residual(residual: MyResult) -> 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 FromResidual> for Result { #[inline] #[track_caller] fn from_residual(residual: std::result::Result) -> 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:

```rust impl> From> for ErrorStack { #[track_caller] fn from(mut e: ErrorStack) -> 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:

```rust 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()?) } ```
fxdave commented 2 years ago

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?

tmccombs commented 2 years ago

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.

fxdave commented 2 years ago

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 i impl FromResidual<PhantomData> for MyResult { fn fromresidual(: PhantomData) -> Self { Self(Err(SomeError::NoStringError)) } } impl FromResidual<PhantomData> for MyResult { fn fromresidual(: PhantomData) -> Self { Self(Err(SomeError::NoIntError)) } }


3. Define `MyOption` (An `Option<T>` wrapper for the same reasons as for `MyResult`).
4. `impl From<Option<T>>` to let rust convert any option to this `MyOption`. 
5. Implement `FromResidual` and `Try` for this `MyOption`

```rust
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 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))
}
cormacrelf commented 2 years ago

@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:

Problems:

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 commented 2 years ago

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 commented 2 years ago

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.

fogti commented 2 years ago

@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.

Stargateur commented 2 years ago

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

KamilaBorowska commented 2 years ago

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.

fogti commented 2 years ago

@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.

cormacrelf commented 2 years ago

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.

tmccombs commented 2 years ago

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

steffahn commented 2 years ago

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.

cuviper commented 2 years ago

@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.

camsteffen commented 2 years ago

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>;
}
szbergeron commented 2 years ago

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.

scottmcm commented 2 years ago

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>

What if you have the function return the latter, and implement FromResidual for it, but not Try?

Then they'll have to do something to the function return value to convert it to the former -- which implements Try too -- before they can ? it.

szbergeron commented 2 years ago

I think that's roughly what I've done below, but IMO it is somewhat more complicated to write than the simple "divergent residual" code above:

#[derive(Default)]
pub struct GuardFlagsImpl<C: catch::Catchable, J: join::Joinable> {
    _c: PhantomData<C>,
    _j: PhantomData<J>,
}

trait GuardFlags: Default {
}

impl From<GuardFlagsImpl<catch::Caught, join::Joined>> for GuardFlagsImpl<catch::Uncaught, join::Unjoined> {
    fn from(v: GuardFlagsImpl<catch::Caught, join::Joined>) -> Self { Default::default() }
}

impl<V, G: GuardFlags> std::ops::Try for GuardedResult<V, G> {
    type Output = V;

    type Residual = GuardedResult<V, G>;

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

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

impl<V, G: GuardFlags, B: From<G> + GuardFlags> std::ops::FromResidual<GuardedResult<V, G>> for GuardedResult<V, B> {
    fn from_residual(r: GuardedResult<V, G>) -> Self {
        GuardedResult {
            gf: From::from(r.gf),
            ..r
        }
    }
}

impl<C: catch::Catchable, J: join::Joinable> GuardFlags for GuardFlagsImpl<C, J> {}

pub struct GuardedResult<V, G: GuardFlags> {
    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,

    gf: G,
}

This imitates the approach I see in Result<V, E> for converting E from one error to another

szbergeron commented 2 years ago

I just reread what you said earlier, and I tried doing that but ran into a roadblock: FromResidual requires Self: Try

#[unstable(feature = "try_trait_v2", issue = "84277")]
pub trait FromResidual<R = <Self as Try>::Residual> {
    /// Constructs the type from a compatible `Residual` type.
    ///
    /// This should be implemented consistently with the `branch` method such
    /// that applying the `?` operator will get back an equivalent residual:
    /// `FromResidual::from_residual(r).branch() --> ControlFlow::Break(r)`.
    /// (It must not be an *identical* residual when interconversion is involved.)
    ///
    /// # Examples
    ///
    /// ```
    /// #![feature(try_trait_v2)]
    /// use std::ops::{ControlFlow, FromResidual};
    ///
    /// assert_eq!(Result::<String, i64>::from_residual(Err(3_u8)), Err(3));
    /// assert_eq!(Option::<String>::from_residual(None), None);
    /// assert_eq!(
    ///     ControlFlow::<_, String>::from_residual(ControlFlow::Break(5)),
    ///     ControlFlow::Break(5),
    /// );
    /// ```
    #[lang = "from_residual"]
    #[unstable(feature = "try_trait_v2", issue = "84277")]
    fn from_residual(residual: R) -> Self;
}

If this requirement was changed, though, that would work fine for my purposes (I can't realistically stop the consumer of the library from intentionally abusing return types)

scottmcm commented 2 years ago

FromResidual requires Self: Try

It doesn't, actually. Only the default needs that. If an impl specifies the generic type parameter, rather than using the default, then the Self type doesn't need to be Try.

Quick demonstration:

#![feature(try_trait_v2)]

pub struct Foo(bool);
impl std::ops::FromResidual<Option<std::convert::Infallible>> for Foo {
    fn from_residual(_: Option<std::convert::Infallible>) -> Self { Foo(false) }
}

pub fn demo() -> Foo {
    Some(1)?;
    Foo(true)
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=63baa3bdff6c7fc6dbc6162874a4793c

szbergeron commented 2 years ago

Neat, that works. It does come at the cost of requiring a potentially "wrong" FromResidual impl to exist.

Take https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=41467f8b6250614106f7b5ab13655e12 as a simple(ish) example.

Currently, I'm forced to add a image block even though I never want a Foo to be possible to construct from a Foo residual

fogti commented 2 years ago

@szbergeron I tried to get your first example to work and came up with: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=21ae1121a515b43d206b21272f146944 A problem I see with this approach is that we can't enforce every property we want via the language directly; the following ones conflict:

Although it might be possible to hack around this using procedural macros, and maybe two different Try types, to make sure that a function on the inside can only return GuardedResult<_, Joined>, but the outside return value is always GuardedResult<_, Unjoined>...

m-ou-se commented 2 years ago

After having explained the new Try trait to several people, I noticed that it repeatedly comes down to three things that are the main source of confusion:

  1. The names Residual and Output. Especially since Output makes most people think of the 'output' of a function, the return value, which points them in exactly the wrong direction since Output is for the case where ? does not result in returning from a function.

  2. The Try::from_output method. It is mostly relevant for the unstable try {} block, which most are not yet familiar with. Most think of the Try trait as just the trait for the ? operator, which doesn't need a from_output when used in functions.

  3. The FromResidual's default Residual type. It is <Self as Try>::Residual even though FromResidual does not require Try. It is left out in the definition of the Try trait (trait Try: FromResidual { .. }). Making it explicit would make it much clearer that it's a requirement about the conversion from Self::Residual to Self. (trait Try: FromResidual<Self::Residual> { .. })

During the library api meeting we had before merging the RFC, I suggested using the names Break and Continue rather than Residual and Output, and I see it has been suggested by @camsteffen in this thread too. After some experience with using and explaining the new Try feature, I still think these names would work better, and would resolve a lot of the confusion around point 1.

During that same meeting, I also brought up the idea splitting the Try trait into two traits to address confusion source 2. After some experience with this new trait, I am now much more convinced we should do this, as it both makes it easier to explain and removes an unnecessary restriction.

For confusion source 3, I think we can remove the default type and instead write it explicitly in the Try definition.

Here's the idea in more detail:

We split Try into Branch and Try, where Branch represents the ? operator, and Try represents the try block. That way, there is a 1:1 mapping of operators and traits, just like we have for the other operators like Add and AddAssign.

The Branch trait does not require FromResidual (or FromBreak), meaning that it can be implemented for custom error types that you cannot construct, which can potentially be useful for FFI wrappers.

The Try trait now only has the from_output (or from_continue or wrap) method to allow it to wrap the outcome in Ok or Some (etc.), and now requires both Branch and FromResidual<Self::Residual> (or FromBreak<Self::Break>) as super traits.

I think the name wrap rather than from_output could work well, since it does the opposite of .unwrap() on types like Option and Result.

The result, with the renames applied, would look like this:

/// The `?` operator.
trait Branch {
    /// The type of the value to continue with.
    type Continue;

    /// The type of the value to break the control flow with.
    ///
    /// This will be converted through `FromBreak::from_break` into the return value
    /// of the surrounding function or `try` block.
    type Break;

    /// Decide whether to continue or return early.
    fn branch(self) -> ControlFlow<Self::Break, Self::Continue>;
}

/// Conversion of `Branch::Break` to the return type of the function or `try` block.
trait FromBreak<Break> {
    fn from_break(_: Break) -> Self;
}

/// The `try` block.
trait Try: Branch + FromBreak<Self::Break> {
    /// Wrap the final value of a `try` block into the resulting type.
    fn wrap(_: Self::Continue) -> Self;
}

(Side note: An option might be to also split off a Wrap trait, changing Try to trait Try: Branch + Wrap<Self::Continue> + FromBreak<Self::Break> {}, which feels almost like a terse explanation of what a try {} block does. (Or maybe we don't even need a Try trait anymore at that point.))

m-ou-se commented 2 years ago

To explain why Residual Break is Result<!, E> rather than E or Result<T, E>, I usually explain that it is basically the type of Err(e), in which T is not relevant.

Using the name changes proposed above, a more complete explanation could be:

Taking Result as an example, branch() will map a Result<T, E> containing Ok(x) or Err(y) to either continuing with x, or breaking with Err(y). Note that while x is no longer wrapped in Result::Ok, y is still wrapped in Result::Err. If r is Ok(x), r? will evaluate to x (unwrapped!), but if r is an Err(_), the surrounding function will return with an Err(_) (still wrapped!).

The types of the two possible outcomes, x and Err(y), are called Continue and Break. Since the type of x is not relevant for Err(y), Break is of type Result<!, E> rather than Result<T, E>. This break value is converted into the return type of the surrounding function or try block through the FromBreak trait.

Keeping the Err(_) wrapper around in the Break value is important to remember it came from a Result and represents an error. This prevents it from getting accidentally getting used as another Break type that does not represent errors.

Any Result<_, E> implements FromResidual<Result<!, E>> such that you can apply ? to a Result<i32, E> even when the surrounding function or try block returns a Result<String, E>.

The resulting value of a try block is automatically wrapped in Ok through the Try trait's wrap method.

kevincox commented 2 years ago

I would recommend avoiding the name Continue because there is already a continue in Rust and IIUC it is completely unrelated.

In my opinion the most understandable solution is to use Result in the API because everyone who uses rust already understand what it means and it means that implementing this trait for a custom type is as simple as mapping it to Result. It seems like we are favouring the built-in type but I don't think that it actually important. It is just that it happens to be the purest incarnation of the concept. But I know moved away from that with v2 so maybe I missed the important reason.

m-ou-se commented 2 years ago

In my opinion the most understandable solution is to use Result in the API

Then we'd be back at 'Try trait v1' and its problems. See the motivation section of the v2 RFC: https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#motivation

kevincox commented 2 years ago

Maybe I disagree with the motivations listed. To me the From part makes sense and I would stick with another trait. But I think using Result in the API of that trait still makes sense.

tmccombs commented 2 years ago

@kevincox you think that returning an Err when something succeeds is less confusing than the "Continue" terminology?

Personally, I think that is much more confusing. And from what I've read on this issue and related issues, I think a lot of people would agree with me.

Stargateur commented 2 years ago

I would recommend avoiding the name Continue because there is already a continue in Rust and IIUC it is completely unrelated.

It's really not a problem, it's follow the same concept of continue keyword, and we already have a Continue with ControlFlow without any problem of confusion with keyword continue

nyuichi commented 2 years ago

@m-ou-se Perhaps I might have missed the discussion, but I could not find the reason why we want to exclude from_break in the Try trait. My thought is merging FromBreak and Try into a single one would be much clearer to me, where the API would look like the following.

/// The `?` operator.
trait Branch {
    type Continue;
    type Break;
    fn branch(self) -> ControlFlow<Self::Break, Self::Continue>;
}

/// The `try` block.
trait Try: Branch {
    fn from_continue(_: Self::Continue) -> Self;
    fn from_break(_: Self::Break) -> Self;
}
camsteffen commented 2 years ago

@nyuichi Splitting FromBreak allows you to have impl FromBreak<T> for multiple T.

scottmcm commented 2 years ago

@m-ou-se I've been thinking more about this, and I think where I really want to hear more is from people who actually have this scenario:

meaning that it can be implemented for custom error types that you cannot construct, which can potentially be useful for FFI wrappers

Because AFAIK that's currently only a hypothetical, not something that I've seen people doing to know how much of an issue it is. For example, windows::core::HRESULT can easily implement both the introduction and elimination directions.

The reason that's important to me is that the option for "you can create it with from_continue or from_break but you can't actually branch it" would also be a useful direction.

For example, I could imagine writing something like this

#[test]
fn yes_it_works() -> QuestionMarkIsUnwrap {
    foo()?;
    bar()?;
    QuestionMarkIsUnwrap::SUCCESS
}

Where there's no need for QuestionMarkIsUnwrap to support ? -- it's fine just having from_output(()) and from_break(impl Debug).

(Coupled with try{} that type might also make a great rustdoc default main function return type.)

Such a design might then have a structure like

trait TryBlock {
    type Continue;
    fn from_continue(_: Self::Continue) -> Self;
}

trait FromBreak<B> : TryBlock {
    fn from_break(_: B) -> Self;
}

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

The bound on FromBreak could also be on Branch instead, with FromBreak having no supertrait. I haven't thought through those implications in detail. But part of me thinks that it would be good to have create-from-short-circuit-in-? only work on types that are also valid for create-from-continue-in-try{}.

m-ou-se commented 2 years ago

Interesting! I'll try to think of an example.

m-ou-se commented 2 years ago

@m-ou-se I've been thinking more about this, and I think where I really want to hear more is from people who actually have this scenario:

meaning that it can be implemented for custom error types that you cannot construct, which can potentially be useful for FFI wrappers

If I remember correctly, @dtolnay also thought this might be a realistic scenario. @dtolnay, do you have of any example of this?

afetisov commented 2 years ago

I don't like the Residual terminology, it feels very abstract and doesn't have any precedent in other languages or the literature (I have no problems with Output though). From that PoV the names Try::Continue and Try::Break indeed make more sense. However, ControlFlow already has Continue and Break variants, which makes the terminology strongly confusing for me. I would expect that for ControlFlow or a similar type I would just map ControlFlow::Continue to Try::Continue and ControlFlow::Break to Try::Break, but of course that's not the way it works. The asymmetry in the definition and usage of those two associated types is very jarring, and I feel it would be a point of confusion. The names Try::Output and Try::Residual are much better from that view: Output is a very natural thing that I want the try ? operator to return (not "throw out of function", just return like a normal operator, like a dereference or an unary minus), while Residual is something slightly esoteric and complicated which I need to look up in the docs and give some special thought. The name Output is especially great IMHO, since it directly describes the output of the operator, just like Add::Output (addition operator), Deref::Output (reference operator) or FnOnce::Output (call operator).

I'm all for a better name than Residual, but I would want it to be just as asymmetric and without wrong connotations.

I am also very wary of the ideas to split the trait further. Yes, it gives more flexibility, but it also gives more moving parts, which means more stuff to wrap the head around and more ways to subtly break the implementation. In particular, it feels very wrong to split try {} and ? into separate parts. This would mean that someone could implement one but not the other, which would be very weird. Why can I use ? to early-return from a function, but I can't do the same in a try block? Why would I ever want to implement success-wrapping in a try {} block without the possibility of using try operator?

This is reminiscent of splitting addition (and other arithmetic operations) into Add and AddAssign, and I am very unhappy with it. I know all the reasons why they are separate, but it's still a bunch of small and large papercuts in the language for my use cases. It means that I must implement more stuff. If I must support operations on both values and references, then there is even more boilerplate to add, and it is a pain to use those bounds in generic code. For all I care a += b is syntax sugar to avoid writing a = a + b (where a and b can be very long and complex identifiers, or even expressions, so that sugar is very worthwhile). But if I'm writing generic code, then I must choose between Add or AddAssign or Add + AddAssign bounds (and it gets even worse with operations on references). If I choose Add, then I cannot use += sugar in the code. If I require AddAssign, then it's one more thing that the consumer needs to worry about (and if they want to use a foreign type which doesn't impl AddAssign, they're stuck). And in any case I must deal with the possibility that a = a + b and a += b could be doing very different things.

The same kind of troubles will likely follow excessive splitting of Try. But while the Add/AddAssign split is inevitable due to the language semantics (+= operates on places and thus must use a &mut, which we can't get from Add without cloning), the splitting of Try feels like overengineering, rather than solving a pressing problem. The possibility of enabling esoteric use cases will cause trouble in rather standard generic code.

It also makes no sense to split try {} and ? from the PoV of effect semantics. From that PoV try is an effect (fallibility), with the normal evaluation producing the Output, and the side effect produces a thrown exception, which can either be propagated to the caller via the ? operator or explicitly handled with a match. It is similar to async {}, which describes the asynchrony effect, or the sequence effect captured by the Generator trait (no current special denotation on blocks). With asynchrony, the side effect consists of execution suspension with no value, and the side effect can be propagated to the caller via .await operator, and the effect handling is performed by the executor. With generators, the side effect consists of an extra returned value, and can be propagated upwards with yield expressions (no special effect handlers). Both effects allow resumption after handling the effect, but that is the detail of their semantics rather than something inherent to effects.

From that PoV splitting try {} and ? into separate parts makes as much sense as splitting async {} and .await into separate parts, or splitting yield from a normal return of value from the generator. Which is, not any sense at all. What would a construct like that even mean? On the other hand, the Try/FromResidual pair is much closer to the effect/handler pair: the Try trait describes the possible side effects (either producing a value normally or returning with an exception), while FromResidual turns the weird side effect (producing a Residual) into a normal value Self: Try which can be consumed by the calling code.

fogti commented 2 years ago

I also tried applying the "present in nightly" try_trait_v2 traits to one of my projects: https://github.com/zseri/gardswag/commit/0923d8d5d7af7ac9ef8079990ceb605091249d1c For me, the need to duplicate part of the implementation ( From<Option<Result<_,_>>> vs the FromResidual + Try implementations) was a bit annoying, it wasn't clear to me how that should be avoided (it was necessary bc of usage in line 313, to convert in case that the ? operator isn't used), except via try blocks.