Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager via either forum DM or email. When contacting me directly, please put "[SE-0532]" at the start of the subject line.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at
As in the pitch, I continue to be impressed with how well these APIs leverage recently added features. I have some nits about the naming aspect of these APIs:
As before, I worry that insert reads poorly particularly with the example (maybe raised during the pitch) where Wrapped is a collection, as it reads most naturally like one is inserting into the wrapped value. I think assign or another variation on that would go a long way to avoiding that misreading.
It's weird that mutate() gives something mutable but doesn't actually mutate anything. I wonder if borrowed and mutable, or just ref and mutableRef (à la span and mutableSpan) would be the superior. I'm not sure I understand the argument given in the proposal text that the "nature" of the verbs "fits quite well"—indeed, I think I'd argue that the "nature" of the verb mutate specifically fits poorly in this case.
There has always been resistance to adding more functionality to Result, like isSuccess, or a fold method (recent async additions notwithstanding), because it's felt those should be more general enum features. This functionality also seems like something that should be available to all enums, but such an approach isn't even mentioned as an alternative or future direction. So while this functionality seems necessary, it seems best to make it available to all enums in some way. Otherwise we'll have to use something like swift-case-paths to synthesize imitation versions of these, if that's even possible.
Personally, I'd also like something more ergonomic, though I suppose we can see how popular these APIs are and make them more ergonomic later.
I’m not sure I understand which of these changes you feel would be covered by generic support on all enums.
I think you might be arguing that map could be a generic enum-homomorphism in the same way that fold could be a generic enum-catamorphism? But I honestly can’t remember anyone ever arguing that we shouldn’t add a fold to Result because we might add generic catamorphisms to all enums. I mean, other than the generic catamorphisms on enums we already have via switch expressions.
Sorry, wouldn't the access patterns outlined in the Motivation apply to all enums with associated values, except one layer removed? It would require case accessors, but wouldn't I need something like result.success.borrow() to borrow the success value out of a Result? Or is the thought that this can just be generalized in terms of Optional directly? Or have I misunderstood the purpose of these APIs?
And my mention of fold was simply an example generalizable API. I think I had it or something like it in the original Result proposal that was subsequently removed, but it was really just an example.
The example is probably poor given the wrapped type. @benlings suggested put as the opposite of take which I quite like.
One of the reasons why I opted not for computed variables for these like span and mutableSpan is that these have the chance for "failure" by returning nil for empty optionals unlike the span accessors which can never "fail" even if the array is empty.
I think we definitely want some language feature here that broadly solves this for all enums. Something akin to the following:
if case .success(borrow x) = result {
// ok we're borrowing the success value
x.performSomeMethod()
}
// result is still usable here
if case .success(inout x) = result {
// ok we can directly mutate the success value in result
x += 1
}
// result is still usable here
Generally speaking, this borrow x is only valid for that tight scope. Certain enum layouts require us to temporarily destroy the enum value itself in order to get a direct reference to payload contents which means this language feature can never return the reference to an arbitrary enum payload out of this scope:
func something(x: borrowing SomeComplicatedEnum) -> ??? {
guard case .myCase(borrow y) = x else {
return
}
// ok I can use 'y' here which is borrowing the payload
// in 'x'.
// error: can't return 'y'.
// we must cleanup the 'y' reference and build 'x' back
return y
}
Optional, however, has a special layout that allows us to project this reference without destroying the optional value itself allowing us to return this reference out without having to perform any cleanup of the enum value:
If we don't want to go the "Optional as a collection" route of naming the operation insert, we could always use set, store, or put.
While non-destructive pattern matching is a nice feature to have, I don't think it's a problem to give Optional a special case implementation given that it's apparently not straight forward to generalize.