Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
9 of 23 tasks
Tracked by #1568
scottmcm opened this issue Apr 17, 2021 · 111 comments
Open
9 of 23 tasks
Tracked by #1568
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-try_trait_v2 Tracking issue for RFC#3058 S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Apr 17, 2021

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 rust-lang/rfcs#1859, tracked in #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:

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

Implementation history

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-try_trait_v2 Tracking issue for RFC#3058 B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue May 18, 2021
Implement the new desugaring from `try_trait_v2`

~~Currently blocked on rust-lang#84782, which has a PR in rust-lang#84811 Rebased atop that fix.

`try_trait_v2` tracking issue: rust-lang#84277

Unfortunately this is already touching a ton of things, so if you have suggestions for good ways to split it up, I'd be happy to hear them.  (The combination between the use in the library, the compiler changes, the corresponding diagnostic differences, even MIR tests mean that I don't really have a great plan for it other than trying to have decently-readable commits.

r? `@ghost`

~~(This probably shouldn't go in during the last week before the fork anyway.)~~ Fork happened.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue May 20, 2021
Implement the new desugaring from `try_trait_v2`

~~Currently blocked on rust-lang/rust#84782, which has a PR in rust-lang/rust#84811 Rebased atop that fix.

`try_trait_v2` tracking issue: rust-lang/rust#84277

Unfortunately this is already touching a ton of things, so if you have suggestions for good ways to split it up, I'd be happy to hear them.  (The combination between the use in the library, the compiler changes, the corresponding diagnostic differences, even MIR tests mean that I don't really have a great plan for it other than trying to have decently-readable commits.

r? `@ghost`

~~(This probably shouldn't go in during the last week before the fork anyway.)~~ Fork happened.
@artemii235
Copy link

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
Copy link
Member

cuviper commented Jul 23, 2021

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
Copy link
Contributor

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
Copy link

Do you mind opening a separate issue?

No objections at all 🙂 I've just created it #87401.

@crlf0710
Copy link
Member

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
Copy link
Contributor

huntiep commented Aug 3, 2021

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

@tmccombs
Copy link
Contributor

tmccombs commented Aug 3, 2021

Use .ok_or(MyError)?

@RagibHasin
Copy link

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
Copy link
Member

@RagibHasin

see #31436 (comment)
and #31436 (comment)
(and the following discusion, respectively)

@BGR360
Copy link
Contributor

BGR360 commented Aug 23, 2021

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
Copy link
Member Author

@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, #42327 (comment) 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
Copy link
Contributor

BGR360 commented Aug 25, 2021

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: #84277 (comment). 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.

@kevincox
Copy link
Contributor

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

@BGR360
Copy link
Contributor

BGR360 commented Aug 27, 2021

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()?)
}

@withoutboats
Copy link
Contributor

withoutboats commented Jun 29, 2023

(NOT A CONTRIBUTION)

The trait which would actually be appropriate for generic return types is Residual, which maps back from a residual to canonical Try type (that one-to-one correspondence), allowing you to map over success type (e.g. Result<T, E> -> Result<U, E>). This is the axis that try_* functionality would potentially be generic over, e.g. collecting impl Iterator<Result<T, E>> -> Result<impl FromIterator, E>.

I think this is the part that people really are concerned about. Frankly, I think Residual should be removed, and all of these. highly generic APIs that cannot be written without it should be removed as well.

I'm going to be very honest about what I think, because I think it is also the opinion of many other prominent community members who are not involved in the libs/lang team clique and have been critical on issues like this or on Twitter. It seems to me that the product design group is trapped in some sort of code golf death spiral where one side creates extremely generic convenience APIs (i.e. try_collect) and the other side proposes whole new axes of abstraction to hide some of that highly generic type signature behind a language feature (i.e. keyword generics), while most users do not have any pressing need for these APIs or abstractions, which are at best "nice to have" and cannot justify the huge cognitive overhead they bring to the language and standard library.

The Try feature should be brought back to its original scope: making try and ? work with multiple types. The actual content of the RFC for Try v2 is fine, in my opinion, with the exception of the one little change I proposed in my previous comment. The addition of the Residual trait and the fake-monad type hacking by way of double projection from Try to Residual and back to Try with a swapped enclosed type needs to be dropped.

@fogti
Copy link
Contributor

fogti commented Jun 29, 2023

the fake-monad type hacking by way of double projection

yeah, I'd rather have proper functor, monad interfaces in Rust than such hackery (and I'd really appreciate that solely because dealing with ASTs which are extended/modified through various stages of progressing lead to that sort-of naturally, and being unable to abstract over multiple of them makes it harder than necessary to write generic AST mangling libraries imo).

@CAD97
Copy link
Contributor

CAD97 commented Jul 3, 2023

Out of curiosity, @withoutboats, modulo exact naming,

Would you consider the use of monadic reprojection be more palatable if it were spelled -> R::Rebind<O> instead of the double projection (-> <R::Residual as Residual<O>>::TryType)? Presuming that it was just an alias to the use of Residual, that it showed up in docs as the alias, and that the language feature exists to associate a type alias to a trait without letting it be overridden.

What if it was a typical generic associated type of the Try trait instead of the double projection, but there was an associated trait alias used to bound the generic type1?

I ask because I'm legitimately interested in whether it's the use of a generic monadic rebind at all (given that it's not strictly necessary when alternative Try types can temporarily reify into Result) which is considered unnecessary cognitive overhead, or if it's just the use of a double projection to accomplish it. (The former is the inherent complexity of making the API generic over this axis; the latter is incidental complexity of how the result is achieved.)

I ask this now prompted by learning of an open draft experiment utilizing a similar double projection scheme to permit allocator-generic collections to do -> A::ErrorHandling::Result<T, E> to select between -> Result<T, E> and -> T based on whether the allocator is (statically) in fallible or infallible mode. This permits the unification of the e.g. reserve/try_reserve split, but at the cost of any potentially allocating method which would have had a try_* version having the more complex signature.


I do agree that doubled type projection is essentially unprecedented in stable std function signatures. The use of a type alias for clarity in the definition of functions doing so is telling, and at an absolute minimum imo the alias should be made public so it will show up in the API docs.

I doubted it for a moment, but the use of a <Type as Trait>::Type return type in the API docs of stable functions is precedented, at least. Namely, Option::as_deref is documented as having the signature (&self) -> Option<&<T as Deref>::Target> where T: Deref2.

Footnotes

  1. The ability to bound this is why we currently have <R::Residual as Residual<O>>::TryType and not R::Residual::TryType<O>. (...And that doing so would currently also cause an ambiguous associated type error, but IIUC that's considered a bug/fixable limitation.) The bound is necessary to support e.g. ErrnoResult.

  2. This is despite it being implemented with -> Option<&T::Target>; I presume rustdoc doesn't bother figuring out whether a projection can be treated as unambiguous. Especially since when such a type projection would be ambiguous in rustdoc is a complicated and distinct question than it being unambiguous in the source code.

@withoutboats
Copy link
Contributor

withoutboats commented Jul 3, 2023

(NOT A CONTRIBUTION)

Would you consider the use of monadic reprojection be more palatable if it were spelled -> R::Rebind<O> instead of the double projection

Both the action of rebinding the wrapped type and the use of double projection to do so add cognitive overhead to the API. A GAT certainly seems simpler than a double projection, though when you start talking about "associated trait aliases" I get very suspicious. I think either aspect of this alone is complex enough to warrant a lot of caution, and require very compelling motivation. I don't see any of the enabled APIs as particularly well motivated, for exactly the reason we've discussed - you can just map things to Result if you really want, though even with Result I doubt the utility of all these try_ variants at all.

I ask this now prompted by learning of an open #111970 utilizing a similar double projection scheme to permit allocator-generic collections

My immediate impression is also to be dubious of these fallible allocator APIs. I think allocators is another area where the working group seems to have gone down a rabbit hole and lost touch with how much complexity they're adding to the APIs. I realize there are motivations, but these motivations need to be balanced against how difficult they make std to understand. Maybe I underestimate how compelling the motivations are for that case, I haven't followed the work closely.

Overall, I think most people who work on Rust open source are very disconnected from how most non-hobbyist users experience Rust. They don't have time to nerd out about the language, so the model of the type system in their head is a bit wrong or incomplete, and they can easily be frustrated or confused or led astray by the kinds of type wizardry the online Rust community embraces without question. Rust has always been trying to bring a great type system "to the masses" (in total opposition to Rob Pike's statement that users "are not capable of understanding a brilliant language"), but this means recognizing when the cognitive overhead complex types are adding is not justified by the utility they bring to the API. This is especially true when that utility is basically just omitting a type conversion, as in a lot of these Try cases.

Of course, this sort of thing is fine in third party crates, where it can be proved out and experimented with and in libraries targeted at a certain kind of user can be effective for that user in achieving their goals. But the standard library should be a bulwark against this sort of programming, because the standard library is the lowest common denominator for all users.

Namely, Option::as_deref is documented as having the signature (&self) -> Option<&<T as Deref>::Target> where T: Deref

This seems like sloppiness to me, as_deref should be in a separate impl block with the where clause on the impl block, rather than the where clause on the method in a normal impl block. This should solve the problem with rustdoc (though I also think this is a poor showing from rustdoc as well). Not sure if this is a technically breaking change to make, though.

@clarfonthey
Copy link
Contributor

I don't have too much to say regarding the actual design of the trait, although I think it's apt to bring up this argument I made in the original RFC: rust-lang/rfcs#3058 (comment)

The term "residual", despite being weird English, is unique enough that it could become synonymous with what it's being used for here. There are lots of terms that aren't used in Normal English that are used in Computer English (for example, verbose) and I think that adding another, although weird, is not the worst.

Essentially, however weird the idea of a "residual" is, the term is unique enough that its current meaning in Rust can be learned in isolation.

Folks have demonstrated that they have made use of the FromResidual trait in their own implementations and I think that it would be nice to retain this in the stable implementation. I do however sympathise greatly with the bounds required for methods that do use the Try trait, requiring both Try and FromResidual in the bounds.

I think that FromResidual should probably exist as a relatively "niche" feature that does not need to be understood to use the Try trait in most cases. It should be required to implement the Try trait, but that's a higher bar IMHO, and most people just want to make try_* methods that work without having to understand the whole system.

I feel like maybe more effort should be put into ensuring that the Try trait is easy to use, without sacrificing the flexibility of FromResidual.

@fogti
Copy link
Contributor

fogti commented Jul 3, 2023

@CAD97 just to note: I consider such double projection, especially as in applications of them also trait bounds on them can appear in downstream or implementation code, to be an implementation detail that imo shouldn't be necessary, i.e. should be abstracted by the language, leading to a cleaner interface.

re: the concern of "niche" or

Overall, I think most people who work on Rust open source are very disconnected from how most non-hobbyist users experience Rust. They don't have time to nerd out about the language, so the model of the type system in their head is a bit wrong or incomplete, and they can easily be frustrated or confused or led astray by the kinds of type wizardry the online Rust community embraces without question.

I believe that it shouldn't be necessary for most normal users to deal with the intricate parts of the API directly at all in most use cases, and if they do, it should be minimally invasive. A long type signature with a large amount of trait bounds and such that can't be abstracted away (except via macros, which makes the docs unhelpful) is a strong anti-pattern, and imo thus warrants a more fundamental solution, e.g. making all monads easier to express in rust instead of abstracting over it via a combination of multiple interlocked traits with potentially confusing semantics, and also potentially harder to read error messages in case of failures of type inference and such.

Such interfaces are not only annoying to write/copy-paste and debug, they pose a mental burden, make it harder to present users with good error messages (because the compiler doesn't see the actual abstraction but a workaround around the lack of abstraction, mostly), and might also make type inference/checking unnecessarily harder (and e.g. "simple guesses" by the compiler in case of errors also get much harder, both from the "implement this in the compiler" and "make it fast and maintainable" (also in regard to similar patterns which might evolve in third-party crates, etc.).

e.g. (rust-like pseudo-code)
enum ControlFlow<B, C> {
    /// Exit the operation without running subsequent phases.
    Break(B),
    /// Move on to the next phase of the operation as normal.
    Continue(C),
}

monad_morph<B, C> ControlFlow<B, C> {
    type Monadic<C> = ControlFlow<B, C>;
    fn pure<C>(t: C) -> Self::Monadic<C> {
        ControlFlow::Continue(t)
    }
    fn flat_map<C1, C2, F>(input: Self::Monadic<C1>, f: F) -> Self::Monadic<C2>
    where F: /* function trait used might vary per monad_morph */ FnOnce(C1) -> Self::Monadic<C2>,
    {
        match input {
            ControlFlow::Break(b) => ControlFlow::Break(b),
            ControlFlow::Continue(c) => f(c),
        }
    }
    /* a question that remains here would be how to handle the "short-circuiting" generally and effectively (that is, without many nested closures)
     * probably similar to the residual stuff, but it would be interesting to see how it could be done generally
     * (e.g. simple ControlFlow), while also allowing lazier interfaces (like monads similar to iterators (considering `flat_map` there)) */
}

also, another idea would be to introduce some kind of "tagged ControlFlow", and just use that everywhere (especially try_ functions), forgoing the most difficult parts of this, while still allowing guided interconversions (mediated by some kind of type-level tags (ZSTs))

@gtsiam
Copy link

gtsiam commented Jul 4, 2023

I think that FromResidual should probably exist as a relatively "niche" feature that does not need to be understood to use the Try trait in most cases.

That is already kinda the case. If you're using a Try type, you don't need to care about FromResidual. If you're writing one, then you will be forced to write exactly one impl of it, which is only fair.

Then again, if you're using one of the ready-made functions, you won't need to care much about the traits beyond the documentation.


However, unless I'm reading the thread wrong, most of the discussion right now seems to be about the Residual trait, as opposed to the Try & FromResidual traits. The relevant tracking issue is #91285, not here.

For what it's worth, I think things like try_collect are useful (and will be even more so when try trait stabilises and Result isn't the only result-like type anymore). That said, double projection does make all the relevant type signatures headache-inducing. So maybe something like this would work better?

trait TryWith<O>: Try {
    type TryWith: Try<Output = O, Residual = Self::Residual>;
}

I'd be cautious of GATs here, since they'd forbid any impl blocks from adding additional bounds on the projected Output, though I haven't thought about it much so I'll leave it at that.

But again, wrong tracking issue.


Also I'll echo @withoutboats on removing the default for R from FromResidual<R> for readability reasons. Appart from that I think the try trait design (which does not include the Residual trait) is fine.

@rakshith-ravi
Copy link
Contributor

As somebody who would like to see this stabilized, what can I do to help? Is there anything I can do to help push this forward?

@rdrpenguin04
Copy link

What still needs to be done?

@onestacked
Copy link
Contributor

onestacked commented Sep 12, 2023

From what I understand the main blocking points are:

  • Naming (keep Output/Residual?)
  • Ergonomics, currently it is kind of painful to use in a lot of cases. Example from std:
    fn try_collect<B>(&mut self) -> ChangeOutputType<Self::Item, B>
        where
            Self: Sized,
            <Self as Iterator>::Item: Try,
            <<Self as Iterator>::Item as Try>::Residual: Residual<B>,
            B: FromIterator<<Self::Item as Try>::Output>,
  • And the other points listed in the issue description

@rdrpenguin04
Copy link

Alright, response:

  • Name seems fine as-is; doesn't seem to need extra bikeshedding if nobody is specifically complaining.
  • Ergonomics could probably be improved without breaking changes. Though, to be fair, that specific function seems like it would just be complicated; it's dealing with Iterator and Try, and the complex types just stem from the same fully-qualified syntax headaches applied to both traits in sequence.
  • The issue description seems to be out of date; multiple unfilled checkboxes point to closed issues. The tracker should probably be updated, which was the main point of my question.

Thank you for the answer; I look forward to pushing this through 😄

@rdrpenguin04
Copy link

@scottmcm Would you be willing to update your checklist to reflect newer changes?

@T-Dark0
Copy link

T-Dark0 commented Oct 20, 2023

I'd like to raise a concern about the default type parameter on FromResidual (that is, <Self as Try>::Residual). I was doing some experimentation today, and it turns out this implementation is considered conflicting

struct Test;
impl<T> FromResidual<Test> for Option<T> {}

With some help from someone on discord (@zachs18), we've been able to determine that this seems to be a consequence of there being an impl<T> FromResidual<<Self as Try>::Residual> for Option<T> rather than a "handwritten" impl<T> FromResidual<Option<Infallible>> for Option<T>. Checking the libcore source, it appears that there's simply an impl<T> FromResidual for Option<T>: The impl is "inheriting" the problematic way to spell the type parameter from the default.

Should we perhaps remove the default type parameter, if using it can lead to this problem? Even if it's just as a temporary measure while we fix whatever bug causes the issue: we can always add the default again later. In the meantime, would a PR changing the impl to not use the default parameter be accepted?

Here's a MRE, adapted from Zachs' multi-file MRE they shared on the Rust community discord: paste this in a library crate named example, comment out either of the impls, and run cargo test --doc to see how one compiles and one does not.

pub trait MyTry {
    type Residual;
}
pub trait MyFromResidual<T> {}

pub struct MyOption<T>(T);
pub enum MyInfallible {}

impl<T> MyTry for MyOption<T> {
    type Residual = Option<MyInfallible>;
}

// Of course these two impls won't both compile. Comment one out.
impl<T> MyFromResidual<MyOption<MyInfallible>> for MyOption<T> {}
impl<T> MyFromResidual<<Self as MyTry>::Residual> for MyOption<T> {}

/// using a doctest as a quick way to have an inline external crate
/// ```
/// use example::{MyFromResidual, MyOption};
/// struct Test;
/// impl<T> MyFromResidual<Test> for MyOption<T> {}
/// ```
pub struct Dummy;

@Kimundi
Copy link
Member

Kimundi commented Jan 13, 2024

For me personally, the main use I would like to see out of a stable Try v2 trait is the ability to write generic adapter types to modify what gets returned on a ?, so that we no longer need to write try_xxx!() macros (that match and return in a custom way) if we have control-flow-heavy code that wants to return standard types like Result or Option on Ok/None cases.

Such an API could look like this:

fn example(arg: Option<u8>) -> Option<u8> {
    arg.try_some()?; // returns if arg is a `Some`
    Some(42)
}

I tried to write up a complete example of this right now, but I'm running into the same issue as the previous poster about the FromResidual impl for Options. Still, here is my attempt: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b1bf5652864308956812f72e71488430

For Results it works fine though, although I had to do a weird workaround to get a Infallible pattern match working, but that seems unrelated to the Try API: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b826e7fd8c8f91e5186501f606f1bcb9

(Pattern match error for who is curios: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=56916aba1cee38c4b1376414cf5e379a)

So, in summary:

  • I'd like to see this kind of API being possible, which means we should keep the Try/FromResidual split
  • We should fix the issue that prevents the Option impl
  • ... which indicates we should probably remove the default type param from FromResidual

As for the Residual trait, I have no specific opinion, nor have I looked into why or if we should have it. As far as I understand it, its an optional extra compononent to make writing generic code easier, so I have the following suggestion:

Lets split up this feature so that we can focus on stabilizing the Try+FromResidual part first, and focus on Residual in isolation.


Also, just an observation, but: When writing the code above, I started to wonder if using the Try Self type with an Infallible type parameter is worth it, just to avoid defining an extra type. Because it makes the impls quite a bit harder to read, compared to just having a type like struct ResultResidue<T>(T). Eg this is what my custom impl looks like with such a type: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=02dff64bfe7b6d70d390aa6c0e73712c

This seems to be a important educational point to me, as explaining how the Output+Residue split works gets harder if you also need to wrap your head around the Infallible trick. Eg explaining and understanding that a Result<T, E> can be split into a T and ErrResidue<E> pair seems a bit simpler to me.

@Stargateur
Copy link
Contributor

Stargateur commented Mar 16, 2024

Here my project that use try trait, https://crates.io/crates/binator:

I didn't work since few months of this projet, I know I tell previously in this thread I will post it when I release it then it is haha not perfect but that something. That a pretty big project that use try trait in a very practical way.

try trait allow me to do really cool stuff having a type that better represent the result of a parser is very nice. and the user can use it like result or option with ?.

@luksan
Copy link

luksan commented Apr 19, 2024

Experience report

I used try_traits_v2 to implement FromResidual for Result<Infallible, impl Into> and Option for a custom Iterator<Item=Result<Value,EvalError>>. I use this in an AST walker where each node can return zero or more Values, or an error. Before I implemented FromResidual I had to use Result<iterator, EvalError> as return type in the visitor methods in order to use "?", which caused a lot of Ok-wrapping and having to handle the fact that an error could be both the outer result or inside the iterator. Also, returning an empty iterator was very explicit.

After FromResidual was implemented I could change the return type on the visitors "iterator" and still use Err(EvalError)? to return errors inside an iterator, or return an empty iterator with None?. All the Ok-wrapping went away.

It was quite straightforward to figure out what was needed. The only stumbling blocks was that the default for R in FromResidual made the RustRover autocomplete the impl skeleton incorrectly for my usage, which caused a few minutes of head-scratching. The other thing that took a few tries was to see that the Ok type on the Result impl must be Infallible, but in that case the type errors from rustc were quite helpful.

All in all, great feature. Works for me. I don't mind the "Residual" name, even though it's not intuitive for me. Just another concept to learn. The default for R might cause more problems than it solves, though.

@paulyoung
Copy link

I recently tried this out by implementing an Either type (isomorphic to Result)

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

I still don't understand why the type of L (equivalent to Ok in Result) can't be inferred. I needed to provide type arguments when I expected not too.

It's possible that I got something wrong or had incorrect expectations but wanted to share my experience in case this wasn't intended.

Thanks!

@WaffleLapkin
Copy link
Member

@paulyoung as you've written, L is the equivalent of Err. Try::Output is the type of x?. Second of all, you don't need all the types, just the ones that can't be inferred.

In let foo = Either::<String, _>::Right("foo".to_string())?; and let foo_bar_baz = Either::Right(format!("{foo_bar} baz"))? the left type can't be inferred because the compiler can't solve ?T with the only bound being String: From<?T> (there are many types for which this is true) (remember that L is the Err-like here). Note that you specify the From bound in the FromResidual impl (if you remove the F and From it will work):

impl<L, R, F: From<L>> FromResidual<Either<L, Infallible>> for Either<F, R> {

With let foo_bar = Either::<_, String>::Left(format!("{foo} bar"))? the right is otherwise unbounded also (Left is Err-like, which means it is just returned and then the R could be anything which implements Display).

@ianks
Copy link

ianks commented Aug 10, 2024

At risk of sounding overly ambitious, let's… ship this?

The overall sentiment has been positive. People feel it's solving real problems and filling an important gap.

Two quick decisions could unblock us:

  1. FromResidual: Keep the flexibility or simplify?
  2. Stick with Output/Residual or change? If someone is passionate about this one, please speak up but let’s not shed too much

Thoughts on pushing this across the finish line?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 14, 2024
…=scottmcm

Explicitly specify type parameter on FromResidual for Option and ControlFlow.

~~Remove type parameter default `R = <Self as Try>::Residual` from `FromResidual`~~ _Specify default type parameter on `FromResidual` impls in the stdlib_ to work around rust-lang#99940 / rust-lang#87350 ~~as mentioned in rust-lang#84277 (comment).

This does not completely fix the issue, but works around it for `Option` and `ControlFlow` specifically (`Result` does not have the issue since it already did not use the default parameter of `FromResidual`).

~~(Does this need an ACP or similar?)~~ ~~This probably needs at least an FCP since it changes the API described in [the RFC](rust-lang/rfcs#3058). Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api).~~ This probably doesn't need T-lang input, since it is not changing the API of `FromResidual` from the RFC? Maybe needs T-libs-api FCP?
jieyouxu added a commit to jieyouxu/rust that referenced this issue Aug 14, 2024
…=scottmcm

Explicitly specify type parameter on FromResidual for Option and ControlFlow.

~~Remove type parameter default `R = <Self as Try>::Residual` from `FromResidual`~~ _Specify default type parameter on `FromResidual` impls in the stdlib_ to work around rust-lang#99940 / rust-lang#87350 ~~as mentioned in rust-lang#84277 (comment).

This does not completely fix the issue, but works around it for `Option` and `ControlFlow` specifically (`Result` does not have the issue since it already did not use the default parameter of `FromResidual`).

~~(Does this need an ACP or similar?)~~ ~~This probably needs at least an FCP since it changes the API described in [the RFC](rust-lang/rfcs#3058). Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api).~~ This probably doesn't need T-lang input, since it is not changing the API of `FromResidual` from the RFC? Maybe needs T-libs-api FCP?
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 14, 2024
Rollup merge of rust-lang#128954 - zachs18:fromresidual-no-default, r=scottmcm

Explicitly specify type parameter on FromResidual for Option and ControlFlow.

~~Remove type parameter default `R = <Self as Try>::Residual` from `FromResidual`~~ _Specify default type parameter on `FromResidual` impls in the stdlib_ to work around rust-lang#99940 / rust-lang#87350 ~~as mentioned in rust-lang#84277 (comment).

This does not completely fix the issue, but works around it for `Option` and `ControlFlow` specifically (`Result` does not have the issue since it already did not use the default parameter of `FromResidual`).

~~(Does this need an ACP or similar?)~~ ~~This probably needs at least an FCP since it changes the API described in [the RFC](rust-lang/rfcs#3058). Not sure if T-lang, T-libs-api, T-libs, or some combination (The tracking issue is tagged T-lang, T-libs-api).~~ This probably doesn't need T-lang input, since it is not changing the API of `FromResidual` from the RFC? Maybe needs T-libs-api FCP?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-try_trait_v2 Tracking issue for RFC#3058 S-tracking-design-concerns Status: There are blocking ❌ design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests