Why is wrapping a match statement in an Ok considered bad form?

Question

In a blog post, Rust contributor withoutboats mentions:

Most of my functions with many return paths terminate with a match statement. Technically, these could be reduced to a single return path by just wrapping the whole match in an Ok, but I don’t know anyone who considers that good form, and I certainly don’t.

A (somewhat contrived) example of this--

bad:

Ok(match response {
    UserResponse(user) => user,
    ItemResponse(item) => item?,
})

better:

match response {
    UserResponse(user) => Ok(user),
    ItemResponse(item) => Ok(item?),
}

Why is that the case?

As far as I know this is just a personal opinion. I've written code like the first example before. Reading boats' blog post is the first time I saw any suggestion of it being "bad form". — trent, Apr 09 '20 at 21:31
I understand why this question was closed, but I think there is context that may give some justification for reopening it. Beyond just "is Ok-wrapping bad?", it actually is a fact-based question about what the community considers good form, which is along the same lines as asking about [idiomatic code](https://stackoverflow.com/q/41510424/3650362) or [best practices](https://stackoverflow.com/q/45086595/3650362), which we don't (always) have a problem with. And burntsushi's answer is good enough to keep around for others who might read the original article and arrive at the same question. — trent, Apr 11 '20 at 02:02

BurntSushi5 · Accepted Answer · 2020-04-09T22:55:52.087

This is kind of a tricky question to answer. I personally can't recall ever seeing anyone express this opinion before. To be fair, I can't say that I've ever heard the opposite. That is, I don't remember anyone ever explicitly encouraging this sort of pattern in code.

I've certainly used this pattern before, although somewhat sparingly. To demonstrate, this typically occurs when you have a match expression with several cases in a tail expression that returns a Result, and you don't want to write Ok for each case. Like this:

Ok(match something {
    Something::Foo => 1,
    Something::Bar => 2,
    Something::Quux => fallible?,
    Something::Baz => return Err(...),
})

As opposed to

match something {
    Something::Foo => Ok(1),
    Something::Bar => Ok(2),
    Something::Quux => fallible,
    Something::Baz => Err(...),
}

It's not a huge difference, but if you have a lot of Ok cases, it can get a bit annoying. Or at least, this was the primary complaint in Boat's blog post (in defense of Ok-wrapping).

Another variant of this pattern is 1) when your function returns a Result, 2) the tail expression of the function is also a Result, 3) the error types are not the same and 4) the error type in (1) has a From impl for the error type in (2). That is, instead of writing

something.fallible().map_err(From::from)

one can write

Ok(something.fallible()?)

? is so ubiquitous that I've always kind of chalked up the difference between these to personal style.

I can't possibly know what the blog author had in mind when saying that this was poor form. My guess---and I somewhat share this---is that there is a lack of symmetry between the arms of the match expression. Some of them are effectively early returns while others aren't early returns at all. I could see how some might consider that jarring.

Otherwise, I do occasionally use both of these patterns, but not religiously so. If it comes up and it makes sense, then I don't have too much of a problem with it.

Thanks for the answer. It was a new idea for me, too. I've used `Ok(match{})` in my code, and definitely use `Ok(fallible()?)` a ton, and couldn't see any problems with it. My hunch was that it was about being explicit in each match arm, but mainly I asked because the opinion was so vehement. — squidpickles, Apr 10 '20 at 05:22

Why is wrapping a match statement in an Ok considered bad form?

1 Answers1