[Accepted with Modifications] SE-0235: Add Result to the Standard Library


(John McCall) #1

Hello, Swift Community.

The review for the revisions to SE-0235: Add Result to the Standard Library ran from November 28th through December 2nd. You can find the review thread here. There was also an earlier round of review for the initial proposal.

The community provided a lot of very good feedback on the proposed revisions. Almost all of it was strongly positive about the revised proposal overall, but many community members expressed reservations about various aspects of it. The Core Team discussed this feedback today and reached several conclusions:

  • While several members of the community supported naming the cases value and error, overall people seemed to prefer the original proposal's case names: success and failure. After considering this feedback, the Core Team has come to agree that these are excellent "semantic" names which work better than value and error as labels for the current state of a Result value. They read more fluently when constructing a Result or pattern-matching on it, especially when the value type is (), and they avoid confusion over the fact that a Result is itself a value.

    The concern that originally led the Core Team to suggest renaming the cases was about the cognitive load of using different names for the cases, case accessors, and type parameters of the same type. It's easy to remember these differences when reading the proposal, but this will predictably become an annoyance after a few months. But this doesn't specifically require using value and error; any consistent set of names will do. So we should consider whether we could use success and failure for the type parameters and case accessors.

    Success and Failure are reasonable choices for the type parameter names. Failure is particularly nice because it's a problem to have both an Error parameter and a global Swift.Error type: not because the shadowing is difficult for the compiler, but because the reuse of the name makes code harder to read and — perhaps more importantly — makes it unnecessarily awkward to talk about in person. These names are also rarely used in code, as they only ever need to be written within extensions of Result.

    Case accessors are a more complex question. The current proposal doesn't include case accessors, but the original proposal did, and we're anticipating a future proposal that will add automatic synthesis for case accessors. That synthesis will undoubtedly use names based on the actual case names, so at a first glance, it seems imprudent to use case names that we're not sure we'd want for the accessors. However, feedback on this review has led the Core Team to more seriously consider this issue, and it now seems quite complicated. A name that's a good name for a particular state of an enum (like failure) might not be a good name for a value carried in that case (like error). It may be true that case accessor names will always end up feeling a little contrived, and so it's just better to use the actual case names because they're easy to interpret. More importantly, we don't want to burden this proposal because we haven't resolved those questions yet to our satisfaction.

    Accordingly, the case names are revised (back) to success and failure, and the type parameter names are revised to Success and Failure.

  • Many people in the community strongly supported requiring Failure to conform to Error, although this was not universal. The Core Team remains convinced that this is the right thing to do. We do not see any need to add new conformances to Error for library types like Array.

  • Several members of the community voiced disappointment that the proposal did not include a variant of flatMap that took a throwing closure. The Core Team reaffirms its desire to never overload methods by whether an argument function can throw. We spent quite some time debating whether a different name would be acceptable, such as flatMapThrowing, but were unable to reach consensus on this, especially as there are alternative means of expressing this using the catching initializer.

    Several members of the Core Team expressed discomfort with the use of the name flatMap on non-collection types. Since it would be unreasonable for the Core Team to introduce a novel name without review, and since there is already precedent for this name from Optional.flatMap, and since this is quite an important operation on Result, there was consensus to accept it under the name flatMap regardless of that discomfort. However, we would like to invite further discussion about the name of this operation on Optional and Result.

    Accordingly, the map functions remain as stated in the revision.

  • Several members of the community expressed dislike of the name unwrapped(), as it seems insufficiently "active" for an operation that is essentially designed to throw as a core part of its operation. The Core Team agrees and also feels that unwrapped is awkward because nothing else about Result uses "wrapping" as an analogy. Furthermore, a similar operation will be needed on other computation-related types like Future, and it would be good for these to share a consistent name. The Core Team debated this and ultimately settled on get(). That this operation can throw should be apparent enough from the mandatory use of try before the call.

The Core Team acknowledges the call from several reviewers to make a special effort to communicate how we think Result ought to be used in Swift. We absolutely agree that is an important part of adding Result to the language, and we intend to provide this guidance as part of the release.

Although we are revising the proposal again, the Core Team feels that these issues have already received adequate review, and there is no need for a third round of review. Accordingly, SE-0235 has been Accepted with Modifications.

Please remember that there will be opportunities to extend this type in the future.

As always, thank you for helping to make Swift a better language.

John McCall
Review Manager


[Revised] SE-0235: Add Result to the Standard Library
(Jon Shier) #2

I'll update the PR as soon as I can! :smile:

My only concern is get(), which feels weird, since I think it's an unprecedented name for the operation (as far as Result types go), but everything else looks great!


(Matthew Johnson) #3

I'm disappointed to see the reversion on the case names and especially sad to see Success and Failure chosen as the parameter names. These feel like rather awkward names for the type parameters to me where Value and Error are very straightforward. I don't think I will ever get used to or fully comfortable with these names. Nevertheless, I am happy to see that we will finally have a Result type in the standard library!


(Ankit Aggarwal) #4

I brought this up before in one of the discussion thread but haven't been able to follow up. The source compatibility section in the proposal is way too vague. I am not sure what happens to existing source code when this type is introduced in the stdlib. For e.g., what happens in SwiftPM itself? If existing source code will break then maybe type should be gated on swift-version 5+.


(John McCall) #5

@Ben_Cohen, care to weigh in on that?


(Richard Wei) #6

I'm disappointed to see get(), as this is probably the first function name with "get" in the standard library, and it's inconsistent with existing naming conventions. If we are going with get(), should the naming conventions be updated to explain when and why a "get" name makes sense?


#7

transform()” comes to mind.

If we had throwing properties, then value would be perfect here. Perhaps value() instead?


(John McCall) #8

transform() ” comes to mind.

I didn't mean to invite that conversation to happen in this thread. :)

If we had throwing properties, then value would be perfect here. Perhaps value() instead?

I feel like this is somewhat ruled out by existing practice, where value() is a case accessor.


(Jon Shier) #9

I've updated the PR with the revised API.


#10

I thought the case was spelled success now?


(John McCall) #11

I mean that there are common Result implementations that declare func value() -> Value? (i.e. non-throwing). And I wouldn't want to rule out adding this to our standard Result eventually.


(Hooman Mehr) #12

It is called get() in my Result type (it is not open source).


(Guillaume Lessard) #13

It’s get() in mine too.


#14

I'm OK with get(), but I think that it should be reserved to immediate calls that do not involve any concurrency subtlety. An eventual Future type, for example, should not use get(). A better choice for futures would be wait().

I say that because I want code reading to quickly tell what is happening. Type inference easily hides the real type of values, and it is important that get() has a consistent meaning (here, a very fast and cheap operation).


(Xiaodi Wu) #15

The only justification I can see is that get will be a term-of-art exception like map. Which is fine.

Note that even the core team justification circles back to contradict its own premise:

[T]he name unwrapped() [...] seems insufficiently "active" for an operation that is essentially designed to throw as a core part of its operation. [...] That this operation can throw should be apparent enough from the mandatory use of try before the call.


(Nobody1707) #16

get() seems fine, if a bit unusual for Swift. I think it would probably be named resolve() on a Promise/Future type.


(Adrian Zubarev) #17

I‘m really confused by this decission, because I don‘t understand the revised Type parameter names. During the second review the most people disliked the enum case names value / error and wanted the previous version of success / failure back while keeping the generic type parameter as they were proposed.

I as a non-native English speaker can tell that this reads now very much akwardly.

  • result success vs. result value
  • result failure vs. result error (this is kind of “okay“)

In pattern matching we now have yet again the same weird situation where the case type is equal to the case name which is redundant. This was exactly the reason most of us wanted success(Value) / failure(Error) but not value(Value) / error(Error) nor success(Success) / failure(Failure).

IMHO Result<Value, Error> is just superior to Result<Success, Failure>.


#18

But, as the decision text says, you generally won't use those type names unless you are extending Result itself. From my perspective, at least, the main issue was that it would be natural to use let .error(error) and similar in switch statements, which would now naturally be let .failure(error).


(Adrian Zubarev) #19

I read it, but it does not mean that I agree with that decision. Well I guess not everyone can be happy with this. At least we‘re getting the type with an explicit error type parameter.


(Dale Buckley) #20

Since there are a number of negative comments about the naming decision, I thought I would throw my hat into the ring and say I'm happy with the naming. So a +1 from me :slight_smile: