Feature request: Adding Generics parameter in DelegateProxy

tarunon commented 7 years ago

Hello, This proposal is about improving DelegateProxy type supports.

DelegateProxy had weak type support, so we needed to use many force casting when implement DelegateProxy subclass. This proposal goal is adding Generics type in DelegateProxy, and removing unnecessary force casting from implementation of DelegateProxy subclass. This proposal will make many code differences and API changes, I hope to discussion it targeting Swift4 and RxSwift4.0.

Plan

Adding associatedtype on DelegateProxyType and change DelegateProxy implementation like

public protocol DelegateProxyType : AnyObject {
    associatedtype ParentObject: AnyObject
    associatedtype Delegate: AnyObject
...
} 
open class DelegateProxy<P: AnyObject, D: AnyObject>: _RXDelegateProxy, DelegateProxyType { 
    public typealias ParentObject = P
    public typealias Delegate = D
...
}

 Implements DelegateProxy subclass is specify Generics parameter like 

 public class RxScrollViewDelegateProxy
    : DelegateProxy<UIScrollView, UIScrollViewDelegate>
    , UIScrollViewDelegate {
...
}

I try to this way in my fork, It's seems work well.

Pros

DelegateProxy is now support DelegateProxyType, It mean can remove some of force casting and type assertion from internal implementation of DelegateProxy.
We don't need to write many force casting anymore when we implement DelegateProxy subclass.
installForwardDelegate is safer than before, Delegate type will be check in compile time.

Cons

It's unfortunately we still need to write force casting when we implement DelegateProxy subclasses subclass.
DelegateProxy has a Generics type, then we cannot use DelegateProxy at extension function return type because of Swift 3.0 don't support overriding function that has generics type on extension. I use Delegate type instead of DelegateProxy for this workaround. In future swift, maybe this workaround will be unnecessary.
DelegateProxy is abstract class.

kzaher commented 7 years ago

Hi @tarunon ,

I've tried to figure out some more "nicer" typesafer solution a while ago, but it didn't work out.

I've glanced at your suggestion, but there are tons of changes, so please correct me if I'm wrong.

I would expect the general solution to have this form.

Type1: Type2: Type3 .... NSObject
DelegateProxy<Type1>: DelegateProxy<Type2>: DelegateProxy<Type3>: ... NSObject

where ...

class DelegateProxy<Type1>: DelegateProxy {
    associatedtype ParentType = Type1
}

class DelegateProxy<Type2>: DelegateProxy {
    associatedtype ParentType = Type2
}

class DelegateProxy<Type3>: DelegateProxy {
    associatedtype ParentType = Type3
}

it seems to me that you've refactored the code to only use one level?

class DelegateProxy<RootType>: DelegateProxy {
    associatedtype ParentType = RootType
}

The problems I've had were that I couldn't have class hierarchies redeclaring conformance to DelegateProxy protocol.

It seems to me that we now have another more pressing issue we first need to solve.

https://github.com/ReactiveX/RxSwift/pull/1282#discussion-diff-122204108L175

tarunon commented 7 years ago

Hello @kzaher

If my understanding is correct, your point is true. My refactoring can only support RootParentObject/RootDelegate. There are two reason, 1 is a feature of swift language, 2 is a usage of DelegateProxy generics parameter. For example, scrollView.rx.delegate type will be DelegateProxy<UIScrollView, UIScrollViewDelegate> in my refactoring. And if I hope to change generics parameter in subclass, then I will get DelegateProxy<UITableView, UITableViewDelegate> in tableView, I cannot resolve this type differences.

Actually swift does not support covariance of generics parameter. Array and Optional have a covarianced generics parameter, but its specific support by compiler. In DelegateProxy, ParentObject is covariance parameter, because it's only used in read only property. But Delegate is not covariance, because it's used in function argument, and return value. Usually, we should use map function when we hope to change like type of DelegateProxy. But few days ago, I couldn't implement DelegateProxy.map. In my opinion, my refactoring is good way that considering current swift feature and DelegateProxy implementation.

BTW, I have a just idea of https://github.com/ReactiveX/RxSwift/pull/1282#discussion-diff-122204108L175 Maybe we can use factory pattern in this case, I referred URLProtocol pattern.

public final class DelegateFactory<ParentObject: AnyObject, Delegate: NSObjectProtocol> {
    public typealias Create = (ParentObject) -> Delegate?
    let _create: Create
    public init(create: @escaping Create) {
        self._create = create
    }

    func create(for parentObject: ParentObject) -> Delegate? {
        return _create(parentObject)
    }

    public static func create(_ factories: [DelegateFactory], for parentObject: ParentObject) -> Delegate {
        return factories.reversed().reduce(Delegate?.none) { $0 ?? $1.create(for: parentObject) }!
    }
}

public class RxScrollViewDelegateProxy
    : DelegateProxy<UIScrollView, UIScrollViewDelegate>
    , UIScrollViewDelegate {
    static var factories: [DelegateFactory<UIScrollView, UIScrollViewDelegate>] = [
        .init { RxScrollViewDelegateProxy(parentObject: $0) },
        .init { ($0 as? UITableView).map { RxTableViewDelegateProxy(parentObject: $0) } },
        .init { ($0 as? UICollectionView).map { RxCollectionViewDelegateProxy(parentObject: $0) } },
        .init { ($0 as? UITextView).map { RxTextViewDelegateProxy(parentObject: $0) } }
    ]

    public class func register(factory: DelegateFactory<UIScrollView, UIScrollViewDelegate>) {
        factories.append(factory)
    }

    public override class func createProxyForObject(_ object: UIScrollView) -> UIScrollViewDelegate {
        return DelegateFactory<UIScrollView, UIScrollViewDelegate>.create(factories, for: object)
    }
...

Some guy who want to extend RxScrollViewDelegateProxy can call register function.

RxScrollViewDelegateProxy.register(.init { /*making own delegate proxy*/ })

kzaher commented 7 years ago

If my understanding is correct, your point is true. My refactoring can only support RootParentObject/RootDelegate. There are two reason, 1 is a feature of swift language, 2 is a usage of DelegateProxy generics parameter. For example, scrollView.rx.delegate type will be DelegateProxy<UIScrollView, UIScrollViewDelegate> in my refactoring. And if I hope to change generics parameter in subclass, then I will get DelegateProxy<UITableView, UITableViewDelegate> in tableView, I cannot resolve this type differences.

I suspected that :)

Actually swift does not support covariance of generics parameter. Array and Optional have a covarianced generics parameter, but its specific support by compiler. In DelegateProxy, ParentObject is covariance parameter, because it's only used in read only property.

But Delegate is not covariance, because it's used in function argument, and return value.

If Array has covariant parameter Element, then probably Delegate should also be covariant, but yes, compiler support sucks.

Usually, we should use map function when we hope to change like type of DelegateProxy. But few days ago, I couldn't implement DelegateProxy.map. In my opinion, my refactoring is good way that considering current swift feature and DelegateProxy implementation.

I would like to really think through before doing anything like this.

Let's maybe first focus on solving this

BTW, I have a just idea of https://github.com/ReactiveX/RxSwift/pull/1282#discussion-diff-122204108L175

and then see if we can improve something else.

Maybe we can use factory pattern in this case, I referred URLProtocol pattern.

Yeah, that seems like a reasonable approach, we would just need to figure out scope for this registry.

Maybe only one registry for all delegate proxies (separated by class) would be better, because this way we don't have to duplicate the registration mechanism, but yeah, we'll need to do something like this.

tarunon commented 7 years ago

@kzaher I also think it's need to fix #1282 problem first, and next, I would like to make proposal of DelegateProxy with generics parameter again. I have an idea of DelegateProxy.map now, maybe it's work well because there are no extension function in ParentObject class anymore.

kzaher commented 7 years ago

Hi @tarunon ,

could you maybe first create delegate factory and make a PR that targets swift4.0 branch? Or I can also do that part if you aren't interested.

tarunon commented 7 years ago

Hi @kzaher , Of course I'll do it. Please wait a this weekend. XD

tarunon commented 7 years ago

Hi there, I have investigated in few days, finally, I found that unsafeDownCast work well. Let's think about DelegateProxy<ParentObject, Delegate>, In this case, RxScrollViewDelegateProxy will be

class RxScrollViewDelegateProxy<P: UIScrollView>: DelegateProxy<P, UIScrollViewDelegate>, UIScrollViewDelegate {
...

And RxTableViewDelegateProxy will be

class RxTableViewDelegateProxy<P: UITableView>: RxScrollViewDelegateProxy<P>, UITableViewDelegate {
...

There were problem of we cannot make cast between RxScrollViewDelegateProxy and RxTableViewDelegateProxy. unsafeDownCast can solve this problem, because the generics parameter is always class, and it mean a same of memory size. So the generics parameter is Fantom type effectively.

Of course, there is also a good possibility that map implementation is better than using unsafeDownCast, but I cannot make map without public MapDelegateProxy.

GabrielAraujo commented 6 years ago

Guys,

I'm trying to implement RxSwift 4.0 into the RxGoogleMaps from RxSwiftCommunity repo..

But I'm stuck in this DelegateProxy part.. The code is always breaking at rxFatalError("DelegateProxy has no factory of \(object). Implement DelegateProxy subclass for \(object) first.")

This is the Delegate Proxy: public class RxGMSMapViewDelegateProxy : DelegateProxy<RxGMSMapView, RxGMSMapViewDelegate> , RxGMSMapViewDelegate , DelegateProxyType {

The project is at https://github.com/GabrielAraujo/RxGoogleMaps

Do you guys have any idea why the registration isn't working?

tarunon commented 6 years ago

@GabrielAraujo Hi, I try to reproduce the problem, but seems fine, Example app work well. Could you please explain your environment?

ReactiveX / RxSwift