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

Add missing `@escaping` for delegate-closures #1951

Merged
merged 1 commit into from Feb 27, 2017

Conversation

Projects
None yet
3 participants
@djmadcat
Contributor

djmadcat commented Feb 7, 2017

No description provided.

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Feb 26, 2017

Member

Hi @djmadcat,

Thanks for putting this PR together...much appreciated! Unfortunately though, we won't be accepting this set of changes at this time. The @escaping was left out of the property declarations on purpose because closures captured as properties are @escaping by default. As far as I can tell, @escaping and @nonescaping should only be used in function parameter declarations.

If you could point us to some documentation that states otherwise, then we'll be happy to reconsider this change.

Thanks anyways! 🍻

Member

cnoon commented Feb 26, 2017

Hi @djmadcat,

Thanks for putting this PR together...much appreciated! Unfortunately though, we won't be accepting this set of changes at this time. The @escaping was left out of the property declarations on purpose because closures captured as properties are @escaping by default. As far as I can tell, @escaping and @nonescaping should only be used in function parameter declarations.

If you could point us to some documentation that states otherwise, then we'll be happy to reconsider this change.

Thanks anyways! 🍻

@cnoon cnoon closed this Feb 26, 2017

@cnoon cnoon self-assigned this Feb 26, 2017

@djmadcat

This comment has been minimized.

Show comment
Hide comment
@djmadcat

djmadcat Feb 26, 2017

Contributor

@cnoon: closures captured as properties are @escaping by default

None of any properties were marked with @escaping (such as sessionDidReceiveChallengeWithCompletion), but parameters in them.

And for parameters there is implemented in Swift 3 proposal "Make non-escaping closures the default" :3

Proof №1.

Generated Objective-C code for dataTaskDidReceiveResponseWithCompletion property:

@property (nonatomic, copy) void (^ _Nullable dataTaskDidReceiveResponseWithCompletion)(NSURLSession * _Nonnull, NSURLSessionDataTask * _Nonnull, NSURLResponse * _Nonnull, SWIFT_NOESCAPE void (^ _Nonnull)(enum NSURLSessionResponseDisposition));

You can see SWIFT_NOESCAPE in closure parameter declaration.
This doesn't conforms to URLSessionDataDelegate protocol method optional public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void)

Proof №2

Create project that uses Alamofire and place this code instead of AppDelegate boilerplate.

import UIKit
import Alamofire

@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate, URLSessionDataDelegate {
    var window: UIWindow?

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        self.testAlamofireEscaping()

        return true
    }

    func testAlamofireEscaping() {
        let sessionDelegate = SessionDelegate()
        sessionDelegate.dataTaskDidReceiveResponseWithCompletion = self.urlSession
        let _ = SessionManager(configuration: URLSessionConfiguration.default, delegate: sessionDelegate, serverTrustPolicyManager: nil)
    }

    // MARK: - URLSessionDataDelegate

    func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void) {
        completionHandler(.allow)
    }
}

As we knows Swift allows to use instance methods as closures.
I have custom implementation of URLSessionDataDelegate.
But compiler throws error: Cannot assign value of type '(URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> ()' to type '((URLSession, URLSessionDataTask, URLResponse, (URLSession.ResponseDisposition) -> Void) -> Void)?'
With fixed version of open var dataTaskDidReceiveResponseWithCompletion: ((URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> Void)? it doesn't throw any error.

Proof №3

@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate, URLSessionDataDelegate {
    var window: UIWindow?

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        self.testAlamofireEscaping()

        return true
    }

    var completionHandler: ((URLSession.ResponseDisposition) -> Void)!

    func testAlamofireEscaping() {
        let sessionDelegate = SessionDelegate()
        sessionDelegate.dataTaskDidReceiveResponseWithCompletion = { (session: URLSession, dataTask: URLSessionDataTask, response: URLResponse, completionHandler: ((URLSession.ResponseDisposition) -> Void)) in
            self.completionHandler = completionHandler
        }
        let _ = SessionManager(configuration: URLSessionConfiguration.default, delegate: sessionDelegate, serverTrustPolicyManager: nil)
    }

    // MARK: - URLSessionDataDelegate

    func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void) {
        completionHandler(.allow)
    }
}

As we see there is @escaping attribute for completionHandler parameter in URLSessionDataDelegate urlSession(_:dataTask:didReceive:completionHandler:) signature.
So I can copy this handler in any variable I want.
dataTaskDidReceiveResponseWithCompletion closure signature doesn't have such attribute. So I can't copy this handler anywhere.
Compiler throws recoverable error Assigning non-escaping parameter 'completionHandler' to an @escaping closure
After fixing-up it throws new error Cannot assign value of type '(URLSession, URLSessionDataTask, URLResponse, @escaping ((URLSession.ResponseDisposition) -> Void)) -> ()' to type '((URLSession, URLSessionDataTask, URLResponse, (URLSession.ResponseDisposition) -> Void) -> Void)?'

With fixed version of open var dataTaskDidReceiveResponseWithCompletion: ((URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> Void)? it doesn't throw any error.

Conclusion

I think that there MUST be @escaping in closure parameters.

Contributor

djmadcat commented Feb 26, 2017

@cnoon: closures captured as properties are @escaping by default

None of any properties were marked with @escaping (such as sessionDidReceiveChallengeWithCompletion), but parameters in them.

And for parameters there is implemented in Swift 3 proposal "Make non-escaping closures the default" :3

Proof №1.

Generated Objective-C code for dataTaskDidReceiveResponseWithCompletion property:

@property (nonatomic, copy) void (^ _Nullable dataTaskDidReceiveResponseWithCompletion)(NSURLSession * _Nonnull, NSURLSessionDataTask * _Nonnull, NSURLResponse * _Nonnull, SWIFT_NOESCAPE void (^ _Nonnull)(enum NSURLSessionResponseDisposition));

You can see SWIFT_NOESCAPE in closure parameter declaration.
This doesn't conforms to URLSessionDataDelegate protocol method optional public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void)

Proof №2

Create project that uses Alamofire and place this code instead of AppDelegate boilerplate.

import UIKit
import Alamofire

@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate, URLSessionDataDelegate {
    var window: UIWindow?

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        self.testAlamofireEscaping()

        return true
    }

    func testAlamofireEscaping() {
        let sessionDelegate = SessionDelegate()
        sessionDelegate.dataTaskDidReceiveResponseWithCompletion = self.urlSession
        let _ = SessionManager(configuration: URLSessionConfiguration.default, delegate: sessionDelegate, serverTrustPolicyManager: nil)
    }

    // MARK: - URLSessionDataDelegate

    func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void) {
        completionHandler(.allow)
    }
}

As we knows Swift allows to use instance methods as closures.
I have custom implementation of URLSessionDataDelegate.
But compiler throws error: Cannot assign value of type '(URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> ()' to type '((URLSession, URLSessionDataTask, URLResponse, (URLSession.ResponseDisposition) -> Void) -> Void)?'
With fixed version of open var dataTaskDidReceiveResponseWithCompletion: ((URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> Void)? it doesn't throw any error.

Proof №3

@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate, URLSessionDataDelegate {
    var window: UIWindow?

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        self.testAlamofireEscaping()

        return true
    }

    var completionHandler: ((URLSession.ResponseDisposition) -> Void)!

    func testAlamofireEscaping() {
        let sessionDelegate = SessionDelegate()
        sessionDelegate.dataTaskDidReceiveResponseWithCompletion = { (session: URLSession, dataTask: URLSessionDataTask, response: URLResponse, completionHandler: ((URLSession.ResponseDisposition) -> Void)) in
            self.completionHandler = completionHandler
        }
        let _ = SessionManager(configuration: URLSessionConfiguration.default, delegate: sessionDelegate, serverTrustPolicyManager: nil)
    }

    // MARK: - URLSessionDataDelegate

    func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse, completionHandler: @escaping (URLSession.ResponseDisposition) -> Swift.Void) {
        completionHandler(.allow)
    }
}

As we see there is @escaping attribute for completionHandler parameter in URLSessionDataDelegate urlSession(_:dataTask:didReceive:completionHandler:) signature.
So I can copy this handler in any variable I want.
dataTaskDidReceiveResponseWithCompletion closure signature doesn't have such attribute. So I can't copy this handler anywhere.
Compiler throws recoverable error Assigning non-escaping parameter 'completionHandler' to an @escaping closure
After fixing-up it throws new error Cannot assign value of type '(URLSession, URLSessionDataTask, URLResponse, @escaping ((URLSession.ResponseDisposition) -> Void)) -> ()' to type '((URLSession, URLSessionDataTask, URLResponse, (URLSession.ResponseDisposition) -> Void) -> Void)?'

With fixed version of open var dataTaskDidReceiveResponseWithCompletion: ((URLSession, URLSessionDataTask, URLResponse, @escaping (URLSession.ResponseDisposition) -> Void) -> Void)? it doesn't throw any error.

Conclusion

I think that there MUST be @escaping in closure parameters.

@cnoon cnoon reopened this Feb 27, 2017

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Feb 27, 2017

Member

Now that's what I call a justification @djmadcat! I really appreciate the thoroughness of that reply, not to mention I learned a thing or two along the way...much appreciated. 🍻

Member

cnoon commented Feb 27, 2017

Now that's what I call a justification @djmadcat! I really appreciate the thoroughness of that reply, not to mention I learned a thing or two along the way...much appreciated. 🍻

@cnoon cnoon added this to the 4.5.0 milestone Feb 27, 2017

@cnoon cnoon merged commit b03b43c into Alamofire:master Feb 27, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment