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

Proposed fix to send_super #108

Merged
merged 7 commits into from May 1, 2018
Merged

Conversation

@cculianu
Copy link
Contributor

cculianu commented Mar 10, 2018

Related: Issue #107

I'm not sure if my approach is the most elegant.. but at least it has the same semantics as what obj-c does for you at compile-time.

The idea is the caller tells send_super where to start looking, which 100% emulates what the compiler does in obj-c for you.

If they omit the keyword, they get the (possibly incorrect) automatic behavior we had before.

Not sure how elegant this is though. Maybe we can do better. But this is better than nothing IMHO.

Let me know what you think!

Tested on the following code and it works like a charm:

Code:

class MyA(NSObject):
    
    aProp = objc_property()
    
    @objc_method
    def init(self) -> ObjCInstance:
        #self = ObjCInstance(send_super(self, 'init', superclass='NSObject'))
        self = ObjCInstance(send_super(__class__, self, 'init'))
        print ("MyA init")
        self.aProp = "MyA Property"
        return self
    
    @objc_method
    def dealloc(self) -> None:
        print ("MyA dealloc")
        self.aProp = None
        #send_super(self, 'dealloc', superclass='NSObject')     
        send_super(__class__, self, 'dealloc')     

class MyB(MyA):
    
    bProp = objc_property()
    
    @objc_method
    def init(self) -> ObjCInstance:
        #self = ObjCInstance(send_super(self, 'init', superclass='MyA'))
        self = ObjCInstance(send_super(__class__, self, 'init'))
        print ("MyB init")
        self.aProp = "MyB Property, overwritten"
        self.bProp = "MyB's own property"
        return self
    
    @objc_method
    def dealloc(self) -> None:
        print ("MyB dealloc")
        self.bProp = None
        #send_super(self, 'dealloc', superclass='MyA')
        send_super(__class__, self, 'dealloc')

# ... then, somewhere in a function or whatever ... 

    a=MyB.new().autorelease()
    print("MyB propA=%s propB=%s"%(str(a.aProp),str(a.bProp)))



Output:

MyA init
MyB init
MyB propA=MyB Property, overwritten propB=MyB's own property
MyB dealloc
MyA dealloc

EDIT: Updated this comment to reflect changed API as discussed below (so that someone googling has good usage to look at in front of their eyes rather than non-existant usage).

cculianu added 2 commits Mar 10, 2018
…stead of awkward get_class call
@dgelessus

This comment has been minimized.

Copy link
Collaborator

dgelessus commented Mar 10, 2018

I think this is the right approach - while I agree that explicitly passing in the superclass isn't very pretty, it's the most straightforward way of implementing this. send_super isn't used very frequently either, so I don't think the extra typing is a big issue.

I have a couple of comments/questions though:

  • Any particular reason why superclass is the class name as a string? I saw that you originally made superclass be a Class pointer, but changed that to avoid needing to call get_class every time. Wouldn't it be easier to allow superclass to be either an ObjCClass or Class, and unwrap it if necessary? That's how receiver is handled (it can be an ObjCInstance or objc_id).
  • For symmetry with the long form of Python's super, I think it would be better if the extra send_super argument would be the current class, rather than its superclass. This would also make refactoring less error-prone - you don't need to change every send_super call when the superclass changes.
    • This would also allow us to make use of a somewhat obscure Python feature: all functions inside a Python class definition have access to a special variable named __class__, which is set to the class object that's currently being created. For example inside a class Foo block, __class__ is Foo. This means that all objc_super calls can be written as objc_super(self, "method", ..., cls=__class__) and they will work regardless of what the current class is called.
  • I think it would be a good idea to make the "current class" argument required, because without it send_super won't behave correctly. If we make use of __class__ as explained above, it wouldn't be a lot of extra typing work: send_super(self, "method") becomes send_super(__class__, self, "method").
    • If we do this, old code that doesn't pass __class__ will stop working - either because of too few arguments, or because receiver ends up being a string (the method name) due to the parameters shifting. I think it's okay to break backwards compatibility here - Rubicon is not stable yet, send_super is not used very often, and the old behavior is broken. We can even detect "old-style" calls that are missing the class, by checking whether receiver is a string, and throw an error with a helpful message in that case.

PS: Quick note - if you use __class__ inside an Objective-C subclass on Python 3.6, you'll get a DeprecationWarning. This has something to do with how our ObjCClass.__new__ is written. I've looked into it and already know how to fix it, so don't worry about the warning. (Relevant docs.)

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 11, 2018

First, your question:

Any particular reason why superclass is the class name as a string? I saw that you originally made superclass be a Class pointer, but changed that to avoid needing to call get_class every time. Wouldn't it be easier to allow superclass to be either an ObjCClass or Class, and unwrap it if necessary? That's how receiver is handled (it can be an ObjCInstance or objc_id).

Sure, I agree we should support that. Although I see advantages to supporting a string too (less typing!). In fact -- I think you'd be ok with that given your __class__ idea. (I figured a string was easier to type, and I noticed 'get_class' isn't exported to the rubicon.objc namespace by default, and I wanted to be minimally intrusive with this change... so that was the rationale for going with the string).

As for the rest of your comment:

I didn't know about __class__. Nice.

Oh man! I love this idea! I was super bothered by the fact that manually typing the classname for superclass= is very very bug prone. This is doubly true if you go around changing the inheritance heirarchy. You have to also remember to change all calls to send_super. I couldn't figure out how else to lexically scope this. Turns out Python has this __class__ keyword which is lexically scoped perfectly for us!

I love this approach that you propose and can see only benefit to it.

It's true, it breaks existing calls. If we issue a friendly message, as you say, all calls can quickly be found and corrected.

I'm all for this approach you propose. It fixes the potential maintainability nightmare of having subtle errors because you changed your object hierarchy but missed the odd "send_super" call.

In ObjC, even though they don't use that term in their docs -- 'super' is lexically scoped. We need a lexically scoped mechanism to get it right. Enter the __class__ pseudo-keyword in Python which is just that. I love it.

So send_super isn't used much eh? Ha -- I use it all the time. But I can see how not everyone is crazy enough to actually inherit from UIKit classes in Python in their iOS project like I do. :) I noticed the Toga framework uses an entirely different pattern to do its magic, for example.

I'll go ahead and change this PR to do what you say (I have to do a bit of digging on how to get from the 'class' to its superclass but I trust ObjC has easy methods to do it -- if not runtime.py having helpers to do it already).

Thanks so much for your very thoughtful guidance on this! I'm really glad this bug is getting fixed!

I also noticed you just made a PR that addresses a potential python warning related to this. Thanks! 👍

…nd better and more Pythonesque
@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 11, 2018

Ok, done. Let me know what you think!

Also note that flake8 on my system flaked (heh) on the __class__ built-in. It complained it doesn't exist. When it does -- code runs fine and Python does the right thing. Likely a bug in flake8. Not sure how to handle this.. :/

…tests worked anyway. Spooky. Also, fixed the tests to work with new send_super API
@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 11, 2018

Yeah, tests got that DeprecatedWarning, plus flake8.. barfs on __class__.. how to fix? (The flake8 warning, that is) :(

@dgelessus

This comment has been minimized.

Copy link
Collaborator

dgelessus commented Mar 11, 2018

The __class__ issue has been fixed in pyflakes already (PyCQA/pyflakes#326 typo, I meant PyCQA/pyflakes#325), but there hasn't been a release containing the fix yet. I've updated #109 to change our flake8 test environment to use a development version of pyflakes, that way it will understand __class__. So once that PR is merged, the tests should run again.

if not isinstance(cls, ObjCClass):
if inspect.isclass(cls):
# __class__ was passed-in, convert to str for obj-c lookup
cls = cls.__name__

This comment has been minimized.

Copy link
@dgelessus

dgelessus Mar 11, 2018

Collaborator

This check isn't necessary; __class__ is an ObjCClass object if you use it inside an ObjCClass:

>>> from rubicon import objc
>>> class Foo(objc.NSObject):
...     @objc.objc_method
...     def foo(self) -> None:
...         print(type(__class__), __class__)
...         
>>> Foo.alloc().init().foo()
<class 'rubicon.objc.runtime.ObjCClass'> ObjCClass('Foo')

This comment has been minimized.

Copy link
@cculianu

cculianu Mar 11, 2018

Author Contributor

Whaa.. ok. Yeah that makes sense. You Python-Fu is strong. Much respect.

# __class__ was passed-in, convert to str for obj-c lookup
cls = cls.__name__
# Convert class name/ptr argument to an ObjCClass (from either a name or a naked Class pointer)
cls = ObjCClass(cls) # raises an exception if class is not found by name and/or pointer was nil

This comment has been minimized.

Copy link
@dgelessus

dgelessus Mar 11, 2018

Collaborator

Since cls is only needed for its superclass, I think it would be better to leave it as a raw Class and use the get_superclass_of_class function to get its superclass.

I'm honestly surprised that using ObjCClass here is possible at all, without causing any bootstrapping issues - usually you cannot (unconditionally) use the high-level ObjCInstance and ObjCClass inside send_message and send_super, because these functions are used in the implementation of ObjCInstance.

This comment has been minimized.

Copy link
@cculianu

cculianu Mar 11, 2018

Author Contributor

Ok, so basically grab the cls.ptr and use that, got it. No messing about with strings or other "smart" stuff. Only complain if passed-in cls is not an ObjCClass, right?

Got it!

It basically instructs this runtime what the lexical scope was of the caller, so that it
may find the appropriate superclass method implementation (if any) to call. (See issue #107)
As such, cls can be a string, which is the class name of the lexical scope of the caller,

This comment has been minimized.

Copy link
@dgelessus

dgelessus Mar 11, 2018

Collaborator

I'm still not sure when using a string for cls would be needed. In all normal use cases, you'll pass __class__ as the first argument. If for whatever reason that doesn't work, you can use ObjCClass explicitly to get a class with a specific name.

This comment has been minimized.

Copy link
@cculianu

cculianu Mar 11, 2018

Author Contributor

Yeah you're right. Will fix.

Man your Python Kung Fu is strong. Thanks for taking the time to explain this to me. Like I said, I'm a C++/C/Obj-C guy -- Python is relatively new to me (really only learned it properly for this project I'm working on).

Very cool! Thanks!

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 11, 2018

Question: I noticed this in the code in class ObjCClass:

    def __instancecheck__(self, instance):
        if isinstance(instance, ObjCInstance):
            return bool(instance.isKindOfClass(self))
        else:
            return False

    def __subclasscheck__(self, subclass):
        if isinstance(subclass, ObjCClass):
            return bool(subclass.isSubclassOfClass(self))
        else:
            raise TypeError(
                'issubclass(X, {self!r}) arg 1 must be an ObjCClass, not {tp.__module__}.{tp.__qualname__}'
                .format(self=self, tp=type(subclass))
            )

Note the isSubclassofClass and isKindOfClass calls into objc missing the trailing _. Is this a feature of rubicon-objc I'm unaware of or is this an oversight/bug?

@dgelessus

This comment has been minimized.

Copy link
Collaborator

dgelessus commented Mar 11, 2018

That is indeed a feature. Rubicon lets you use two different styles for calling methods with arguments. For example, if you want to call the method -[NSMutableArray replaceObjectsInRange:withObjectsFromArray:range], you can use either of the following two method calls:

  • arr1.replaceObjectsInRange_withObjectsFromArray_range_(range1, arr2, range2)
  • arr1.replaceObjectsInRange(range1, withObjectsFromArray=arr2, range=range2)

The first style is what you're probably used to - the Python method name is the Objective-C method name with all colons replaced with underscores.

The second style tries to mimic Objective-C's method call syntax a little, by placing the method arguments "in between" the parts of the method name. The first method name part becomes the Python method name, the first argument is passed as a positional argument (right after the opening paren), and all further method name parts and arguments become Python keyword arguments. It's a bit lengthy to explain, it's probably easier to understand by looking at the example code above.

When you call a single-argument method and leave off the underscore at the end, you're actually using the second method call style - but because the method only takes a single argument, the call doesn't use any keyword arguments.

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 11, 2018

Wow.. you just blew my miiiind.

Yeah, I get it. Clever that you support both styles. The second style is more "objective-c" ish. Also kind of more Pythonesque if you think about it. HA! Didn't know that. Will have to read that relevant part of runtime.py to get a sense for how the trickery is done.

Awesome! Thanks for explaining that. 👍

@cculianu cculianu changed the title Proposed fix to send_super issue discussed in issue tracker #107 Proposed fix to send_super Mar 11, 2018
"""
if isinstance(cls, Class) or type(cls) is ObjCClass:
pass
else:

This comment has been minimized.

Copy link
@dgelessus

dgelessus Mar 11, 2018

Collaborator

Minor thing: this could be written as if not isinstance(cls, (Class, ObjCClass)), right?

raise TypeError("Missing/Invalid cls argument: '{tp.__module__}.{tp.__qualname__}' -- "
.format(tp=type(cls))
+ "send_super now requires its first argument be an"
+ " ObjClass or an objc raw Class pointer."

This comment has been minimized.

Copy link
@dgelessus

dgelessus Mar 11, 2018

Collaborator

Typo: ObjClass should be ObjCClass.

Copy link
Collaborator

dgelessus left a comment

👍 Looks good to me. Once #109 is merged, the CI build will run again, and then this can be merged.

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Mar 12, 2018

Awesome. Can't wait. Thank you so much for everything man! 👍

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Apr 29, 2018

Hey man -- You been busy? I noticed neither this PR nor the one fixing the deprecated warnings got merged. Just was wondering if you've been busy or what's up?

Best regards,

-Calin

@dgelessus

This comment has been minimized.

Copy link
Collaborator

dgelessus commented Apr 30, 2018

Yep, things have been somewhat busy lately, so I wasn't able to work much on Rubicon for a few weeks - sorry about that.

The reason why this PR couldn't be merged yet is because it requires #109 for the CI build to pass, and that PR couldn't be merged yet because it hasn't been approved yet (because we have required reviews enabled). @freakboy3742 Apologies for pinging you again, I know you're busy with PyCon preparations and such at the moment - but when you have time, could you have a look at #109? Thanks 😃

@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented Apr 30, 2018

No worries -- I was just eager to start pointing my sources at your repo again and not my own forked repo, is all.

Whenever you guys get around to it -- if you're busy with other stuff it's cool.

Thanks for the update!

-Calin

@freakboy3742

This comment has been minimized.

Copy link
Member

freakboy3742 commented Apr 30, 2018

@dgelessus Apologies for the delay reviewing #109; I've done that now, and kickstarted a new build (although now that I've done that... the #109 changes won't be included, so I don't know why I pushed that button... I'm chalking it up to being in transit for (checks watch) 35 hours... :-P)

@dgelessus

This comment has been minimized.

Copy link
Collaborator

dgelessus commented May 1, 2018

Well, the CI build passed anyway, go figure 😆 I'm not quite sure what's going on - the PR build already seems to be using the updated tox.ini somehow? In any case, since the only build failure before was the flake8 check, there should be no issues with merging this.

Thank you for the quick response @freakboy3742! That's quite a long trip, hope you'll arrive soon 😃

@dgelessus dgelessus merged commit 730e694 into beeware:master May 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cculianu

This comment has been minimized.

Copy link
Contributor Author

cculianu commented May 1, 2018

You guys are awesome. Thank you for everything (including guidance on getting this properly implemented).

-Calin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.