New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from Dec 12, 2018

Conversation

@jshier
Copy link
Contributor

jshier commented Dec 3, 2018

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 jshier referenced this pull request Dec 3, 2018

Closed

Add Result<Value, Error> #19982

@jshier

This comment has been minimized.

Copy link
Contributor

jshier commented Dec 3, 2018

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.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented on stdlib/public/core/Result.swift in 33dcfc4 Dec 3, 2018

2018?

This comment has been minimized.

Copy link
Contributor

jshier replied Dec 3, 2018

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

@benrimmington

This comment has been minimized.

Copy link
Contributor

benrimmington commented Dec 3, 2018

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

It should be the year of first publication: Copyright (c) 2018 Apple Inc.

See SR-7507 and #14576 (comment).

Show resolved Hide resolved test/stdlib/Result.swift Outdated
return nil
}

var error: Error? {

This comment has been minimized.

@harlanhaskins

harlanhaskins Dec 3, 2018

Collaborator

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.

This comment has been minimized.

@jshier

jshier Dec 4, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

jshier commented Dec 4, 2018

@benrimmington @slavapestov Copyright year fixed in 7fb5905.

@lancep lancep requested a review from moiseev Dec 5, 2018

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 5, 2018

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

@natecook1000 natecook1000 self-requested a review Dec 5, 2018

@apple apple deleted a comment from grand-china Dec 6, 2018

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 6, 2018

@swift-ci Please test

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 6, 2018

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

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 6, 2018

Build failed
Swift Test Linux Platform
Git Sha - 7fb5905

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

moiseev and others added some commits Dec 6, 2018

Update test/stdlib/Result.swift
Co-Authored-By: jshier <jon@jonshier.com>
Update test/stdlib/Result.swift
Co-Authored-By: jshier <jon@jonshier.com>
expectEqual(newResult4, .failure(.err))
}

ResultTests.test("Equatable") {

This comment has been minimized.

@moiseev

moiseev Dec 6, 2018

Member

This comment has been minimized.

@jshier

jshier Dec 6, 2018

Contributor

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?

This comment has been minimized.

@moiseev

moiseev Dec 6, 2018

Member

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.

This comment has been minimized.

@moiseev

moiseev Dec 6, 2018

Member

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 !=.

This comment has been minimized.

@jshier

jshier Dec 10, 2018

Contributor

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.

This comment has been minimized.

@lorentey

lorentey Dec 10, 2018

Member

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

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 6, 2018

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

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 6, 2018

@swift-ci Please test

@jshier

This comment has been minimized.

Copy link
Contributor

jshier commented Dec 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

benrimmington commented Dec 6, 2018

#21073 isn't merged yet.

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 6, 2018

Build failed
Swift Test Linux Platform
Git Sha - 2bc4cbb

@benrimmington

This comment has been minimized.

Copy link
Contributor

benrimmington commented Dec 7, 2018

#21073 has just been merged.

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 })

This comment has been minimized.

@lorentey

lorentey Dec 7, 2018

Member

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

Suggested change Beta
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.)

This comment has been minimized.

@jshier

jshier Dec 10, 2018

Contributor

Added.

@xwu
Copy link
Collaborator

xwu left a comment

Some comments on the documentation.

Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift Outdated
Show resolved Hide resolved stdlib/public/core/Result.swift
public init(catching body: () throws -> Success) {
do {
let value = try body()
self = .success(value)

This comment has been minimized.

@BenLeggiero

BenLeggiero Dec 9, 2018

Contributor

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

Show resolved Hide resolved test/Interpreter/error_self_conformance.swift Outdated

@jshier jshier force-pushed the jshier:add-result branch to 80ae4de Dec 9, 2018

@xwu
Copy link
Collaborator

xwu left a comment

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

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

This comment has been minimized.

@xwu

xwu Dec 10, 2018

Collaborator
Suggested change Beta
/// - Returns: A`Result` instance, either from the closure or the previous
/// - Returns: A `Result` instance, either from the closure or the previous
/// 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`.

This comment has been minimized.

@xwu

xwu Dec 10, 2018

Collaborator
Suggested change Beta
/// - Throws: The failure value, if the instance is `.failure`.
/// - Throws: The failure value, if the instance is `.failure`.

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`.

This comment has been minimized.

@xwu

xwu Dec 10, 2018

Collaborator
Suggested change Beta
/// returned value as a `.success` or any thrown error as `.failure`.
/// returned value as a `.success` or any thrown error as a `.failure`.
@moiseev
Copy link
Member

moiseev left a comment

:shipit:

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 10, 2018

@swift-ci Please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 10, 2018

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

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 10, 2018

Build failed
Swift Test Linux Platform
Git Sha - 80ae4de

@xwu 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 natecook1000 removed their request for review Dec 10, 2018

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 10, 2018

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 10, 2018

Build failed
Swift Test Linux Platform
Git Sha - c739498

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 10, 2018

Build failed
Swift Test OS X Platform
Git Sha - c739498

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 10, 2018

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Dec 10, 2018

Build failed
Swift Test Linux Platform
Git Sha - b167703

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Dec 11, 2018

@swift-ci Please test source compatibility

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

4 of 6 checks passed

Swift Source Compatibility Suite on macOS Platform (Debug)
Details
Swift Source Compatibility Suite on macOS Platform (Release)
Details
Swift Test Linux Platform 11551 tests run, 10409 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 57965 tests run, 2480 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
@jshier

This comment has been minimized.

Copy link
Contributor

jshier commented Dec 12, 2018

@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

This comment has been minimized.

Copy link
Member

moiseev commented Dec 13, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment