Killing `Ok(())` with `ok!()`

libs
21

I have published okk crate to crates.io, which provides the ok() function and ok!() macro described through this thread. :smiley:

1 Like
22

Pulling in a dependency to avoid typing Ok(()) and then use a macro nobody knows seems overkill. I'd rather just return ::std::result::Result::Ok(::std::iter::once(::std::default::Default::default()).collect()) then to be more explicit and cryptic at the same time.

5 Likes
23

Although I don't think Ok(()) is too bad, it is definitely a tad ugly for how simple of a value it represents. In many cases it is the default and four punctuation characters to return it does feel like a lot. And Ok(()) is not the only time this happens; I've also seen Ok(None) and Some(()) and various other flavors of “this function went fine, return the ok-but-valueless sentinel”.

What I'd really like is just a function that always returns <RetType as Try>::from_output(Default::default()). Basically,

#![feature(try_trait_v2)]
use std::ops::Try;

fn good<O: Default, T: Try<Output = O>>() -> T {
    T::from_output(Default::default())
}

fn return_result() -> Result<(), String> {
    try_some_stuff()?;
    good()
}

However, good() has little value if it's going to be six characters; ideally it'd be really short, but it seems like that would only be possible with punctuation, which would require syntactic changes, which would be maybe be too magical to be worth it. And the only suggestion for intuitive punctuation I can think of off the top of my head is ?! (was it ok? yes!) but... ew, no.

24

I agree, the first time I wrote a try block, I ended it with Ok(()), and was confused when that didn't work. the automatic wrapping of try blocks feels extremely unideomatic considering just how hard rust tries to avoid implicit conversions.

And since there's no trailing Ok(()), there's nowhere to fill in the inferred types unless you assign it to a variable, which can cause problems with temporary lifetime extension.

4 Likes
25

I feel that. Even Default::default() itself can be considered a "noisy default". But replacing any of these with a different function or macro invocation is zero-sum.

It would have to be a magical inference to really gain anything of value. Briefly I thought that it could look like a coercion from () to Default::default(). Which implies a need for impl<T: Default, E> Default for Result<T, E> ... Does anyone know why this doesn't exist? Possibly because it wouldn't be symmetric to impl<T> Default for Option<T>, but I honestly don't know.

But then I snapped out of it and realized just how many bugs that would accidentally introduce.

26

Yes, and some have argued that a default() function should be in the std prelude. It's a bit less important since we often write Foo::default() in practice. But I believe default would've been a more logical addition to the prelude than the recent size_of/align_of additions.

Note that we do not use the "correct, but noisy" Result::Ok/Err and Option::Some/None. Because of their commonality the enum variants are included into the prelude. Arguably, eliminating Ok(()) and Ok(val)/Some(val) in general using Ok-wrapping is a similar streamlining of the language in this area.

27

Yeah, I have no idea where all that anti-Ok(()) sentiment is coming from. We should be proud to have Ok(()).

I can only imagine that a lot of folks coming from languages that don't have Ok(()).

Makes me wonder what would happen if a lot of folks came from languages that don't have a * for multiplication. Would they then also complain that * looks ugly and try to replace it with something else?

3 Likes
28

When the sentiment was the strongest was before it was decided that try blocks should wrap their result, so try { x? } is an identity transform. At least a portion (e.g. the thread I created) was exploring what a more general solution could look like as opposed to the more targeted case of try blocks wrapping.

The dislike of tail Ok(()) is that what it represents is just “control flow reached here successfully.” It doesn't really say anything meaningful (purposefully). Omitting it is desirable for the same reason the () value produced by an empty tail expression is itself implicit, and why clippy prefers op(); Ok(()) to Ok(op()).

I personally don't think that replacing Ok(()) with something else could ever be beneficial, though; the alleged benefit is only from being able to fully omit it. And that benefit is in not insignificant part making using Result<(), E> look more like exceptions.

Because function bodies are ? scopes, I do think that there should be an option to make the function body wrap the output like try blocks do[1]. But I also think that the default behavior being limited to direct returns and implicit​ () is valuable, maybe extended to Try::from_output(()). () is the unit value for a completed computation.

There was certainly some of this around ? and .await discussions. I forget the name of it, but there's a principle that people want longer, noisier syntax for unfamiliar operations, and shorter, more implicit syntax once they're familiar with a concept.

  1. In my wildest fantasies, the syntax is fn f() -> try T, E { … } and this is polymorphic over caller choice of impl Try type at the ABI level. For non-scalar T/E, taking two out-pointers, logically writing undef to one and a value to the other, and returning a bool indicating which was initialized. The most common way to call would be with a union{T,E} place and calling the appropriate Try constructor based on the bool return, but specific cases could potentially be further optimized. If it turns out that's not practical, it'd just always use one out pointer instead of two. ↩︎

11 Likes
29

I’m (minorly) anti-Ok(()) because once you start using ? it’s inconsistent with normal ()-returning functions, where we don’t make people write () as the last expression in the function block. But I don’t know if I would want to take it further than (). Let’s see how it would work for the other Try types today:

  • ControlFlow: would default to Continue(()). That’s pretty reasonable for visitors.
  • Option: would default to Some(()). Kind of weird, but you’d have to have the function result be Option<()> to begin with to run into that, and that’s pretty uncommon. Definitely makes me concerned about extending to Default in general though.
  • Poll: Honestly these are weird to me, I would have preferred ? on Poll to propagate Pending and then use a separate ? based on what’s inside. But you’d end up with…a compiler error, because the expected output type of a Poll-based try block is still supposed to use Poll.

…and that is all the Try types. :person_shrugging: Less useful than I hoped, but I think enough to demonstrate adding Default is an extra step and not a natural extension. We don’t do that for functions today, using Default as an implicit return.

4 Likes
30

My suggestions for reducing syntactic noise. Published in some ergonomics crate, nobody will really be in a position to do anything about it. Don't be put off by conservatives. What the compiler waves through is correct Rust. (I don't believe they will add ok or the like to the standard library.)

macro_rules! purify_syntax {
    ($(let $id:ident $($X:ident)? ($($x:ident: $tx:ty),*):
    $rt:ty = $body:expr;)*) => {
        $(fn $id $(<$X>)? ($($x: $tx),*) -> $rt {$body})*
    }
}

trait Pipe: Sized {purify_syntax!{
    let pipe R (self: Self, f: impl FnOnce (Self) -> R): R =
        f(self);
}}
impl<T> Pipe for T {}

purify_syntax!{
    let add(x: i32, y: i32): i32 =
        x + y;

    let main(): std::io::Result<()> = {
        let z = add(1, 2);
        println!("{z}");
    }.pipe(Ok);
}
31

Yeah, I suggested it as mostly a joke, but still as an explicit call. Sometimes you just don't care :yum:

32

I forget the "name" of the quote, but it comes from Bjarne Stroustrup.

1 Like
33

So here's a utility function I just wrote:

/// Return a String parsed from byte slice `s` up to the first NUL.
/// `what` should describe what the string is supposed to represent;
/// it is used only for error messages.
pub fn string_from_byte_slice(
    s: &[u8],
    what: &'static str,
) -> MyResult<String> {
    Ok(ffi::CStr::from_bytes_until_nul(s)
        .map_err(|e| MyError::NotCString(e, what))?
        .to_str()
        .map_err(|e| MyError::NotUTF8(e, what))?
        .to_string())
}

I don't love it. What I most don't love about it is that the Ok constructor is at the top instead of the bottom. I wish I could write the body something like

    // can't do this today because the Ok constructor can't be called
    // like it's a String method
    ffi::CStr::from_bytes_until_nul(s)
        .map_err(|e| MyError::NotCString(e, what))?
        .to_str()
        .map_err(|e| MyError::NotUTF8(e, what))?
        .to_string()
        .Ok()

or else like

    // can't do this today because after the to_str() we'll have a type
    // conflict between the two error cases, and also how is the second
    // map_err to know it's only to be applied to errors coming from the
    // and_then
    ffi::CStr::from_bytes_until_nul(s)
        .map_err(|e| MyError::NotCString(e, what))
        .and_then(|s| s.to_str())
        .map_err(|e| MyError::NotUTF8(e, what))
        .map(|s| to_string())

I could write it like

    ffi::CStr::from_bytes_until_nul(s)
        .map_err(|e| MyError::NotCString(e, what))
        .and_then(|s| s.to_str().map_err(|e| MyError::NotUTF8(e, what)))
        .map(|s| to_string())

but the nested chaining is worse than the original.

IMHO a nice solution to this that also dealt with Ok(()) would be worth more than something that only addressed Ok(()).

34

With wrapping try fns your snippet could look like this:

try fn foo(s: &[u8], what: &'static str) -> MyResult<String> {
    ffi::CStr::from_bytes_until_nul(s)
        .map_err(|e| MyError::NotCString(e, what))?
        .to_str()
        .map_err(|e| MyError::NotUTF8(e, what))?
        .to_string()
}

I think it's cleaner than your hypothetical .Ok().

It's a bit off-topic, but with a helper library like snafu you can improve it a bit further:

try fn foo(s: &[u8], what: &'static str) -> MyResult<String> {
    ffi::CStr::from_bytes_until_nul(s)
        .context(NotCStringSnafu{ what })?
        .to_str()
        .context(NotUTF8Snafu { what })?
        .to_string()
}
2 Likes
35

I never write Ok(big_expr). I just refactor it into

let result = big_expr;
Ok(result)

But in this specific case, I believe it's better to write a match statement instead of forcing functional combinators.

2 Likes
36

That can be done relatively easily, some/most IDEs even have a .ok snippet that expands to Ok(...). Though whether this should be in the std-lib/prelude I don't know.

trait OkExt: Sized {
    fn ok<E>(self) -> Result<Self, E>;
}
impl<T> OkExt for T {
    fn ok<E>(self) -> Result<Self, E> {
        Ok(self)
    }
}

fn test() -> Result<usize, ()> {
    5.ok()
}
37

I've wanted this proposed .ok() extension trait to wrap things in results for a very long time, ideally in stdlib and prelude.

.await proved that postfix is simply more ergonomic when you're working with a "unit" of data that proceeds in a pipeline. There's just something uncomfortable about putting the Ok constructor "above" or "around".

But there are a couple issues with .ok() unfortunately. One of those issues is that core::result::Result already exposes a stable .ok() method but you could just have the method be called .wrap_ok() instead. However, what about supporting the opposite, wrapping a result in Err?

Well, something that's a little bit more generalised than .wrap_ok() and .wrap_err() etc. would just be .wrap(Constructor) which takes advantage of the fact that constructors are functions (or do they just implement the Fn traits - which is it, technically?)

trait WrapExt: Sized {
    fn wrap<Wrapped, Wrapper: FnOnce(Self) -> Wrapped>(self, wrapper: Wrapper) -> Wrapped;
}

impl<T> WrapExt for T {
    fn wrap<Wrapped, Wrapper: FnOnce(Self) -> Wrapped>(self, wrapper: Wrapper) -> Wrapped {
        wrapper(self)
    }
}

fn test_ok() -> Result<usize, ()> {
    1.wrap(Ok)
}

fn test_err() -> Result<(), usize> {
    0.wrap(Err)
}

fn test_some() -> Option<usize> {
    1.wrap(Some)
}

Since this is just a general purpose mapping, instead of wrap it could be map (which unfortunately is already taken) or apply or something along those lines.

5 Likes
38

Very, very hard disagree. I actively avoid all async and hunting for .await all over the code is one reason I find it hard. ? is similar but at least doesn't pretend it's a field and the function is done with itself afterwards.

I can see how it's easier to type, but that writing convenience is usually something someone pays for when they have to read it afterwards.

39

I disagree. JavaScript has prefix await, which I find less clean; it often requires using several intermediate variables.

Compare:

let r = foo()
    .await
    .bar
    .baz()
    .await
    .frob;

to

const r = await (
    (await foo())
        .bar
        .baz()
)
    .frob;

or

const r1 = await foo();
const r2 = await r1
    .bar
    .baz();
const r = r2.frob;

Now I don't have strong feelings about the "field-like" syntax, but IMO postfix is definitely the way to go.

3 Likes
40

I know that there are people with different perspectives. But my experience is my experience. Disagreeing with it doesn't make it go away.

I wouldn't have commented at all if it hadn't been suggested that I'm proven not to exist.

Edit: And those intermediate variables also help the next guy that has to figure out what is going on.