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 Result::flatten (result_flattening) #70142

Open
1 of 3 tasks
Nemo157 opened this issue Mar 19, 2020 · 23 comments
Open
1 of 3 tasks

Tracking Issue for Result::flatten (result_flattening) #70142

Nemo157 opened this issue Mar 19, 2020 · 23 comments
Labels
A-result-option B-unstable C-tracking-issue Libs-Small Libs-Tracked T-libs-api

Comments

@Nemo157
Copy link
Member

Nemo157 commented Mar 19, 2020

This is a tracking issue for the Result::flatten API.
The feature gate for the issue is #![feature(result_flattening)].

impl<T, E> Result<Result<T, E>, E> {
      pub fn flatten(self) -> Result<T, E>;
}

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

@Nemo157 Nemo157 added the C-tracking-issue label Mar 19, 2020
@Centril Centril added B-unstable T-libs-api labels Mar 19, 2020
Centril added a commit to Centril/rust that referenced this issue Mar 29, 2020
Add Result<Result<T, E>, E>::flatten -> Result<T, E>

This PR makes this possible (modulo type inference):

```rust
assert_eq!(Ok(6), Ok(Ok(6)).flatten());
```

Tracking issue: rust-lang#70142

<sub>largely cribbed directly from <https://github.com/rust-lang/rust/pull/60256></sub>
@KodrAus KodrAus added A-result-option Libs-Small Libs-Tracked labels Jul 29, 2020
@josephlr
Copy link
Contributor

josephlr commented Sep 12, 2020

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

Diggsey commented Sep 28, 2020

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

de-vri-es commented Oct 7, 2020

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

yoshuawuyts commented Oct 12, 2020

In #70140 (comment) @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 #70140 (comment) 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
Copy link
Member Author

Nemo157 commented Oct 12, 2020

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

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Oct 12, 2020

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:

  1. Should the existence of Result::flatten_into forbid Result::flatten from having an Into bound? I would assume not?
  2. To which degree do flatten and flatten_into overlap? Assuming we can declare the default of fn flatten_into<E = E1>, having both seems redundant?

@Diggsey
Copy link
Contributor

Diggsey commented Oct 12, 2020

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

de-vri-es commented Oct 12, 2020

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.

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

Nemo157 commented Oct 12, 2020

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

Elrendio commented Dec 23, 2020

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.

Recap

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),
		}
	}
}

Explanation

Context

I work at Stockly, a startup doing mostly web development and to better manage errors we strongly differentiate two types of error:

  • Internal Errors: when the code that triggered this error requires some changes. For example, you do a complex algorithm and an invariant, that should have held, doesn't hold at the end.
  • Fails: when the code worked as expected but couldn't perform the operation. This is mostly due to an invalid input, as an empty string given when a non empty one was required, or the system being in a invalid state, as trying to delete a file that doesn't exists.

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:

  • Caught to do alternative computation,
  • Returned to the client to print an explanation message to the user
  • Turned into internal errors (rare case, for example when your code just checked that a file existed and wants to delete it you should convert the fail 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:

  • It's the use case I've seen most often
  • Since ? 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 !

Side Notes

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

Fishrock123 commented Aug 11, 2021

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.

  • flatten to inner - async_std::future::timeout's error is just a marker type which is effectively (), and the inner error type is preferable.
  • flatten to outer - @Elrendio's case above.

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

ajmcmiddlin commented Nov 15, 2021

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

Diggsey commented Nov 16, 2021

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

ajmcmiddlin commented Nov 16, 2021

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

boringcactus commented Dec 5, 2021

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

thomaseizinger commented Dec 5, 2021

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

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

Diggsey commented Dec 5, 2021

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

thomaseizinger commented Dec 5, 2021

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

SuperFluffy commented Jan 27, 2022

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

sollyucko commented Jan 27, 2022

@SuperFluffy

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?

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.

@Mathspy
Copy link

Mathspy commented Jan 27, 2022

@SuperFluffy your use case might be satisfied with or_else

@scottmcm
Copy link
Member

scottmcm commented Mar 18, 2022

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 it's even simpler than that, thanks to #70941 :

let flattened_result = try { r?? };

(Coincidentally exactly the same number of characters as r.flatten().)

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Mar 18, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-result-option B-unstable C-tracking-issue Libs-Small Libs-Tracked T-libs-api
Projects
None yet
Development

No branches or pull requests