SE-0235: Add Result<Success, Failure: Error> to Standard Library #20958

jshier · 2018-12-03T03:22:28Z

This PR adds the updated Result implementation for SE-0235, as I broke the last PR when trying to rebase off of @rjmccall's Error self conformance PR. Previous PR discussion and review was here.

This PR includes #20629, as Error self conformance is required for this implementation of Result.

jshier · 2018-12-03T03:26:41Z

I did run into some awkwardness on line 74 of the tests, where I couldn't use the property accessor for error (though the value property works fine.

benrimmington · 2018-12-03T11:18:22Z

I copied the header from the other files. Should this one have an updated date?

See SR-7507 and #14576 (comment).

stdlib/public/core/Result.swift

test/stdlib/Result.swift

harlanhaskins · 2018-12-03T23:57:39Z

test/stdlib/Result.swift

+      return nil
+    }
+
+    var error: Error? {


This is nested inside var value: Value?, which is why you aren't able to call result.error. IMO, it's better to just remove this extension and remove those tests.

Thanks, fixed in 7fb5905. I'd like to leave the tests for now, as they're the only ones testing the unwrapping. I can remove the property extension if you think that's important.

jshier · 2018-12-04T02:54:42Z

@benrimmington @slavapestov Copyright year fixed in 7fb5905.

moiseev · 2018-12-05T22:01:33Z

@natecook1000, do you mind looking at the doc comments?

moiseev · 2018-12-06T22:35:34Z

@swift-ci Please test

moiseev · 2018-12-06T22:37:25Z

Now that #20629 has been merged, I would expect the diff to only contain stdlib changes...

swift-ci · 2018-12-06T22:37:50Z

Build failed
Swift Test Linux Platform
Git Sha - 7fb5905

stdlib/public/core/Result.swift

test/stdlib/Result.swift

Co-Authored-By: jshier <[email protected]>

moiseev · 2018-12-06T22:54:49Z

test/stdlib/Result.swift

+  expectEqual(newResult4, .failure(.err))
+}
+
+ResultTests.test("Equatable") {


Did you consider using the checkEquatable from StdlibUnitTest?
https://github.com/apple/swift/blob/master/stdlib/private/StdlibUnittest/StdlibUnittest.swift#L2171

No, as I mostly cribbed these tests by looking at those from Optional and earlier feedback. I didn't know that existed. Can you provide an example of what they would simplify here?

In general, is there any documentation around this special StdlibUnittest API or testing requirements for the standard library?

I was just thinking out loud really. These conformances are synthesized by the compiler, and should be working. Otherwise we're in trouble.

In general, is there any documentation around this special StdlibUnittest API or testing requirements for the standard library?

Unfortunately no.

Can you provide an example of what they would simplify here?

It's not about simplification. Just that checkEquatable tests for the cases you and I might not necessarily think about, like whether == is transitive or not, or whether the implementation is correct in that == is the opposite of !=.

I've added a use of checkEquatable, in additional to the tests that were already there. Let me know if there're any other scenarios you want tested, or if you feel the tests are now redundant.

Note: checkHashable starts by calling checkEquatable -- so it is okay to omit the checkEquatable test if we have tests elsewhere with checkHashable.

moiseev · 2018-12-06T23:10:41Z

@jshier do you mind creating a separate PR for the swift-5.0-branch?

moiseev · 2018-12-06T23:13:41Z

@swift-ci Please test

jshier · 2018-12-06T23:14:07Z

@jshier do you mind creating a separate PR for the swift-5.0-branch?

Sure. From this same branch to the swift-5.0-branch?

benrimmington · 2018-12-06T23:14:42Z

#21073 isn't merged yet.

moiseev · 2018-12-06T23:16:36Z

Sure. From this same branch to the swift-5.0-branch?

Just git cherry-pick -x your commits on top of swift-5.0-branch for a new PR.

#21073 isn't merged yet.

But when it lands, we will have a PR ready for CI :-)

swift-ci · 2018-12-06T23:20:59Z

Build failed
Swift Test Linux Platform
Git Sha - 2bc4cbb

benrimmington · 2018-12-06T23:43:17Z

Now that #20629 has been merged, I would expect the diff to only contain stdlib changes...

Is it worth squashing the stdlib commits and rebasing from master?
Would this make it any easier to cherry pick for the 5.0 branch?

stdlib/public/core/Result.swift

lorentey · 2018-12-07T16:58:00Z

test/stdlib/Result.swift

+  let result1: Result<Int, Err> = .success(1)
+  let result2: Result<Int, Err> = .success(2)
+  let result3: Result<Int, Err> = .failure(.err)
+  checkHashable([result1, result2, result3], equalityOracle: { $0 == $1 })


It would also be useful to have a hashing check with potentially colliding Success/Failure hash encodings:

Suggested change

checkHashable([result1, result2, result3], equalityOracle: { $0 == $1 })

checkHashable([result1, result2, result3], equalityOracle: { $0 == $1 })

let confusables: [Result<Err, Err>] = [

.success(.err)

.success(.derr)

.failure(.err)

.failure(.derr)

]

checkHashable(confusables, equalityOracle: { $0 == $1 })

checkHashable verifies that all four of these cases have different hash encodings. (The original implementation of hash(into:) had an issue with this, so it's not a theoretical concern.)

xwu

Some comments on the documentation.

stdlib/public/core/Result.swift

KyNorthstar · 2018-12-09T17:18:01Z

stdlib/public/core/Result.swift

+  public init(catching body: () throws -> Success) {
+    do {
+      let value = try body()
+      self = .success(value)


Why split these lines instead of just

self = .success(try body())

? I'm curious, as that's something I do a lot in my code

test/Interpreter/error_self_conformance.swift

xwu

Some minor nits, but I like the changes to the documentation; @natecook1000, of course, is the guru and may have wiser thoughts.

xwu · 2018-12-10T03:28:29Z

stdlib/public/core/Result.swift

+  ///
+  /// - Parameter transform: A closure that takes the success value of the
+  ///   instance.
+  /// - Returns: A`Result` instance, either from the closure or the previous 


Suggested change

/// - Returns: A`Result` instance, either from the closure or the previous

/// - Returns: A `Result` instance, either from the closure or the previous

xwu · 2018-12-10T03:28:57Z

stdlib/public/core/Result.swift

+  /// Get the success value as a throwing expression.
+  ///
+  /// - Returns: The success value, if the instance is `.success`.
+  /// - Throws:  The failure value, if the instance is `.failure`.


Suggested change

/// - Throws: The failure value, if the instance is `.failure`.

/// - Throws: The failure value, if the instance is `.failure`.

xwu · 2018-12-10T03:29:11Z

stdlib/public/core/Result.swift

+
+extension Result where Failure == Swift.Error {
+  /// Create an instance by evaluating a throwing closure, capturing the 
+  /// returned value as a `.success` or any thrown error as `.failure`.


Suggested change

/// returned value as a `.success` or any thrown error as `.failure`.

/// returned value as a `.success` or any thrown error as a `.failure`.

moiseev

moiseev · 2018-12-10T07:09:44Z

@swift-ci Please test

swift-ci · 2018-12-10T07:11:52Z

Build failed
Swift Test OS X Platform
Git Sha - 80ae4de

swift-ci · 2018-12-10T07:11:53Z

Build failed
Swift Test Linux Platform
Git Sha - 80ae4de

moiseev · 2018-12-10T19:55:58Z

swiftlang/swift-package-manager#1912
@swift-ci Please test

swift-ci · 2018-12-10T19:59:11Z

Build failed
Swift Test Linux Platform
Git Sha - c739498

swift-ci · 2018-12-10T21:04:30Z

Build failed
Swift Test OS X Platform
Git Sha - c739498

moiseev · 2018-12-10T21:22:13Z

swiftlang/swift-package-manager#1912
@swift-ci Please test

swift-ci · 2018-12-10T21:24:14Z

Build failed
Swift Test Linux Platform
Git Sha - b167703

moiseev · 2018-12-11T04:38:59Z

@swift-ci Please test source compatibility

jshier · 2018-12-12T22:04:58Z

@moiseev Should there be a CHANGELOG entry for this? Also, should I do a PR to update the proposal text to match the final implementation?

moiseev · 2018-12-13T21:51:59Z

@jshier update to a proposal would be great. Don't know the policy for CHANGELOG. Let me find out.

UPD: Yes, please update the CHANGELOG as well.

Add update Result type to standard library.

33dcfc4

jshier mentioned this pull request Dec 3, 2018

Add Result<Value, Error> #19982

Closed

benrimmington reviewed Dec 3, 2018

View reviewed changes

stdlib/public/core/Result.swift Outdated Show resolved Hide resolved

rjmccall reviewed Dec 3, 2018

View reviewed changes

test/stdlib/Result.swift Outdated Show resolved Hide resolved

harlanhaskins reviewed Dec 3, 2018

View reviewed changes

Updates for review comments.

7fb5905

lancep requested a review from moiseev December 5, 2018 21:59

natecook1000 self-requested a review December 5, 2018 22:05

Update for revised proposal.

2bc4cbb

swiftlang deleted a comment from grand-china Dec 6, 2018

moiseev reviewed Dec 6, 2018

View reviewed changes

stdlib/public/core/Result.swift Show resolved Hide resolved

moiseev reviewed Dec 6, 2018

View reviewed changes

test/stdlib/Result.swift Outdated Show resolved Hide resolved

moiseev reviewed Dec 6, 2018

View reviewed changes

test/stdlib/Result.swift Outdated Show resolved Hide resolved

moiseev and others added 2 commits December 6, 2018 17:54

Update test/stdlib/Result.swift

1834f0a

Co-Authored-By: jshier <[email protected]>

Update test/stdlib/Result.swift

80ae4de

Co-Authored-By: jshier <[email protected]>

moiseev reviewed Dec 6, 2018

View reviewed changes

DevAndArtist reviewed Dec 7, 2018

View reviewed changes

stdlib/public/core/Result.swift Show resolved Hide resolved

lorentey reviewed Dec 7, 2018

View reviewed changes

xwu reviewed Dec 8, 2018

View reviewed changes

KyNorthstar reviewed Dec 9, 2018

View reviewed changes

jshier added 3 commits December 9, 2018 19:08

Add additional test cases and use checkEquatable.

85df179

Update inline documentation.

e4a700c

Remove usage lines from docs.

c739498

xwu reviewed Dec 10, 2018

View reviewed changes

moiseev reviewed Dec 10, 2018

View reviewed changes

xwu changed the title ~~SE-0235: Add Result<Value, Error: Swift.Error> to Standard Library~~ SE-0235: Add Result<Success, Failure: Error> to Standard Library Dec 10, 2018

natecook1000 removed their request for review December 10, 2018 17:55

moiseev mentioned this pull request Dec 10, 2018

Prepare SwiftPM for the addition of Result to the stdlib swiftlang/swift-package-manager#1912

Merged

Add qualified import of Result in the test

b167703

Revise some documentation comments.

2bb0b5c

moiseev mentioned this pull request Dec 11, 2018

[5.0] Add Result type to standard library. #21225

Merged

moiseev merged commit 83ccb23 into swiftlang:master Dec 12, 2018

AnthonyLatsis added swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented feature A feature request or implementation standard library Area: Standard library umbrella labels Apr 18, 2023

-  checkHashable([result1, result2, result3], equalityOracle: { $0 == $1 })
+  checkHashable([result1, result2, result3], equalityOracle: { $0 == $1 })
+  let confusables: [Result<Err, Err>] = [
+    .success(.err)
+    .success(.derr)
+    .failure(.err)
+    .failure(.derr)
+  ]
+  checkHashable(confusables, equalityOracle: { $0 == $1 })

	/// - Returns: A`Result` instance, either from the closure or the previous
	/// - Returns: A `Result` instance, either from the closure or the previous

	/// - Throws: The failure value, if the instance is `.failure`.
	/// - Throws: The failure value, if the instance is `.failure`.

	/// returned value as a `.success` or any thrown error as `.failure`.
	/// returned value as a `.success` or any thrown error as a `.failure`.

SE-0235: Add Result<Success, Failure: Error> to Standard Library #20958

SE-0235: Add Result<Success, Failure: Error> to Standard Library #20958

Uh oh!

Conversation

jshier commented Dec 3, 2018

Uh oh!

jshier commented Dec 3, 2018

Uh oh!

benrimmington commented Dec 3, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshier commented Dec 4, 2018

Uh oh!

moiseev commented Dec 5, 2018

Uh oh!

moiseev commented Dec 6, 2018

Uh oh!

moiseev commented Dec 6, 2018

Uh oh!

swift-ci commented Dec 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moiseev Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshier Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moiseev Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moiseev Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moiseev commented Dec 6, 2018

Uh oh!

moiseev commented Dec 6, 2018

Uh oh!

jshier commented Dec 6, 2018

Uh oh!

benrimmington commented Dec 6, 2018

Uh oh!

moiseev commented Dec 6, 2018

Uh oh!

swift-ci commented Dec 6, 2018

Uh oh!

benrimmington commented Dec 6, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moiseev Dec 6, 2018 •

edited

Loading

jshier Dec 6, 2018 •

edited

Loading

moiseev Dec 6, 2018 •

edited

Loading

moiseev Dec 6, 2018 •

edited

Loading

moiseev left a comment •

edited

Loading

moiseev commented Dec 13, 2018 •

edited

Loading