Open Nemo157 opened 4 years ago
josephlr
commented
4 years ago With the stabilization of #49146, this function can now have a trivial const fn implementation using match. Should this function just be directly stabilized as a const fn?
Diggsey
commented
3 years ago I think it would also be useful to have a flatten_into method that would also allow conversion of the error type.
de-vri-es
commented
3 years ago It could also be
impl<T, E1, E2> for Result<Result<T, E1>, E2>
where
E1: From<E2>,
{
fn flatten(self) -> Result<T, E1> {
self?
}
}But: which error type do you convert into the other?
Keeping the inner error type seemed more logical to me, under the assumption that the inner thing is what you really want, and the outer result represents further processing.
An example where that occurs is using tokio::time::timeout to add a timeout to I/O operations. The result has an Elapsed error that can be converted into an std::io::Error, but not vice-versa.
yoshuawuyts
commented
3 years ago In https://github.com/rust-lang/rust/pull/70140#issuecomment-602507375 @CryZe brought up:
Should this possibly also convert the error type like the try operator does?
This seems to touch on the same point as @Diggsey's and @de-vri-es's comments on converting error types. And it's easy to see why; the ? operator just works when one error can be converted into another, and it's not unreasonable to expect other methods that deal with converting error types to work much the same.
The only argument against this so far has been https://github.com/rust-lang/rust/pull/70140#issuecomment-605614716 which talks about monadic similarities between Option and Result. I don't find that argument particularly appealing, as it centers a subjective theoretical view on what I think is an ergonomics question. I think it's far preferable for Rust to feel internally consistent to over consistency with a monadic model.
I think the implementation of @de-vri-es looks right. I intend to open a PR shortly to implement this
Nemo157
commented
3 years ago I don't know that E1: From<E2> is the correct order, my instinct would have been the inverse. It seems like getting some examples of usecases would be good to see how each order affects it?
(Though personally I don't think the base .flatten() method should do conversion, and having a separate .flatten_into() might be a good approach).
One example for when you have two incompatible error types, and you have to convert into a third error type:
// with `E1: From<E2>`
result.map(|res| res.map_err(anyhow::Error::from)).flatten_into()?;
// with `E2: From<E1>`
result.map_err(anyhow::Error::from).flatten_into()?;For maximum ergonomics, with improved defaults and ? type inferencing it could even be something like
impl<T, E1, E2> for Result<Result<T, E1>, E2> {
fn flatten_into<E = E1/E2>(self) -> Result<T, E> where E: From<E2>, E: From<E1> {
Ok(self??)
}
}which would allow converting two incompatible error types into a final error type that they are both compatible with
fn foo() -> Result<Result<(), impl Error>, impl Error> { ... }
fn foobar() -> anyhow::Result<()> {
foo().flatten_into()?
}I don't know that E1: From
is the correct order, my instinct would have been the inverse. It seems like getting some examples of usecases would be good to see how each order affects it?
Oh yeah good one. I guess the one I had in mind was the one @de-vri-es mentioned with an async timeout method. For example in async-std the API would be the following (ref):
use async_std::{fs, future};
use std::time::Duration;
let dur = Duration::from_millis(100);
let s = fs::read_to_string("./my.db").timeout(dur).await??;The return type here is
Result<Result<String, io::Error>, future::Timeout>
Result<Result<String, E1>, E2>We'd want to convert from future::Timeout to io::Error, or convert E2 into E1 -- which the implementation does. But I'd be curious to know if there are any practical counter-examples.
I think you're asking an intriguing question: what if both errors could somehow converge into some third error type? You suggest using flatten_into for this; but that raises two questions:
Result::flatten_into forbid Result::flatten from having an Into bound? I would assume not?flatten and flatten_into overlap? Assuming we can declare the default of fn flatten_into<E = E1>, having both seems redundant?Diggsey
commented
3 years ago FWIW, the specific case where I encountered this requires the full flatten_into version where a 3rd error type is specified.
That said, I think there is value in both, as flatten_into will usually require explicit type annotations, whereas the variations of flatten should for the most part be able to infer their types, and so will be more ergonomic where it is possible to use them.
de-vri-es
commented
3 years ago That said, I think there is value in both, as
flatten_intowill usually require explicit type annotations, whereas the variations offlattenshould for the most part be able to infer their types, and so will be more ergonomic where it is possible to use them.
Agreed.
The E1 vs E2 issue might be enough reason to only support a single error type without conversion for flatten(). Or have a flatten_inner() and flatten_outer(), but maybe that is a bit overkill.
/edit: Also, you can't have defaults for generic type arguments for functions, right? So that won't be a solution (see also #36887).
Nemo157
commented
3 years ago Also, you can't have defaults for generic type arguments for functions, right?
Yeah, also ? doesn't (currently) influence type inference. The second half of my comment was very "in the far future this could be possible". (And with good generic defaults and ? inferencing support I think it could get away without explicit type annotations in the majority of cases).
Elrendio
commented
3 years ago Hello everyone !
It seems I'm a bit late to the debate but the warning on the use of flatten was published in rust 1.48.0, aka one month ago.
I believe flatten should convert the error with bound E2: From<E1> and here's my implementation proposal:
impl<V, E, F> Result<Result<V, F>, E> {
fn flatten(self) -> Result<V, E>
where
F: Into<E>,
{
self.flatten_with(|e| e.into())
}
fn flatten_with<O: FnOnce(F) -> E>(self, op: O) -> Result<V, E> {
match self {
Ok(Ok(v)) => Ok(v),
Ok(Err(f)) => Err(op(f)),
Err(e) => Err(e),
}
}
}I work at Stockly, a startup doing mostly web development and to better manage errors we strongly differentiate two types of error:
Most of our functions return types like Result<Result<V, F>, E>, with V being an expected return value, F a fail and E an internal error. The primary concrete difference between the fail and the error is that the error computes a lengthy context and a stacktrace. Internal Errors are meant to be reported with a maximum of context to help developpers understand the bug and fix the code as fast as possible. We compute stack traces and send all the appropriate contextual data. Fails can be either:
the file doesn't exist into an internal error).We always use the same InternalError type:
/// Wrapper around `failure::Error` with support for additional json context
#[derive(Deref, DerefMut)]
pub struct InternalError {
inner: Box<InternalErrorInner>,
}
pub struct InternalErrorInner {
pub err: failure::Error,
pub context: BTreeMap<String, serde_json::Value>,
#[cfg(feature = "sentry")]
pub patch_event: Option<Box<dyn Fn(&mut sentry_helpers::sentry::protocol::Event) + Send + Sync>>,
_private: (),
}Fails are often enums and always context specific. For example, working in e-commerce, when a consumer wants to cancel an order, the function cancelling it returns Result<Result<CancellationId, Fail>, InternalError> where Fail is:
pub enum Fail {
NotFound,
AlreadyCancelled,
DelayExpired,
}Due to the frequency of using results of results, we've wrote a helper trait called Result2 with functions that cover most of our needs, I've provided it at the end. This philosophy works really well on our code base of approx 150K lines of rust. It might definitely not be representative of the need of the rust community.
I think the signature Result<Result<T, E>, E> -> Result<T, E> is not best idea because I don't the think the use case of a func returning a Result<Result<T, E>, E> happens quite often. What advantage would be in splitting the error in two levels if they're the same type? If it provides no advantages and is purely the result of how the code was written, I believe the best would be to flatten the result directly in the function with the operator ?. Does rust has anyway of measuring how often someone did that in the crates of crates.io ?
As to which error type should be converted, I think E2: From<E1> is better because:
? returns an error of the first level, I believe most of the time the first level of error is the general error of the crate and the second is more specific to the context. Therefore E2: From<E1> would be more frequent than E1: From<E2>. When we need the other way around, we use .transpose().flatten() (see side notes).For the use case of converting to a third error type, I believe most of the time you can replace result.map_err(anyhow::Error::from).flatten_into()?; by Ok(result??) which is really easy to write and therefore the benefits of flatten are lowered. Also, I'm afraid that the proposed implementation might be complicated to use due to requiring explicit type annotations.
I can write a RFC & implementation for flatten, flatten_with and transpose if consensus is reached 🙂
Happy holidays !
Here's our complete trait Result2:
impl<V, E, F> Result2 for Result<Result<V, F>, E> {
type V = V;
type E = E;
type F = F;
fn and_then2<V2, O>(self, op: O) -> Result<Result<V2, Self::F>, Self::E>
where
O: FnOnce(Self::V) -> Result<Result<V2, Self::F>, Self::E>,
{
match self {
Ok(Ok(val)) => op(val),
Ok(Err(fail)) => Ok(Err(fail)),
Err(err) => Err(err),
}
}
fn flatten(self) -> Result<Self::V, Self::E>
where
Self::F: Into<Self::E>,
{
self.flatten_with(|e| e.into())
}
fn flatten_with<O: FnOnce(Self::F) -> Self::E>(self, op: O) -> Result<Self::V, Self::E> {
match self {
Ok(Ok(v)) => Ok(v),
Ok(Err(f)) => Err(op(f)),
Err(e) => Err(e),
}
}
fn map2<V2, O: FnOnce(Self::V) -> V2>(self, op: O) -> Result<Result<V2, F>, Self::E> {
match self {
Ok(r) => Ok(r.map(op)),
Err(e) => Err(e),
}
}
fn map_err2<F2, O: FnOnce(Self::F) -> F2>(self, op: O) -> Result<Result<Self::V, F2>, Self::E> {
match self {
Ok(r) => Ok(r.map_err(op)),
Err(e) => Err(e),
}
}
fn transpose(self) -> Result<Result<Self::V, Self::E>, Self::F> {
match self {
Ok(Ok(val)) => Ok(Ok(val)),
Ok(Err(fail)) => Err(fail),
Err(err) => Ok(Err(err)),
}
}
}
Fishrock123
commented
3 years ago Having just been asked if ?? was a mistake when I wrote it the other day, this code:
async_std::future::timeout(timeout, conn).await??I wonder if ?? won't be hard to understand the implications of.
This is, naturally a good spot for result flattening.
My intuition says that Result::flatten() should probably do |v| Ok(v??) - but I am not sure that is correct, because rustc may have issues telling what the return type ought to be. Also consider that a lot of cases where you'd wrote .flatten() you'd probably follow it with a ? like so: .flatten()?. This is then guaranteed to produce an unknowable intermediate type, and would require annotations, making it also certainly worse than just writing Ok(v??). I think that any form of flatten_into() probably has this issue and is probably not a huge priority, but I suppose a theoretical flatten_into() could probably be |v| Ok(v??) if we're just giving in to having that require some kind of type knowledge in every case.
This leaves a flatten on the table which is sorta like self?, but I can think of use-cases for converting both ways.
async_std::future::timeout's error is just a marker type which is effectively (), and the inner error type is preferable.I suspect that flatten-to-inner is more common from libraries and flatten-to-outer is more common in applications.
So I wonder if we don't actually want two:
Result::flatten_in()Result::flatten_out()And uh, maybe it is worth taking Result::flatten() though an RFC process since it seems clear that it is less than straight-forward? Not sure.
Edit: and, which ever the case, flatten_into() seems less needed as noted above, since that's just .flatten()?. (As I understand it.)
ajmcmiddlin
commented
2 years ago The E1 vs E2 issue might be enough reason to only support a single error type without conversion for flatten()
I am strongly in favour of this position. I argue that the ambiguity of implicitly converting errors in flatten will introduce issues (confusion, errors in user code, unnecessary complexity) whose cost significantly outweighs the perceived benefits. Especially when such low-cost alternatives exist. I am also concerned that the API would be opting users into conversions, such that avoiding them would require not using the API.
My preference would be to only have flatten : Result<Result<T, E>, E> -> Result<T, E> and suggest people use map_err or some other means to explicitly handle conversion of the error type.
Diggsey
commented
2 years ago @ajmcmiddlin
My preference would be to only have flatten : Result<Result<T, E>, E> -> Result<T, E> and suggest people use map_err or some other means to explicitly handle conversion of the error type.
The problem with that workaround is if you need to convert the inner error type. You end up with r.map(|r| r.map_err(Into::into)).flatten(), or even r.map(|r| r.map_err(Into::into)).map_err(Into::into).flatten() if you need to convert both types, which is quite a mouthful!
For that reason, although I agree that plain flatten should do no conversion, I think a flatten_into is definitely worthwhile, as it takes an unreadable 62 character conversion involving doubly nested closures, into a simple 16 character method call.
ajmcmiddlin
commented
2 years ago @Diggsey
I think a flatten_into is definitely worthwhile
I have no qualms with additional methods. I realise my wording suggested I would be against methods that did implicit conversions, and while I might not personally use them, I'm not inclined to argue with their inclusion.
boringcactus
commented
2 years ago I think flatten having an inner-error vs outer-error type preference will absolutely catch some people by surprise when they wanted the other one, and so I'm in favor of
impl<T, E> Result<Result<T, E>, E> {
pub fn flatten(self) -> Result<T, E>;
}
impl<T, E1, E2> Result<Result<T, E1>, E2> {
pub fn flatten_into<E>(self) -> Result<T, E> where E1: Into<E>, E2: Into<E>;
}matching the flatten API as already implemented.
thomaseizinger
commented
2 years ago @Diggsey
My preference would be to only have flatten : Result<Result<T, E>, E> -> Result<T, E> and suggest people use map_err or some other means to explicitly handle conversion of the error type.
The problem with that workaround is if you need to convert the inner error type. You end up with
r.map(|r| r.map_err(Into::into)).flatten(), or evenr.map(|r| r.map_err(Into::into)).map_err(Into::into).flatten()if you need to convert both types, which is quite a mouthful!
If both errors need converting, ? should do the trick no?
Assuming r is Result<T, Result<T, E1>, E2>, Ok(r??) will give you Result<T, E3> where E3 can either be E1, E2 or a completely different Error type that both E1 and E2 convert into.
If we ever get try blocks, you don't need a dedicated function either and one could write:
let flattened_result = try { Ok(r??) };Note that with async blocks, this is already possible today:
let future_with_flattened_result = async {
let dur = Duration::from_millis(100);
let s = fs::read_to_string("./my.db").timeout(dur).await??;
Ok(s)
};And it is possible to (ab)-use closures for this:
let flattened_result = (|| anyhow::Ok(r??))();To aid with type-inference, anyhow for example recently added anyhow::Ok.
Given these - relatively lightweight - alternatives, my vote would also go down for only adding flatten without error conversions.
Diggsey
commented
2 years ago If both errors need converting, ? should do the trick no?
That's only usable if you want to immediately return the error. If you just want to get back a Result<T, E3> where E3: From<E1> + From<E2> then you can't use that.
thomaseizinger
commented
2 years ago If both errors need converting, ? should do the trick no?
That's only usable if you want to immediately return the error. If you just want to get back a
Result<T, E3> where E3: From<E1> + From<E2>then you can't use that.
On stable Rust, you can emulate try blocks with closures so you can get back a Result right away without extracting another function or returning immediately from your current function.
SuperFluffy
commented
2 years ago I was looking for something like flatten_err and found this tracking issue. I have a function fn f<U>(data: T) -> Result<U, T> that returns data in error position. The idea is to be able to redo a different kind of calculation on data and only bail if that also didn't work. So I might end up with something like Result<U, Result<U, E>> or even Result<U, Result<U, Result<U, E>>>.
Flattening over the errors would be very useful!
impl<T, E> Result<T, Result<T, E>> {
pub fn flatten(self) -> Result<T, E>;
}I guess inclusion in this issue would be too late? Should I open a follow-up?
sollyucko
commented
2 years ago @SuperFluffy
I was looking for something like
flatten_errand found this tracking issue. I have a functionfn f<U>(data: T) -> Result<U, T>that returnsdatain error position. The idea is to be able to redo a different kind of calculation ondataand only bail if that also didn't work. So I might end up with something likeResult<U, Result<U, E>>or evenResult<U, Result<U, Result<U, E>>>.Flattening over the errors would be very useful!
impl<T, E> Result<T, Result<T, E>> { pub fn flatten(self) -> Result<T, E>; }I guess inclusion in this issue would be too late? Should I open a follow-up?
FWIW, it seems like ControlFlow would better reflect your semantics, though that currently doesn't have a flatten method either. There, both flatten_break and flatten_continue seem likely to be useful.
scottmcm
commented
2 years ago If we ever get
tryblocks, you don't need a dedicated function either and one could write:let flattened_result = try { Ok(r??) };
Note that it's even simpler than that, thanks to #70941 :
Mathspylet flattened_result = try { r?? };(Coincidentally exactly the same number of characters as r.flatten().)
yoshuawuyts
commented
2 years ago @scottmcm I believe flatten is useful even if we have try {} blocks because it can be chained. Much like why having a Future::map call is useful, even now that we have async {} blocks. Let me explain with an example:
with Result::flatten
// don't worry too much about these imports, they provide things like concurrency and timeouts
use async_time::prelude::*;
use async_concurrency::prelude::*;
use async_std::fs;
let fut1 = fs::read_to_string("some-file.txt")
.timeout(Duration::from_secs(2))
.flatten(); // flatten `io::Result<io::Result<String>>` to `io::Result<String>`
// This returns `io::Result<String>`
let fut2 = fs::read_to_string("other-file.txt");
// Because both futures now have the same signature, we can `race` them.
let s = (fut1, fut2).race().await?;without Result::flatten
// don't worry too much about these imports, they provide things like concurrency and timeouts
use async_time::prelude::*;
use async_concurrency::prelude::*;
use async_std::fs;
// flatten `io::Result<io::Result<String>>` to `io::Result<String>`
let fut1 = try {
fs::read_to_string("some-file.txt")
.timeout(Duration::from_secs(2))??
};
// This returns `io::Result<String>`
let fut2 = fs::read_to_string("other-file.txt");
// Because both futures now have the same signature, we can `race` them.
let s = (fut1, fut2).race().await?;Imo both approaches compliment each other, and which is the better choice will depend on the situation.
RedIODev
commented
1 year ago I think flatten having an inner-error vs outer-error type preference will absolutely catch some people by surprise when they wanted the other one, and so I'm in favor of
impl<T, E> Result<Result<T, E>, E> { pub fn flatten(self) -> Result<T, E>; } impl<T, E1, E2> Result<Result<T, E1>, E2> { pub fn flatten_into<E>(self) -> Result<T, E> where E1: Into<E>, E2: Into<E>; }matching the
flattenAPI as already implemented.
I just wrote flatten_into as a 5 sec hack when I discovered flatten and I ended up writing map_err in a nested map and my nice and clean function chain looked worse than without flatten. I independently wrote char for char the same function. I think this shows that flatten_into is an obvious step when adding flatten.
piegamesde
commented
1 year ago 
estk
commented
1 month ago @Nemo157 is it just docs and a stabilization PR blocking this?
This is a tracking issue for the
Result::flattenAPI. The feature gate for the issue is#![feature(result_flattening)].About tracking issues
Tracking issues are used to record the overall progress of implementation. They are also uses 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
Implementation history