Skip to content
This repository has been archived by the owner on Apr 4, 2018. It is now read-only.

Failable Initializers #7

Closed
ElvishJerricco opened this issue Dec 11, 2015 · 15 comments
Closed

Failable Initializers #7

ElvishJerricco opened this issue Dec 11, 2015 · 15 comments

Comments

@ElvishJerricco
Copy link
Contributor

As mentioned here, it would be best to use failable initializers instead of optional dispatch objects. However, there are a few methods / initializers which (especially in DispatchData.swift) where it is not clear whether the method / initializer should fail, or return an optional or an implicitly unwrapped optional. The goal of this issue is to determine how each of these methods / initializers should behave.

@anpol
Copy link
Owner

anpol commented Dec 11, 2015

Actually, I'm not sure that failable initializers is the best approach. I suspect most users are expecting that most GCD calls succeed, so they will use unwrapped values without checking for nil.

Probably, it would be better to provide an error handler that would be invoked when libdispatch call returns NULL. Such error handler could be set by the user, its possible behavior could be either assert/fail (this is the default), or log message, or ignore error completely. If the error is ignored, than we can fallback to failable initializer when feasible, or use non-failable initializer otherwise.

With an error handler, failable initializers can be safely defined as "init!" which would preserve compatibility and likely annoy users as little as possible.

@ElvishJerricco
Copy link
Contributor Author

While I agree that implicitly unwrapped failable initializers would be the right choice, I'm not sure if an error handler is the way to go about it. What would the error handler really do?

@anpol
Copy link
Owner

anpol commented Dec 11, 2015

Default error handler could call assertionFailure() or preconditionFailure(). Another error handler could do nothing and allow control flow to proceed to failable initializer.

@anpol
Copy link
Owner

anpol commented Dec 11, 2015

One possible way to illustrate this:

typealias ErrorHandler = (String, StaticString, UInt) -> Void
var errorHandler: ErrorHandler = defaultErrorHandler

func defaultErrorHandler(message: String, file: StaticString, line: UInt) {
    preconditionFailure(message, file: file, line: line)
}

func invokeErrorHandler(message: String, file: StaticString = __FILE__, line: UInt = __LINE__) {
    errorHandler(message, file, line)
}

func dispatch_api_example() -> Int? { return nil }

struct Wrapper {
    let value: Int

    init!() {
        if let result = dispatch_api_example() {
            self.value = result
        } else {
            invokeErrorHandler("it happens")
            return nil
        }
    }
}

@ElvishJerricco
Copy link
Contributor Author

Alright that makes sense. I'd imagine we'd want the error handler to be a constructor parameter that defaults to the default error handler?

@anpol
Copy link
Owner

anpol commented Dec 12, 2015

No, I expect the global variable errorHandler, or even the static variable within the Dispatch struct.

More important is that I'm not sure we need failable initializers once we have error handler. I suspect it would be better to avoid failable initializers at all and stick to error handler mechanism.

@ElvishJerricco
Copy link
Contributor Author

I see a HUGE problem with that approach. If module A depends on B, and both A and B make use of DispatchKit, then only one of them can have an error handler.

Moreover, I'm not sure it makes sense that every usage of these methods should require the use of the same error handler. Shouldn't different scenarios call for different handling?

Also, I'm just not seeing why the error handler is necessary at all. If we want to generate some kind of runtime error, we can throw an ErrorType of some kind. This can be caught, handled case-by-case, and provides any necessary details to the catcher.

Finally, I don't see why any of this is better than just having a failable initializer. It'd be different if we were able to provide detailed error information. But if all we're doing is artificially producing an error and calling a handler or throwing or whatever, how is that different than a failable initializer? If the user wants to use some global handler, they can just surround these methods with their own

guard let x = DispatchType(args...) else {
    errorHandler()
}

Overall, it makes most sense to me to forego error handlers entirely in favor of idiomatic swift, with optionals.

@anpol anpol closed this as completed Dec 12, 2015
@anpol anpol reopened this Dec 12, 2015
@anpol
Copy link
Owner

anpol commented Dec 12, 2015

OK then, every initializer that wraps GCD API should be defined as init!, what's special about DispatchData initializers?

BTW, what do you think about eliminating duplicate data members like DispatchQueue.queue in favor of DispatchQueue.rawValue?

@ElvishJerricco
Copy link
Contributor Author

I just mentioned DispatchData because it has a lot of different initializers and methods which call underlying dispatch methods.

@ElvishJerricco
Copy link
Contributor Author

As for eliminating duplicates, I could be wrong, but I'm fairly sure that types that implement a protocol can't change the type of a computed property to a subtype.

Although I supposed it could be genericized.

protocol DispatchObject {
    typealias RawType: dispatch_object_t
    public var rawValue: RawValue

    ...
}

@anpol
Copy link
Owner

anpol commented Dec 12, 2015

It seems promising. Then we are to deprecate init(raw:) too.

The summary of changes proposed so far:

protocol _DispatchObject {
    typealias RawValue : dispatch_object_t
    var rawValue : RawValue { get }
}

struct _DispatchQueue : _DispatchObject {
    @available(*, unavailable, renamed="rawValue")
    var queue: dispatch_queue_t { return rawValue }

    @available(*, unavailable, renamed="_DispatchQueue(rawValue:")
    init!(raw value: dispatch_queue_t) { return nil }

    let rawValue: dispatch_queue_t

    init(rawValue: dispatch_queue_t) {
        self.rawValue = rawValue
    }

    init!() {
        if let queue = dispatch_queue_create(nil, nil) {
            self.rawValue = queue
        } else {
            return nil
        }
    }
}

@ElvishJerricco
Copy link
Contributor Author

Looks good to me. And of course introducing the same change to the other Dispatch types.

@ElvishJerricco
Copy link
Contributor Author

Do you want me to work on a pull request for this? Or have you already started?

@anpol
Copy link
Owner

anpol commented Dec 12, 2015

Actually, I will not be able to actively maintain this until Dec 22, at least.
I would be happy if you'll get this done.

@ElvishJerricco
Copy link
Contributor Author

No problem. I'll get right on it.

ElvishJerricco added a commit to ElvishJerricco/DispatchKit that referenced this issue Dec 13, 2015
Plenty of refactoring involved.
ElvishJerricco added a commit to ElvishJerricco/DispatchKit that referenced this issue Dec 14, 2015
…t properties.

The list of classes below have all been modified in the same way. Each of them had one initializer init(raw:), and initialized the class by assigning its dispatch_object_t subtype property to raw. These initializers have been renamed to init(rawValue:). This is because DispatchObject now requires a get-able property named rawValue whose type is a subtype of dispatch_object_t. These classes each had a property which represented what is now called rawValue. The old properties and initializers have been deprecated in favor of the new ones.

Other initializers have also been modified to be failable (implicitly unwrapped), due to the fact that they would call dispatch_create_X functions which returned implicitly unwrapped optionals. Having failable initializers allows for the ease of use that is expected when these functions are expected not to fail, along with the added ability to handle failure should it occur.

* DispatchData
* DispatchGroup
* DispatchIO
* DispatchQueue
* DispatchSemaphore
* DispatchSource
@ElvishJerricco ElvishJerricco mentioned this issue Dec 14, 2015
@anpol anpol closed this as completed in 9de4dc4 Dec 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants