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

[SR-2804] URLProtocol.setProperty doesn't take URLRequest, instead requires NSMutableURLRequest #4324

Open
swift-ci opened this issue Sep 29, 2016 · 20 comments

Comments

@swift-ci
Copy link
Contributor

Previous ID SR-2804
Radar rdar://problem/31244651
Original Reporter andrewtheis (JIRA User)
Type Bug
Environment

Xcode 8 / Swift 3 / macOS Sierra / iOS 10

Additional Detail from JIRA
Votes 4
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: b6c1d5ebeca551022bac780f772ec9f8

cloned to:

  • SR-2805 Cast from value type to reference type subclass reported as "always fails"

Issue Description:

For example:

import Foundation

var request = URLRequest(url: URL(string: "https://apple.com")!)
URLProtocol.setProperty(true, forKey: "example", in: request)

This throws an error:

Cannot convert value of type 'URLRequest' to expected argument type 'NSMutableURLRequest'

Type casting circumvents the error:

import Foundation

let request = URLRequest(url: URL(string: "https://apple.com")!)

guard let mutableRequest = request as? NSMutableURLRequest else {
    exit(0)
}

URLProtocol.setProperty(true, forKey: "example", in: mutableRequest)

But then a warning is thrown:

Cast from 'URLRequest' to unrelated type 'NSMutableURLRequest' always fails

@belkadan
Copy link

Cloned out SR-2805 to fix the warning and possible mis-optimization.

@swift-ci
Copy link
Contributor Author

Comment by Mark Lilback (JIRA)

I'm not sure if this should be filed as a separate bug, but if you circumvent the error as described above, then

let value = URLProtocol.property(forKey: "example", in: request)

will return nil instead of true

@swift-ci
Copy link
Contributor Author

Comment by aaron crespo (JIRA)

Foundation documentation states:

The Swift overlay to the Foundation framework provides the URLRequest structure, which bridges to the NSMutableURLRequest class and its immutable superclass, NSURLRequest. The URLRequest value type offers the same functionality as the NSMutableURLRequest reference type, and the two can be used interchangeably in Swift code that interacts with Objective-C APIs. This behavior is similar to how Swift bridges standard string, numeric, and collection types to their corresponding Foundation classes.

Which is not the case with respect to NSURLConnectionDelegate and URLProtocol implementations.

This issue is still present in Xcode 8.3 beta's most recent release.

@belkadan
Copy link

@phausler, who's responsible for URLRequest? That documentation seems a little fishy.

@belkadan
Copy link

(but I'm not completely sure)

@phausler
Copy link
Member

Foundation owns the bridge part for it. "which bridges to the NSMutableURLRequest class" the bridge is only to NSURLRequest

@phausler
Copy link
Member

now it so happens that we create a mutable version under the hood but there is no adopter of _ObjectiveCBridgeable for NSMutableURLRequest.

The "safe" (but ugly) way to do this:

var request = URLRequest(url: URL(string: "https://apple.com")!)
URLProtocol.setProperty(true, forKey: "example", in: (request as.mutableCopy() as) NSMutableURLRequest)

@swift-ci
Copy link
Contributor Author

Comment by aaron crespo (JIRA)

One problem is that the cast to NSMutableRequest hides the mutation from the swift compiler so the following code produces a warning:

typical URLProtocol implementation

  override func startLoading() {
    var req = request
    URLProtocol.setProperty(true, forKey: NetworkActivityKeys.HandledKey, in: (req as NSURLRequest).mutableCopy() as! NSMutableURLRequest)
   //... Variable 'req' was never mutated; consider changing to 'let' constant
  }

Is the underlying NSMutableRequest created with let req = request? does it then become unsafe (IUO on cast failure)? The documentation says these may be used interchangeably

@phausler
Copy link
Member

Oh no that isn't doing what you were wanting, sorry.

This is probably more along the lines of what you want:

        var request = URLRequest(url: URL(string: "https://apple.com")!)
        let mutableRequest = (request as NSURLRequest).mutableCopy() as! NSMutableURLRequest
        URLProtocol.setProperty(true, forKey: "example", in: mutableRequest)
        request = mutableRequest as URLRequest

@phausler
Copy link
Member

we should refine the URLProtocol implementation, that is really disgusting.

just a potential change (needs to go through API review etc) that would have to be made in the Foundation overlay

extension URLProtocol {
    public class func setProperty(_ value: Any, forKey key: String, in request: inout URLRequest) {
        request._applyMutation {
            URLProtocol.setProperty(value, forKey: key, in: $0)
        }
    }

    public class func removeProperty(forKey key: String, in request: inout URLRequest) {
        request._applyMutation {
            URLProtocol.removeProperty(value, forKey: key, in: $0)
        }
    }
}

@swift-ci
Copy link
Contributor Author

Comment by aaron crespo (JIRA)

Yeah the ugliness was already tucked away hidden in a helper function but I figured I would make sure some ugly bridging behavior was pointed out somewhere.

@phausler
Copy link
Member

The problem with doing it as the work-around is that it is causing four copies where it should at most do one (and perhaps zero in the common case)

@swift-ci
Copy link
Contributor Author

Comment by aaron crespo (JIRA)

is API review referring to "corelib" or "evolution"?

@phausler
Copy link
Member

It is in reference to the internal process for the Cocoa and software development teams maintaining the implementations backing that where we will follow a similar review process to swift-evolution. Primarily this is to get eyes on APIs for the stakeholders that will be responsible for supporting any API changes.

@swift-ci
Copy link
Contributor Author

Comment by aaron crespo (JIRA)

Cool was curious if there was something someone outside could do to help this along.

@swift-ci
Copy link
Contributor Author

swift-ci commented Apr 7, 2017

Comment by Andrea de Marco (JIRA)

Hi,
I guess the issue I found is related to this one:

let request = URLRequest(url: URL(string: "https://apple.com")!)
URLProtocol.setProperty("bar", forKey: "foo", in: request as! NSMutableURLRequest)
let foo = URLProtocol.property(forKey: "foo", in: request)
//foo is nil

Isn't it?

@phausler
Copy link
Member

phausler commented Apr 7, 2017

That is the same failure, you cannot cast a structural type to a mutable type since they are not bridged. But you can cast a mutable to a structural type because the super-class is bridgeable. So your `as!` will halt.

@swift-ci
Copy link
Contributor Author

swift-ci commented Apr 7, 2017

Comment by Andrea de Marco (JIRA)

On Xcode 8.3 my snippet works, but with an unexpected result (nil).

I think the problem is quite different, becoming URLRequest a structure the property inside the request is added to one of its copy.

To be clear: until this fix, the URLProtocol property mechanins isn't usable. Right?

@phausler
Copy link
Member

phausler commented Apr 7, 2017

It is usable but you would need to bridge back out to the mutable reference and back;

via the snippet I posted earlier

        var request = URLRequest(url: URL(string: "https://apple.com")!)
        let mutableRequest = (request as NSURLRequest).mutableCopy() as! NSMutableURLRequest
        URLProtocol.setProperty(true, forKey: "example", in: mutableRequest)
        request = mutableRequest as URLRequest

@lilyball
Copy link
Mannequin

lilyball mannequin commented Jul 30, 2018

I've run into this issue multiple times now. It would be great to see it fixed. The obvious solution here is to extend URLProtocol to have overloads for the setProperty/removeProperty methods that work on inout URLRequest.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants