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 ops::Try (try_trait feature) #42327

Closed
11 tasks done
scottmcm opened this issue May 31, 2017 · 107 comments
Closed
11 tasks done

Tracking issue for ops::Try (try_trait feature) #42327

scottmcm opened this issue May 31, 2017 · 107 comments

Comments

@scottmcm
Copy link
Member

@scottmcm scottmcm commented May 31, 2017

Feature gate: #![feature(try_trait)]

This is a tracking issue for the Try trait from rust-lang/rfcs#1859.

Split off from #31436 for clarity (per #42275 (comment))

Public API

pub mod core {
    pub mod result {
        impl<T, E> ops::Try for Result<T, E> {
            type Ok = T;
            type Error = E;
            fn into_result(self) -> Self {}
            fn from_ok(v: T) -> Self {}
            fn from_error(v: E) -> Self {}
        }
    }

    pub mod option {
        #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
        pub struct NoneError;

        impl<T> ops::Try for Option<T> {
            type Ok = T;
            type Error = NoneError;
            fn into_result(self) -> Result<T, NoneError> {}
            fn from_ok(v: T) -> Self {}
            fn from_error(_: NoneError) -> Self {}
        }
    }

    pub mod ops {
        mod r#try {
            pub trait Try {
                type Ok;
                type Error;
                fn into_result(self) -> Result<Self::Ok, Self::Error>;
                fn from_error(v: Self::Error) -> Self;
                fn from_ok(v: Self::Ok) -> Self;
            }
        }

        pub use self::r#try::Try;
    }
}

Steps / History

  • Stabilizing this will allow people to implement Iterator::try_fold

    • As part of stabilizing, re-open PR #62606 to document implementing try_fold for iterators
    • Ensure that the default implementations of other things have the desired long-term DAG, since changing them is essentially impossible later. (Specifically, it would be nice to have fold be implemented in terms of try_fold, so that both don't need to be overridden.)
    • Moved these to the try_trait_v2 tracking issue
  • #42275

  • Deprecate and remove these in favour of the new versions.

Unresolved Questions

These resulted in a new rust-lang/rfcs#3058, tracked in #84277

@glaebhoerl
Copy link
Contributor

@glaebhoerl glaebhoerl commented Jun 28, 2017

A couple of pieces of bikeshedding:

  • Do we have a particular motivation to call the associated type Error instead of Err? Calling it Err would make it line up with Result: the other one is already called Ok.

  • Do we have a particular motivation for having separate from_error and from_ok methods, instead of a single from_result which would be more symmetric with into_result which is the other half of the trait?

(updated version of playground link from the RFC)

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Jun 28, 2017

@glaebhoerl

  • Error vs Err was discussed related to TryFrom in #33417 (comment) and #40281; I assume the name was chosen here for similar reasons.
  • I believe they're separate because they have different uses, and I expect it to be rare that someone actually has a Result that they're trying to turn into a T:Try. I prefer Try::from_ok and Try::from_error to always calling Try::from_result(Ok( and Try::from_result(Err(, and I'm happy to just impl the two methods over writing out the match. Perhaps that's because I think of into_result not as Into<Result>, but as "was it pass or fail?", with the specific type being Result as an unimportant implementation detail. (I don't want to suggest or reopen the "there should be a new type for produce-value vs early-return, though.) And for documentation, I like that from_error talks about ? (or eventually throw), while from_ok talks about success-wrapping (#41414), rather than having them both in the same method.

@fluffysquirrels
Copy link

@fluffysquirrels fluffysquirrels commented Jun 30, 2017

I'm not sure if this is the correct forum for this comment, please redirect me if it's not 😃. Perhaps this should have been on rust-lang/rfcs#1859 and I missed the comment period; oops!


I was wondering if there is a case for splitting up the Try trait or removing the into_result method; to me currently Try is somewhat like the sum of traits FromResult (containing from_error and from_ok) and IntoResult (containing into_result).

FromResult enables highly ergonomic early exit with the ? operator, which I think is the killer use case for the feature. I think IntoResult can already be implemented neatly with per-use-case methods or as Into<Result>; am I missing some useful examples?

Following the Try trait RFC, expr? could desugar as:

match expr { // Removed `Try::into_result()` here.
    Ok(v) => v,
    Err(e) => return Try::from_error(From::from(e)),
}

The motivating examples I considered are Future and Option.

Future

We can implement FromResult naturally for struct FutureResult: Future as:

impl<T, E> FromResult for FutureResult {
    type Ok = T;
    type Error = E;
    fn from_error(v: Self::Error) -> Self {
        future::err(v)
    }
    fn from_ok(v: Self::Ok) -> Self {
        future::ok(v)
    }
}

Assuming a reasonable implementation for ? that will return from an impl Future valued function when applied to a Result::Err then I can write:

fn async_stuff() -> impl Future<V, E> {
    let t = fetch_t();
    t.and_then(|t_val| {
        let u: Result<U, E> = calc(t_val);
        async_2(u?)
    })
}
fn fetch_t() -> impl Future<T, E> {}
fn calc(t: T) -> Result<U,E> {}

That is exactly what I was trying to implement earlier today and Try nails it! But if we tried to implement the current Try for Future there is perhaps no canonical choice for into_result; it could be useful to panic, block, or poll once, but none of these seems universally useful. If there were no into_result on Try I can implement early exit as above, and if I need to convert a Future to a Result (and thence to any Try) I can convert it with a suitable method (call the wait method to block, call some poll_once() -> Result<T,E>, etc.).

Option

Option is a similar case. We implement from_ok, from_err naturally as in the Try trait RFC, and could convert Option<T> to Result<T, Missing> simply with o.ok_or(Missing) or a convenience method ok_or_missing.


Hope that helps!

@skade
Copy link
Contributor

@skade skade commented Jul 30, 2017

I'm maybe very late to all this, but it occured to me this weekend that ? has a rather natural semantic in cases where you'd like to short-circuit on success.

fn fun() -> SearchResult<Socks> {
    search_drawer()?;
    search_wardrobe()
}

But in this case, the naming of the Try traits methods don't fit.

It would widen the meaning of the ? operator, though.

@cuviper
Copy link
Member

@cuviper cuviper commented Oct 25, 2017

In prototyping a try_fold for rayon, I found myself wanting something like Try::is_error(&self) for the Consumer::full() methods. (Rayon has this because consumers are basically push-style iterators, but we still want to short-circuit errors.) I instead had to store the intermediate values as Result<T::Ok, T::Err> so I could call Result::is_err().

From there I also wished for a Try::from_result to get back to T at the end, but a match mapping to T::from_ok and T::from_error isn't too bad.

@fluffysquirrels
Copy link

@fluffysquirrels fluffysquirrels commented Oct 28, 2017

The Try trait can provide a from_result method for ergonomics if from_ok and from_error are required. Or vice versa. From the examples given I see a case for offering both.

@ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Dec 29, 2017

Since PR #42275 has been merged, does that mean this issue has been resolved?

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Dec 29, 2017

@skade Note that a type can define whichever as the short-circuiting one, like LoopState does for some Iterator internals. Perhaps that would be more natural if Try talked about "continuing or breaking" instead of success or failure.

@cuviper The version I ended up needing was either the Ok type or the Error-in-original type. I'm hoping that destructure-and-rebuild is a general enough thing that it'll optimize well and a bunch of special methods on the trait won't be needed.

@ErichDonGubler This is a tracking issue, so isn't resolved until the corresponding code is in stable.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Feb 20, 2018

Experience report:

I've been a little frustrated trying to put this trait into practice. Several times now I've been tempted to define my own variant on Result for whatever reason, but each time I've wound up just using Result in the end, mostly because implementing Try was too annoying. I'm not entirely sure if this is a good thing or not, though!

Example:

In the new solver for the Chalk VM, I wanted to have an enum that indicates the result of solving a "strand". This had four possibilities:

enum StrandFail<T> {
    Success(T),
    NoSolution,
    QuantumExceeded,
    Cycle(Strand, Minimums),
}

I wanted ?, when applied to this enum, to unwrap "success" but propagated all other failures upward. However, in order to implement the Try trait, I would have had to define a kind of "residual" type that encapsulates just the error cases:

enum StrandFail {
    NoSolution,
    QuantumExceeded,
    Cycle(Strand, Minimums),
}

But once I've got this type, then I might as well make StrandResult<T> an alias:

type StrandResult<T> = Result<T, StrandFail>;

and this is what I did.

Now, this doesn't necessarily seem worse than having a single enum -- but it does feel a bit odd. Usually when I write the documentation for a function, for example, I don't "group" the results by "ok" and "error", but rather talk about the various possibilities as "equals". For example:

    /// Invoked when a strand represents an **answer**. This means
    /// that the strand has no subgoals left. There are two possibilities:
    ///
    /// - the strand may represent an answer we have already found; in
    ///   that case, we can return `StrandFail::NoSolution`, as this
    ///   strand led nowhere of interest.
    /// - the strand may represent a new answer, in which case it is
    ///   added to the table and `Ok` is returned.

Note that I didn't say "we return Err(StrandFail::NoSolution). This is because the Err just feels like this annoying artifact I have to add.

(On the other hand, the current definition would help readers to know what the behavior of ? is without consulting the Try impl.)

I guess this outcome is not that surprising: the current Try impl forces you to use ? on things that are isomorphic to Result. This uniformity is no accident, but as a result, it makes it annoying to use ? with things that are not basically "just Result". (For that matter, it's basically the same problem that gives rise to NoneError -- the need to artificially define a type to represent the "failure" of an Option.)

I'd also note that, in the case of StrandFail, I don't particularly want the From::from conversion that ordinary results get, though it's not hurting me.

@glaebhoerl
Copy link
Contributor

@glaebhoerl glaebhoerl commented Feb 20, 2018

Is the associated Try::Error type ever exposed to / used by the user directly? Or is it just needed as part of the desugaring of ?? If the latter, I don't see any real problem with just defining it "structurally" - type Error = Option<Option<(Strand, Minimums)>> or whatever. (Having to figure out the structural equivalent of the "failure half" of the enum definition isn't great, but seems less annoying than having to rejigger the whole public-facing API.)

@kevincox
Copy link
Contributor

@kevincox kevincox commented Feb 20, 2018

I don't follow either. I have successfully implemented Try for a type that had an internal error representation and it felt natural having Ok and Error being the same type. You can see the implementation here: https://github.com/kevincox/ecl/blob/8ca7ad2bc4775c5cfc8eb9f4309b2666e5163e02/src/lib.rs#L298-L308

In fact is seems like there is a fairly simple pattern for making this work.

impl std::ops::Try for Value {
	type Ok = Self;
	type Error = Self;
	
	fn from_ok(v: Self::Ok) -> Self { v }
	fn from_error(v: Self::Error) -> Self { v }
	fn into_result(self) -> Result<Self::Ok, Self::Error> {
		if self.is_ok() { Ok(val) } else { Err(val) }
	}
}

If you want to unwrap the success something like this should work:

impl std::ops::Try for StrandFail<T> {
	type Ok = T;
	type Error = Self;
	
	fn from_ok(v: Self::Ok) -> Self { StrandFail::Success(v) }
	fn from_error(v: Self::Error) -> Self { v }
	fn into_result(self) -> Result<Self::Ok, Self::Error> {
		match self {
			StrandFail::Success(v) => Ok(v),
			other => Err(other),
		}
	}
}

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Feb 20, 2018

Defining a structural type is possible but feels pretty annoying. I agree I could use Self. It feels a bit wacky to me though that you can then use ? to convert from StrandResult to Result<_, StrandResult> etc.

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Mar 28, 2018

Great experience report, @nikomatsakis! I've also been dissatisfied with Try (from the other direction).

TL/DR: I think FoldWhile got this right, and we should double-down on the Break-vs-Continue interpretation of ? instead talking about it in terms of errors.

Longer:

I keep using Err for something closer to "success" than "error" because ? is so convenient.

So if nothing else, I think the description I wrote for Try is wrong, and shouldn't talk about a "success/failure dichotomy", but rather abstract away from "errors".

The other thing I've been thinking about is that we should consider some slightly-crazy impls for Try. For example, Ordering: Try<Ok = (), Error = GreaterOrLess>, combined with "try functions", could allow cmp for struct Foo<T, U> { a: T, b: U } to just be

fn cmp(&self, other: &self) -> Ordering try {
    self.a.cmp(&other.a)?;
    self.b.cmp(&other.b)?;
}

I don't yet know whether that's the good kind of crazy or the bad kind 😆 It's certainly got some elegance to it, though, since once things are different you know they're different. And trying to assign "success" or "error" to either side of that doesn't seem to fit well.

I do also note that that's another instance where the "error-conversion" isn't helpful. I also never used it in the "I want ? but it's not an error" examples above. And it's certainly known to cause inference sadness, so I wonder if it's a thing that should get limited to just Result (or potentially removed in favour of .map_err(Into::into), but that's probably infeasible).

Edit: Oh, and all that makes me wonder if perhaps the answer to "I keep using Result for my errors instead of implementing Try for a type of my own" is "good, that's expected".

Edit 2: This is not unlike #42327 (comment) above

Edit 3: Seems like a Continue variant was also proposed in rust-lang/rfcs#1859 (comment)

@tmccombs
Copy link
Contributor

@tmccombs tmccombs commented Mar 31, 2018

My two cents:

I like @fluffysquirrels's suggestion of splitting the trait into two traits. One for converting to a result, and another to convert from a result. But I do think we should keep the into_result or equivalent as part of the desugaring. And I think at this point we have to since using Option as a Try has stabilized.

I also like @scottmcm's idea of using names that suggest break/continue rather than error/ok.

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Apr 2, 2018

To put the specific code in here, I like how this reads:

fn find<P>(&mut self, mut predicate: P) -> Option<Self::Item> where
Self: Sized,
P: FnMut(&Self::Item) -> bool,
{
self.try_for_each(move |x| {
if predicate(&x) { LoopState::Break(x) }
else { LoopState::Continue(()) }
}).break_value()
}

It's of course not quite as nice as a straight loop, but with "try methods" it would be close:

self.try_for_each(move |x| try { 
    if predicate(&x) { return LoopState::Break(x) } 
}).break_value() 

For comparison, I find the "error vocabulary" version really misleading:

self.try_for_each(move |x| { 
    if predicate(&x) { Err(x) } 
    else { Ok(()) } 
}).err() 

@cowang4
Copy link

@cowang4 cowang4 commented Apr 3, 2018

Can we implement Display for NoneError? It would allow the failure crate to automatically derive From<NoneError> for failure::Error. See rust-lang-deprecated/failure#61
It should be a 3 line change, but I'm not sure about the process of RFCs and such.

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Apr 3, 2018

@cowang4 I'd like to try to avoid enabling any more mixing of Result-and-Option right now, as the type is unstable mostly to keep our options open there. I wouldn't be surprised if we ended up changing the design of Try and the desugar into something that didn't need NoneError...

@cowang4
Copy link

@cowang4 cowang4 commented Apr 3, 2018

@scottmcm Okay. I see your point. I'd eventually like a clean way to sugar the pattern of returning Err when an library function returns None. Maybe you know of one other than Try? Example:

fn work_with_optional_types(pb: &PathBuf) -> Result<MyStruct, Error> {
    if let Some(filestem) = pb.file_stem() {
        if let Some(filestr) = filestem.to_str() {
            return Ok(MyStruct {
                filename: filestr.to_string()
            });
        }
     }
    Err(_)
}

Once I found this experimental feature and the failure crate, I naturally gravitated to:

use failure::Error;
fn work_with_optional_types(pb: &PathBuf) -> Result<MyStruct, Error> {
    Ok({
        title: pb.file_stem?.to_str()?.to_string()
    })
}

Which almost works, except for the lack of a impl Display for NoneError like I mentioned before.
But, if this isn't the syntax we'd like to go with, then maybe there could be another function / macro that simplifies the pattern:

if option.is_none() {
    return Err(_);
}

@tmccombs
Copy link
Contributor

@tmccombs tmccombs commented Apr 3, 2018

@cowang4 I believe that would work if you implemented From<NoneError> for failure::Error, which used your own type that implemented Display.

However, it is probably better practice to use opt.ok_or(_)? so you can explicitly say what the error should be if the Option is None. In your example, for instance, you may want a different error if pb.file_stem is None than if to_str() returns None.

@cowang4
Copy link

@cowang4 cowang4 commented Apr 3, 2018

@tmccombs I tried creating my own error type, but I must've done it wrong. It was like this:

#[macro_use] extern crate failure_derive;

#[derive(Fail, Debug)]
#[fail(display = "An error occurred.")]
struct SiteError;

impl From<std::option::NoneError> for SiteError {
    fn from(_err: std::option::NoneError) -> Self {
        SiteError
    }
}

fn build_piece(cur_dir: &PathBuf, piece: &PathBuf) -> Result<Piece, SiteError> {
    let title: String = piece
        .file_stem()?
        .to_str()?
        .to_string();
    Ok(Piece {
        title: title,
        url: piece
            .strip_prefix(cur_dir)?
            .to_str()
            .ok_or(err_msg("tostr"))?
            .to_string(),
    })
}

And then I tried using my error type...

error[E0277]: the trait bound `SiteError: std::convert::From<std::path::StripPrefixError>` is not satisfied
   --> src/main.rs:195:14
    |
195 |           url: piece
    |  ______________^
196 | |             .strip_prefix(cur_dir)?
    | |___________________________________^ the trait `std::convert::From<std::path::StripPrefixError>` is not implemented for `SiteError`
    |
    = help: the following implementations were found:
              <SiteError as std::convert::From<std::option::NoneError>>
    = note: required by `std::convert::From::from`

error[E0277]: the trait bound `SiteError: std::convert::From<failure::Error>` is not satisfied
   --> src/main.rs:195:14
    |
195 |           url: piece
    |  ______________^
196 | |             .strip_prefix(cur_dir)?
197 | |             .to_str()
198 | |             .ok_or(err_msg("tostr"))?
    | |_____________________________________^ the trait `std::convert::From<failure::Error>` is not implemented for `SiteError`
    |
    = help: the following implementations were found:
              <SiteError as std::convert::From<std::option::NoneError>>
    = note: required by `std::convert::From::from`

Okay, I just realized that it wants to know how to convert from other error types to my error, which I can write generically:

impl<E: failure::Fail> From<E> for SiteError {
    fn from(_err: E) -> Self {
        SiteError
    }
}

Nope...

error[E0119]: conflicting implementations of trait `std::convert::From<SiteError>` for type `SiteError`:
   --> src/main.rs:183:1
    |
183 | impl<E: failure::Fail> From<E> for SiteError {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T> std::convert::From<T> for T;

Okay, what about std::error::Error?

impl<E: std::error::Error> From<E> for SiteError {
    fn from(_err: E) -> Self {
        SiteError
    }
}

That doesn't work either. Partly because it conflicts with my From<NoneError>

error[E0119]: conflicting implementations of trait `std::convert::From<std::option::NoneError>` for type `SiteError`:
   --> src/main.rs:181:1
    |
175 | impl From<std::option::NoneError> for SiteError {
    | ----------------------------------------------- first implementation here
...
181 | impl<E: std::error::Error> From<E> for SiteError {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `SiteError`
    |
    = note: upstream crates may add new impl of trait `std::error::Error` for type `std::option::NoneError` in future versions

Which is weird because I thought that NoneError didn't implement std::error::Error. When I comment out my non-generic impl From<NoneError> I get:

error[E0277]: the trait bound `std::option::NoneError: std::error::Error` is not satisfied
   --> src/main.rs:189:25
    |
189 |       let title: String = piece
    |  _________________________^
190 | |         .file_stem()?
    | |_____________________^ the trait `std::error::Error` is not implemented for `std::option::NoneError`
    |
    = note: required because of the requirements on the impl of `std::convert::From<std::option::NoneError>` for `SiteError`
    = note: required by `std::convert::From::from`

Do I have to write all the Froms manually. I though that the failure crate was supposed to derive them?

Maybe I should stick with option.ok_or()

@tmccombs
Copy link
Contributor

@tmccombs tmccombs commented Apr 4, 2018

Do I have to write all the Froms manually. I though that the failure crate was supposed to derive them?

I don't think the failure crate does that. But I could be wrong.

@cowang4
Copy link

@cowang4 cowang4 commented Apr 4, 2018

Ok, so I re-examined the failure crate, and if I'm reading the documentation and different versions right, it's designed to always use the failure::Error as the Error type in your Results, see here. And, it does implement a blanket impl Fail trait for most Error types:

impl<E: StdError + Send + Sync + 'static> Fail for E {}

https://github.com/rust-lang-nursery/failure/blob/master/failure-1.X/src/lib.rs#L218

And then a impl From so that it can Try/? other errors (like ones from std) into the overarching failure::Error type.

impl<F: Fail> From<F> for ErrorImpl

https://github.com/rust-lang-nursery/failure/blob/d60e750fa0165e9c5779454f47a6ce5b3aa426a3/failure-1.X/src/error/error_impl.rs#L16

But, it's just that since the error relevant to this rust issue, NoneError, is experimental, it can't be converted automatically yet, because it doesn't implement the Display trait. And, we don't want it to, because that blurs the line between Options and Results more. It'll all probably get re-worked and sorted out eventually, but for now, I'll stick to the de-sugared techniques that I've learned.

Thank you everyone for helping. I'm slowly learning Rust! 😄

@calebsander
Copy link
Contributor

@calebsander calebsander commented Sep 2, 2020

I'm not sure if anyone have already mentioned this, shall we impl Try for bool, maybe Try<Ok=(), Error=FalseError>?

This would make Try behave as the && operator on bools. As others have pointed out above, there are also use cases for short-circuiting on success, in which case Try should behave as ||. Since the && and || operators aren't much longer to type out, I also don't see much of an advantage to having this implementation.

@clouds56
Copy link

@clouds56 clouds56 commented Sep 2, 2020

@calebsander thanks for kindly reply.
That's true for some simple cases, but I don't think it is often the case, especially we could never have assignment statment like let x = v.get_mut(key)? in expression.
If && and || would always do the trick, we could as well play with .unwrap_or_else(), .and_then() for Option and Error cases.
Could you express the flowing code in && and ||?

fn check_key(v: Vec<A>, key: usize) -> bool {
  let x = v.get_mut(key)?; // Option
  x.not_valid().not()?; // bool
  for i in x.iter() {
    if i == 1 { return true }
    if i == 2 { return false }
    debug!("get {}" i);
  }
  let y = x.transform()?; // Result
  y == 1
}

For some condition that true means failed while false means success, !expr? might be confusing, but we could use expr.not()? to do the trick (note: for ops::Try, we always have false for Error)

@calebsander
Copy link
Contributor

@calebsander calebsander commented Sep 2, 2020

That's true for some simple cases, but I don't think it is often the case, especially we could never have assignment statment like let x = v.get_mut(key)? in expression.

Well, unless I misunderstand your proposal, just implementing Try<Ok = (), Error = FalseError> for bool wouldn't allow this. You would also need to impl From<NoneError> for FalseError so that the ? operator could convert None into false. (And similarly, if you want to apply ? to a Result<T, E> inside a function that returns bool, you would need to impl From<E> for FalseError. I think such a blanket implementation would be problematic.) You could also just use some_option().ok_or(false)? and some_result().map_err(|_| false)? if you're okay with the return value Result<bool, bool> (which you can collapse with .unwrap_or_else(|err| err)).

Leaving issues of converting other Try errors to bool aside, the Try implementation you are proposing for bool is just the && operator. For example, these are equivalent

fn using_try() -> bool {
    some_bool()?;
    some_bool()?;
    some_bool()
}

and

fn using_and() -> bool {
    some_bool() &&
    some_bool() &&
    some_bool()
}

(Admittedly, control flows like if and loop which return non-() are less straightforward to translate.)

For some condition that true means failed while false means success, !expr? might be confusing, but we could use expr.not()? to do the trick (note: for ops::Try, we always have false for Error)

It's not clear to me that false should represent the Error case. (For example, Iterator.all() would want false to short-circuit, but Iterator::any() would want true instead.) As you pointed out, you can get the opposite short-circuiting behavior by inverting the value passed to ? (and inverting the function's return value as well). But I don't think that results in very readable code. It might make more sense to have separate structs And(bool) and Or(bool) that implement the two different behaviors.

@clouds56
Copy link

@clouds56 clouds56 commented Sep 4, 2020

And similarly, if you want to apply ? to a Result<T, E> inside a function that returns bool, you would need to impl From for FalseError. I think such a blanket implementation would be problematic.

No, I don't want to impl From<T> for FalseError, maybe we could do result.ok()?

It's not clear to me that false should represent the Error case.

I think it somehow natural when we have bool::then that map true to Some.

@PvdBerg1998
Copy link

@PvdBerg1998 PvdBerg1998 commented Oct 3, 2020

Forgive me if I missed it - why doesnt NoneError impl std::error::Error? This makes it useless for any functions returning Box<dyn Error> or similar.

@ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Oct 3, 2020

Forgive me if I missed it - why doesnt NoneError impl std::error::Error? This makes it useless for any functions returning Box<dyn Error> or similar.

Not an expert here, but I can see significant problems with trying to come up with a useful error message for "something was None and you expected it to be Some" (which is basically what you'd be gaining here -- some diagnostic message). In my experience, it has always made most sense to use the Option::ok_or{,_else} combinator to use another error type instead, because as the calling code I generally have much better context to give anyways.

EDIT: Option::ok_or

@MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Oct 3, 2020

I agree with @ErichDonGubler. It's super annoying to be unable to use ? with Option, however it's for a good reason, and, as a language user, I think it's in everyone's best interest to handle errors properly. Doing this extra work of binding error context to None instead of just doing ? is really, really annoying, but it forces properly communicating errors, which is well worth it.

The only time I'd allow None be interpreted as an error is at very rough prototyping. I figured, if we add something like a Option::todo_err() call, that would return Ok of the internal value, but will panic on None. It is very counter-intuitive until you analyze the needs of the "rough prototyping" mode of code authoring. It's very similar to Ok(my_option.unwrap()), but doesn't need an Ok(...) wrapping. In addition, it explicitly specifies the "todo" nature, thus communicating the intent to remove this code from the production logic, replacing it with a proper error binding.

@thomcc
Copy link
Contributor

@thomcc thomcc commented Oct 3, 2020

but will panic on None

Strongly feel that we should just leave this to unwrap. If we added a todo_err it should return an actual error type, which begs the question of what error type to return.

Also I suspect fn todo_err(self) -> Result<Self, !> would have the obvious problem of requiring ! to stabilize, which uh, well, Someday.

@PvdBerg1998
Copy link

@PvdBerg1998 PvdBerg1998 commented Oct 4, 2020

My usecase is indeed rough prototyping where I don't care about the errors too much. Doesn't the whole NoneError suffer from these problems you listed? If it is decided that it should exist (which I think is a good thing, at least for prototyping), I believe it should impl Error as it is named to be one.

Because of the type lacking this impl I resorted to just slapping a .ok_or("error msg") on it, which works also but is less convenient.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 13, 2020
…ences, r=<try>

Use `try{}` in `try_fold` to decouple iterators in the library from `Try` details

I'd like to experiment with changing the `?`/`try` desugaring and correspondingly the `Try` trait (see rust-lang#42327 for discussions about the suboptimalities of the current one) and this change would keep from needing any `cfg(bootstrap)` in iterator things.

This will be lowered to the same thing, so shouldn't cause any perf issues:
https://github.com/rust-lang/rust/blob/08e2d4616613716362b4b49980ff303f2b9ae654/compiler/rustc_ast_lowering/src/expr.rs#L428-L429

But I'll trigger a perf run just in case.

~~EDIT: changed to a draft because of the rustfmt-only syntax error.  zulip thread about it: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/New.20bootstrap.20rustfmt.20doesn't.20support.20syntax.20from.20sept.3F/near/213098097~~

EDIT: This now includes a rustfmt version bump to get through tidy.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 19, 2020
…erences, r=shepmaster

Use `try{}` in `try_fold` to decouple iterators in the library from `Try` details

I'd like to experiment with changing the `?`/`try` desugaring and correspondingly the `Try` trait (see rust-lang#42327 for discussions about the suboptimalities of the current one) and this change would keep from needing any `cfg(bootstrap)` in iterator things.

This will be lowered to the same thing, so shouldn't cause any perf issues:
https://github.com/rust-lang/rust/blob/08e2d4616613716362b4b49980ff303f2b9ae654/compiler/rustc_ast_lowering/src/expr.rs#L428-L429

But ~~I'll trigger~~ I've triggered [a perf run](https://perf.rust-lang.org/compare.html?start=d65c08e9cc164b7b44de53503fae859a4fafd976&end=2c067c5235e779cd75e9f0cdfe572c64f1a12b9b) just in case.

~~EDIT: changed to a draft because of the rustfmt-only syntax error.  zulip thread about it: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/New.20bootstrap.20rustfmt.20doesn't.20support.20syntax.20from.20sept.3F/near/213098097~~

EDIT: This now includes a rustfmt version bump to get through tidy.
@smmalis37
Copy link
Contributor

@smmalis37 smmalis37 commented Dec 1, 2020

Being unable to cast a NoneError to a dyn std::error:Error definitely feels like an ergonomics roadbump to me. I get that there's no great message that could be put in it, but if I'm just throwing everything into a trait object I probably don't care too much about the details of each error to begin with. It'd be really nice for prototype code to be able to return Result<Foo, Box<dyn Error>> everywhere and have ? work with Option. Given that I am explicitly declaring that I don't care much about the errors right now, having to call .ok_or or some other adapter every time would really get in the way of the "quick prototyping" ideal.

@thomcc
Copy link
Contributor

@thomcc thomcc commented Dec 1, 2020

Being unable to cast a NoneError to a dyn std::error:Error definitely feels like an ergonomics roadbump to me

I don't think this is being debated, so much as it locking us into the current (clearly suboptimal) design in some ways.

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Jan 10, 2021

For anyone following along here: I've posted a new RFC with an alternative Try/? design at rust-lang/rfcs#3058

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Apr 17, 2021

@yaahc I was going through the checkboxes here, and wanted to check whether you'd be fine saying that the one referencing your comment #42327 (comment) is resolved by the new RFC.

@yaahc
Copy link
Member

@yaahc yaahc commented Apr 19, 2021

@yaahc I was going through the checkboxes here, and wanted to check whether you'd be fine saying that the one referencing your comment #42327 (comment) is resolved by the new RFC.

Definitely 👍

@bstrie
Copy link
Contributor

@bstrie bstrie commented Apr 20, 2021

Now that the Try v2 RFC has been accepted, shall we close this in favor of #84277 ?

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Apr 20, 2021

@bstrie There's still #[unstable] attributed in-tree referencing this, so I think this should stay open until those items have all been deleted. I added a checkbox to the OP for that.

@scottmcm
Copy link
Member Author

@scottmcm scottmcm commented Sep 1, 2021

Try v1 has been completely removed from master (https://github.com/rust-lang/rust/search?q=42327&type=code).

@scottmcm scottmcm closed this Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet