Skip to content
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

Convenience for making Result from NSErrorPointer API #14

Merged
merged 15 commits into from Nov 28, 2014

Conversation

robrix
Copy link
Contributor

@robrix robrix commented Nov 21, 2014

Cocoa API commonly uses BOOL or object type returns coupled with an NSError ** parameter where we would like to use Result instead. This provides a pair of conveniences to produce Result in those circumstances.

This also brings in a deployment target for iOS.

public func try(f: NSErrorPointer -> BooleanType) -> Result<Bool> {
var error: NSError?
return f(&error) ? success(true) : failure(error ?? defaultError([:]))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one feels like it should be returning NSError? instead but then it would be inverted in sense. You can recover that via .error() so I think it should probably stay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Result<()> rather than Result<Bool>? See @BradLarson's code at #10 (comment) for an example of that.

I agree that there are advantages of this over NSError?. Unit is a legitimate thing to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Result<()> is much more satisfying 👍

@robrix
Copy link
Contributor Author

robrix commented Nov 22, 2014

  • Result<()>
  • file/line info collected by try (I think NSLocalizedDescriptionKey should be added by the caller instead of doubling the # of implementations of try)
  • defaultError has forms taking userInfo, file, and line
  • tests of success/failure for both try forms
  • found that using BooleanType was crashing (I should probably file a radar 😞) and therefore changed to Bool

}

func testTryTSuccess() {
XCTAssertEqual(try(makeTryFunction(42 as Int?)) ?? 43, 42)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine with 42 instead of 42 as Int? but I wanted to ensure that the type demonstrates that it could be nil to declare the intent of the test.

@robrix
Copy link
Contributor Author

robrix commented Nov 22, 2014

✅ Ready for re-review 😄

@robrix
Copy link
Contributor Author

robrix commented Nov 25, 2014

bump 😄

@rnapier
Copy link
Contributor

rnapier commented Nov 27, 2014

I'm not quite clear how this is used with a real Cocoa method that takes an NSErrorPointer. Your makeTryFunction doesn't looks like a typical Cocoa method. I've been trying to use this with NSFileManager.contentsOfDirectoryAtPath(error), but I haven't quite figured out how it is supposed to work. Do you have an example?

I'm also very slightly nervous about calling it try. That feels like it implies things that aren't true here. It doesn't handle exceptions, and it's only meant for things that use the ObjC NSError** pattern. So it feels very tried to ObjC in a way that feels awkward for such a common and general name as "try."

I definitely want a good pattern for NSErrorPointer. At some point, I had a Result init(x: T?, error: NSError?) to try to help with this (you could pass the return and the error from a previous Cocoa call). I think try is trying to wrap it up even more than that; I just don't quite know how it works yet. Thanks.

@robrix
Copy link
Contributor Author

robrix commented Nov 27, 2014

Thanks for the feedback!

I'm not quite clear how this is used with a real Cocoa method that takes an NSErrorPointer

Here are usage examples for Bool and NSData respectively:

let result = try { error in NSFileManager.defaultManager().removeItemAtPath(path, error) } // Result<()>
let result = try { error in NSData(contentsOfFile: path, options: 0, error: error) } // Result<NSData>

I'm also very slightly nervous about calling it try.

I have no strong attachment to the name try; it was primarily an analogy with ReactiveCocoa. I think an init would be fine:

let result = Result { String(contentsOfFile: path, error: $0) } // Result<String>

That feels like it implies things that aren't true here. It doesn't handle exceptions

To be fair, nothing in Swift does 😄

@rnapier
Copy link
Contributor

rnapier commented Nov 27, 2014

OK, that usage makes sense. It certainly has a "wow, it just works" quality to it that I like. I don't think it can be an init this way, though, if we're going to parameterize on Error (as noted in #10). But I could just rename it to result(). I'll probably tweet this and see if anyone else has thoughts. It seems common enough and general enough to keep the name short (rather than say errorResult or makeResult or anything like that).

While Swift doesn't currently handle exceptions, you could certainly write a try for Swift by implementing it in ObjC, and I can certainly imagine people doing that. Try just has such a strong meaning across languages. Even Haskell uses it to mean "convert exception into an Either." I'd want it to do the same in Swift.

Having seen this in action, that really is nice…

@robrix
Copy link
Contributor Author

robrix commented Nov 27, 2014

I was going to say that I don’t think parameterizing on the error type is a blocker, except that it probably is because the compiler crashes when attempting to write Result<T, Error>.

I still prefer the constructor, but failing that, result() gets my vote.

@robrix
Copy link
Contributor Author

robrix commented Nov 27, 2014

Actually, this crash probably rules out a constructor altogether: http://www.openradar.me/19091061

@rnapier
Copy link
Contributor

rnapier commented Nov 27, 2014

I realized that "result()" is probably a bad idea. People use "result"
often as a variable name and I think that would be an unfortunate shadow.
(At least I assume it would lead to inconvenient shadowing; if it didn't,
maybe still ok.)

rnapier added a commit that referenced this pull request Nov 28, 2014
Convenience for making `Result` from `NSErrorPointer` API
@rnapier rnapier merged commit 72d9eb7 into LlamaKit:master Nov 28, 2014
@robrix
Copy link
Contributor Author

robrix commented Nov 28, 2014

Oh, good call! I hadn’t thought of that.

@robrix robrix deleted the try branch November 28, 2014 04:53
@rnapier
Copy link
Contributor

rnapier commented Nov 28, 2014

I've been playing with this, and it's raised some interesting things when coupled with parameterizing the error (#10). (I'm currently experimenting with calling it withError rather than try; it makes the first parameter more obvious.)

public func withError<T>(f: NSErrorPointer -> T?, file: String = __FILE__, line: Int = __LINE__) -> Result<T, NSError> {
  var error: NSError?
  return f(&error).map(success) ?? failure(error ?? defaultError(file: file, line: line))
}

The problem is that this particular version only works for NSError-based results. If you try to make it generic on the error, then you run into one of two problems:

  • It must be possible to generate a default error, or
  • It becomes undefined (crashing) behavior if f returns nil but fails to set error.

Maybe that's ok in several directions. Maybe it's ok that this only works with NSError, since this pattern is really an NSError pattern. I don't know if any other error type would work this way.

Maybe it's ok to make a no-result, no-error return be undefined behavior. It is certainly an unusual situation.

If it isn't generic, then it can't be a Result method (since you can't restrict methods). Here's how this wound up looking in practice:

let results =
search
  .stringByAddingPercentEscapesUsingEncoding(NSUTF8StringEncoding)
  .map { "http://en.wikipedia.org/w/api.php?action=opensearch&limit=15&search=\($0)&format=json" }
  .flatMap { NSURL(string: $0) }
  .map { NSURLRequest(URL: $0) }
  .apply { Result($0, error:defaultError("Could create request")) }
  .flatMap { req in withError { error in NSURLConnection.sendSynchronousRequest(req, returningResponse: nil, error: error) } }
  .flatMap { data in withError { error in NSJSONSerialization.JSONObjectWithData(data, options: NSJSONReadingOptions(0), error: error) } }
  .flatMap { json in fromOptional { json as? [AnyObject] } }
  .flatMap { array in fromOptional { array.count == 2 ? array : nil } }
  .flatMap { array in fromOptional { array[1] as? [String] } }

I find the .flatMap { data in withError { error in preamble a little verbose. I'd prefer .withError { (data, error) in

fromOptional is another function I'm playing with that has the same basic problem. For it to be generic, we always have to pass an error (which means something like defaultError() would probably need to be public).

public func fromOptional<T>(f: () -> T?, file: String = __FILE__, line: Int = __LINE__) -> Result<T, NSError> {
  return f().map(success) ?? failure(defaultError(file: file, line: line))
}

@robrix
Copy link
Contributor Author

robrix commented Nov 28, 2014

I don't know if any other error type would work this way.

Let’s hope not! 😬 Edit: But possibly yes. One might want a similar function for other out-of-band error APIs, e.g. glGetError(); and I’m sure I’ve seen ObjC API returning strings by reference for a similar purpose although I can’t place it at the moment.

If it isn't generic, then it can't be a Result method (since you can't restrict methods).

You sort of can with the use of id for type evidence (as in RAC). Regrettable that the language requires this, but it works.

I find the .flatMap { data in withError { error in preamble a little verbose. I'd prefer .withError { (data, error) in

uncurry?

fromOptional is another function I'm playing with that has the same basic problem. For it to be generic, we always have to pass an error (which means something like defaultError() would probably need to be public).

Don’t suppose we can use default parameter values for this? e.g.:

public func fromOptional<T, E: ErrorType>(f: () -> T?, defaultError: E = E.defaultError()) -> Result<T, NSError> {
    
}

IIRC the rule for default parameters is that they have to be literals, so probably not.

Edit: Perhaps it’d be reasonable to put this in an ErrorType protocol (except for the bit where we couldn’t actually extend NSError thus 😠). That is, Result’s Error type would conform to ErrorType which could provide defaultError; withError could be a free function generic over ErrorType (although this would require inference on the return type which is going to irk).

@rnapier
Copy link
Contributor

rnapier commented Nov 28, 2014

I could still make this work if we forced all errors to conform to some
ErrorType protocol that could give me a default value. You'd just pass E?
instead of using a default value. But the question is whether it's worth it
to force error types to conform to anything. And remember that LlamaKit
can't export a public extension to cause NSError to conform currently
(that's considered a Swift bug, but I believe it's still in there).

@robrix
Copy link
Contributor Author

robrix commented Nov 30, 2014

Correct, it’s still present; that’s what I was alluding to in my final edit to the above. (Which presumably didn’t make it as far as your inbox, my apologies!)

@rnapier
Copy link
Contributor

rnapier commented Nov 30, 2014

I will probably have a proposal based on this in about a week and half. I've been revising my upcoming CocoaConf talk to incorporate these ideas, and in introducing it to real code, I've made several little changes that I think makes it nicer.

Here's of my current small-but-not-trivial working example:

func URLForSearch(search: String) -> Result<NSURL, NSError> {
  return success(search)
    .flatMap { Result(
      $0.stringByAddingPercentEscapesUsingEncoding(NSUTF8StringEncoding),
      failWith: mkError("Malformed Search: \($0)")) }

    .flatMap { Result(NSURL(string: queryBase + $0),
      failWith: mkError("Malformed URL: \($0)"))}
}

func DataForURL(url: NSURL) -> Result<NSData, NSError> {
  return withError { error in
    NSURLConnection.sendSynchronousRequest(NSURLRequest(URL: url),
      returningResponse: nil, error: error)}
}

func JSONForData(data: NSData) -> Result<AnyObject, NSError> {
  return withError { error in
    NSJSONSerialization.JSONObjectWithData(data,
      options: NSJSONReadingOptions(0), error: error)}
}

func ParseJSON(json: AnyObject) -> Result<[String], NSError> {
  return
    Result(json as? [AnyObject],
      failWith: mkError("Expected array. Received: \(json)"))

      .flatMap { Result($0.count == 2 ? $0 : nil,
        failWith: mkError("Array incorrect size: \($0)"))}

      .flatMap { Result($0[1] as? [String],
        failWith: mkError("Malformed array: \($0)"))}
}

func pagesForSearch(search: String) -> Result<[String], NSError> {
  return URLForSearch(search)
    .flatMap(DataForURL)
    .flatMap(JSONForData)
    .flatMap(ParseJSON)
}

@robrix
Copy link
Contributor Author

robrix commented Nov 30, 2014

Out of curiosity, at this point, why is this Result<T, NSError> and not Either<NSError, T>?

@rnapier
Copy link
Contributor

rnapier commented Dec 1, 2014

See #10, starting at #10 (comment) for discussion. It is still a open question, though no one has so far suggested actually making the change. The very short version is that Success/Failure is easier to understand than Right/Left.

@robrix
Copy link
Contributor Author

robrix commented Dec 1, 2014

Fair enough.

I’ve just finally got around to publishing my Either type which I’ve been using heavily in Traversal. It includes an EitherType protocol which might be useful here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants