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

RFC: Add method Result::into_ok #2799

Open
wants to merge 6 commits into
base: master
from

Conversation

@mzabaluev
Copy link
Contributor

mzabaluev commented Nov 2, 2019

Add method Result::unwrap_infallible Result::into_ok to provide a convenient alternative to unwrap for converting Result values with an uninhabitable Err type, while ensuring infallibility at compile time.

Rendered

text/0000-unwrap-infallible.md Outdated Show resolved Hide resolved
@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 2, 2019

This is a small enough change that you could just send a PR and get it done on nightly.

Co-Authored-By: kennytm <kennytm@gmail.com>
@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 2, 2019

@KrishnaSannasi I've got it lined up, but the collaboration guidance suggests going through an RFC when introducing API additions. It might yet get bogged down in subcommittees, two parties polarized around unwrap_always vs. unwrap_infallible will debate it for days, this kind of thing.

Found a pre-existing Rust issue which proposed nearly exactly
what I came up with.
mzabaluev added a commit to mzabaluev/rust that referenced this pull request Nov 2, 2019
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 4, 2019

This is a minor nit, and I really don't want to bikeshed this. But I was very much hoping that with the stabilization of ! it would no longer be necessary to write the word infallible. 😄

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 4, 2019

@glaebhoerl into_inner typically applies where there is one main encapsulated thing, while with Result one could still logically try to examine the content of Err and have an unreachable branch on it, so it does not seem so clear cut to me.

I'd like the method name to convey the intent of assuring statically the infallibility that may not be available on the generic Result. Also, when the error becomes inhabitable, it can be as simple as s/unwrap_infallible/unwrap/ to refactor a whole affected module, while with into_inner you'd have to review or unbreak the call sites where it's something else, or even risk inadvertently changing that to unwrap which would compile, but mean something different.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 4, 2019

But I was very much hoping that with the stabilization of ! it would no longer be necessary to write the word infallible.

I'm open to a name devoid of uncommon adjectives, like unwrap_always (as a counterpoint to the never-error).

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 4, 2019

To make it really short, but still distinctive, it can be named into_ok. As pointed out by @glaebhoerl, the infallibility connotation of into would convey the intent well enough.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 4, 2019

Bikeshedding in the RFC issue has been most useful.
@mzabaluev mzabaluev changed the title RFC: Result::unwrap_infallible RFC: Add method Result::into_ok Nov 4, 2019
@remram44

This comment has been minimized.

Copy link

remram44 commented Nov 5, 2019

I think it might be better to call it something that doesn't start with unwrap. I often find myself grepping my code for \bunwrap to find places with possible panics (unwrap, unwrap_err, unwrap_none) and this is not one of them. On the other hand there are already matching methods that don't panic (unwrap_or, unwrap_or_else) so I guess that ship might have sailed 😅

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 5, 2019

To me unwrap-ish style makes more sense. Result wraps the T value with Ok(T), and the unwrapping is extracting that value. It has nothing to do with panics and it already implies we're dealing with Ok variant (though we have unwrap_err and so on - but it only proves my point).

I also like From impl for Result<T, !> - it makes a lot of sense, and it's ergonomic in a sense it's easy to combine with fns that take Into<T>.

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 5, 2019

This is a minor nit, and I really don't want to bikeshed this. But I was very much hoping that with the stabilization of ! it would no longer be necessary to write the word infallible. 😄

We'd still have to refer to it somehow. infallible or something else - we should have a proper name for this thing. The alternative is using "the exclamation type thing" (derived from syntax) - and I like it even less.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 5, 2019

@MOZGIII

I also like From impl for Result<T, !> - it makes a lot of sense, and it's ergonomic in a sense it's easy to combine with fns that take Into<T>.

Did you mean impl From<Result<T, !>> for T? The RFC explains in the alternatives section that it is not feasible, because it may overlap with impls defined for specific types in other crates and therefore would be a breaking change. Also, see the discussion above regarding the need to make the method name distinct to facilitate refactoring.

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 5, 2019

Yes, I meant impl From<Result<T, !>> for T. I would argue that, despite it's a breaking change, it's kind of an acceptable one. Let's go in-depth and see how bad would this breaking change be? That said, I agree that this may be a problem big enough for us to disregard this approach.

As mentioned above already, from the refactoring facilitation point of view, unwrap.* is already not a clear marker for panics. Yet, the semantics of what we're doing here is sort of exactly unwrap. I would value deviating from the unwrap naming pattern as doing more bad than a name being more panic-hint-friendly. I agree that there is a problem, kind of, with the unwrap and panics when doing refactoring, but that problem is better solved separately - and for all affected calls. Also, grepping with unwrap() instead of unwrap should solve the immediate refactoring problem. You'd also would to grep for expect(. A linting rule would do a better job at this.

In general, regarding panics in the code, I feel like if I want to ensure there are no panics along a certain code path, I'd want to enforce that somehow. Grepping is not good enough unfortunately, and I don't know a good way either (well, you can register a custom panic handler for no_std envs, where you control all the code that ends up in the binary, like with the https://github.com/japaric/panic-never crate - but that's a very limited solution).

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

mzabaluev commented Nov 5, 2019

Let's go in-depth and see how bad would this breaking change be?

As far as I understand, if someone, somewhere has defined an impl<T, E> From<Result<T, E>> for Foo where T: ..., this new blanket impl From<Result<T, !>> for T would overlap with that.

I feel like if I want to ensure there are no panics along a certain code path, I'd want to enforce that somehow.

Which is what this whole RFC is about :) into_ok is distinctive enough to audit, or to search-and-replace with another way of handling a Result when the error type needs to change.

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 5, 2019

Which is what this whole RFC is about :)

Will, this RFC doesn't help with enforcing a certain code path doesn't contain panics in a general case. It only help with a particular unwrap() situation. It's ok, but I'd rather prefer an annotation, something like #[never_panics] on a function definition. This is what I mean by enforcing - no matter what you write in the fn body - .unwrap() for a result, or call panic! explicitly - the compiler would error out at compile time. That would guarantee (i.e. enforce) that the code path can't possibly panic. Too bad it doesn't exist yet, but that'd be extremely useful.

I understand that this RFC provides a workaround to aid people avoid unexpected panic points appearing in their code when doing the code change - and this is good and valuable. It's a very specific case though.

into_ok is distinctive enough to audit, or to search-and-replace with another way of handling a Result when the error type needs to change.

It's distinctive where we have a pattern. I'd argue we should follow a pattern as long as there is one.

If we want to change the pattern to ease the audit and search-and-replace - that's a topic worth discussion, but I'd change the existing API too. That'd be a breaking change - not in just the code, but in look-and-feel of the API and it's design (which is a given when optimizing something for audit and search-and-replace). Currently, there's a certain pattern that, just by existing, encourages similar functionality to follow the pattern, otherwise the discoverability of the functionality and logical soundness of the API is hurt. Following pattern is important, because by doing that we can maintain the API as a logically sound set of functionality. Of course it would work both ways, but I'd say the more we deviate from the naming pattern the more chaos we bring to the API. This particular case is definitely not justified enough to break the unwrap-naming pattern.

I agree that it's a shame some calls (unwrap, unwrap_err, etc...) do panic, while other (unwrap_or, unwrap_or_default) don't - but that's we have to live with. Unless we change it :). But doing a single thing out of pattern in not changing - it's making life more difficult for when we do actually want to change the whole pattern.

So, to be concrete, I'll give a counter proposal for the name: how about unwrap_or_trivial? I think it makes sense and has the benefit of not breaking the pattern.

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 5, 2019

Will, this RFC doesn't help with enforcing a certain code path doesn't contain panics in a general case. It only help with a particular unwrap() situation. It's ok, but I'd rather prefer an annotation, something like #[never_panics] on a function definition. This is what I mean by enforcing - no matter what you write in the fn body - .unwrap() for a result, or call panic! explicitly - the compiler would error out at compile time. That would guarantee (i.e. enforce) that the code path can't possibly panic. Too bad it doesn't exist yet, but that'd be extremely useful.

That is a very different feature, and out of scope for this RFC. You could create a new RFC for this purpose though.

But doing a single thing out of pattern in not changing - it's making life more difficult for when we do actually want to change the whole pattern.

There is a naming convention when you consume a value and produce a value that was inside the old value, it is usually named into_inner. But this poses a problem of Result, because we could convert to a Ok or a Err, so to get around this ambiguity we use into_ok, and possibly into_err.

I have used a similar convention in my own project, into_$variant to convert an enum into a specific variant.

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 5, 2019

That is a very different feature, and out of scope for this RFC. You could create a new RFC for this purpose though.

If you read carefully, you'll see that that is my point.

There is a naming convention when you consume a value and produce a value that was inside the old value, it is usually named into_inner. But this poses a problem of Result, because we could convert to a Ok or a Err, so to get around this ambiguity we use into_ok, and possibly into_err.

That's all good, except Result is special and has it's own conventions: unwrap and so on. How about we change the whole unwrap family? If we do - we should do it at the other RFC. If we don't - just follow Result's naming conventions. Again, just to reiterate my point.

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 5, 2019

That's all good, except Result is special and has it's own conventions: unwrap and so on. How about we change the whole unwrap family? If we do - we should do it at the other RFC. If we don't - just follow Result's naming conventions. Again, just to reiterate my point.

I would rather not follow Result's naming conventions here, because that would lead to a really long name, like unwrap_or_trivial, and I wouldn't really use it, or it would lead to confusion with unwrap, and that's not good for anyone. into_ok is short, clear, and distinct from unwrap. All these things are good. I don't think renaming the older variants would be a good idea simply because there is too much churn. But that doesn't mean that we need to stay stuck to that naming scheme.

@MOZGIII

This comment has been minimized.

Copy link

MOZGIII commented Nov 6, 2019

Well, for what difference it makes, I don't like into_ok cause it's so good and used for this very specific use case. I'd alias unwrap as that. Having long and explicit name for this feature is much better than some shorter one. Of course if you won't use it that's a big problem (you also won't use unwrap_or_default, right?).

If we were just thinking about adding a function without thinking about the whole API - the into_ok is obvious solution. It's kind of local best, yet if you look at a bigger picture - unwrap_or_trivial is the next best if we also take the Result's current structure into account.

In addition to all the above, there's a practical benefit for discoverability: autocompletion will happily suggest unwrap_or_trivial when you type unwrap, which would arguable be better with the Result context, than into.

Also, Result already have the fn ok(self) -> Option<T>. into_ok is not justified enough. If we want to go with into_ style, we still need to properly discriminate that this is only for E = ! (i.e. for infallible errors). Again - into_-style fns do not show up anywhere in the Result docs page. There are ok() and err() (and transpose() among the interesting non-unwrap ones).

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Nov 6, 2019

Having long and explicit name for this feature is much better than some shorter one.

Only if the longer name gives some much needed context, which in this case unwrap_or_trivial does not give any additional context over into_ok

(you also won't use unwrap_or_default, right?).

No, I don't. I generally don't use Default anyways because it's semantics are unclear at best.

Also, Result already have the fn ok(self) -> Option. into_ok is not justified enough. If we want to go with into_ style, we still need to properly discriminate that this is only for E = ! (i.e. for infallible errors)

It seems like you are confused. This is the api being proposed here does use !

impl<T> Result<T, !> {
    fn into_ok(self) -> T {
        match self {
            Ok(x) => x,
            Err(x) => x, // here `x: !`
        }
    }
}

But with one slight modification, we allow any error type that can be converted to the uninhabited type. We can rely on From/Into not panicing because that's part of their contract.

impl<T, E: Into<!>> Result<T, E> {
    fn into_ok(self) -> T {
        match self {
            Ok(x) => x,
            Err(x) => x.into(), // here `x.into(): !`
        }
    }
}

Also, Result already have the fn ok(self) -> Option. into_ok is not justified enough. If we want to go with into_ style, we still need to properly discriminate that this is only for E = ! (i.e. for infallible errors). Again - into_-style fns do not show up anywhere in the Result docs page. There are ok() and err() (and transpose() among the interesting non-unwrap ones).

Yes, because it would only make sense to have into_ok and into_err, both of which don't exist yet.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Nov 7, 2019

I think it might be better to call it something that doesn't start with unwrap.

I'm torn on this one, @remram44. I agree with you, but .unwrap_or(0) and friends have already set a precedent that the word unwrap by itself doesn't mean "can panic".

@jdahlstrom

This comment has been minimized.

Copy link

jdahlstrom commented Nov 7, 2019

While we're bikeshedding, how about always_ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.