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

pyobjc==10.3 breakage #610

Open
justvanrossum opened this issue Jun 5, 2024 · 24 comments
Open

pyobjc==10.3 breakage #610

justvanrossum opened this issue Jun 5, 2024 · 24 comments
Labels
bug Something isn't working

Comments

@justvanrossum
Copy link

justvanrossum commented Jun 5, 2024

Describe the bug

There's a regression for a code pattern like this:

import AppKit

class TestClass(AppKit.NSObject):

    def __new__(cls):
        print("in __new__")
        return cls.alloc().init()

    def __init__(self):
        print("in __init__")


inst = TestClass()

With pyobjc==10.2 this outputs this:

in __new__
in __init__

However, with pyobjc==10.3, __init__ doesn't get called, and this is the output:

in __new__
@justvanrossum justvanrossum added the bug Something isn't working label Jun 5, 2024
@ronaldoussoren
Copy link
Owner

What's your use case for this?

I've disabled calling __init__ because that leads to more consistent behaviour, especially when considering initialiser with NSError** arguments, e.g.

class MyDocument(NSDocument):
     def __init__(self, *args, **kwds): pass

document = MyDocument()   # __init__ gets called
document, error = MyDocument(type="mytype", error=None). # __init__ does not get called

I've chosen to disable __init__ wholesale for this reason and because the right way to initialise ObjC subclasses is to use an init* method. But I will reconsider if there's a clear use case for using __init__.

@justvanrossum
Copy link
Author

My use case was this: I'd like to instantiate my NSObject subclass as if it's a Python class, by just calling it with arguments.

@ronaldoussoren
Copy link
Owner

That's already possible, but you have to implement one or more init methods for this. What doesn't work is writing a single intermediate subclass and than implemented its classes as if they are plain python classes. TBH, that's not something I had considered.

The changelog mentions this a bit too concisely, https://pyobjc.readthedocs.io/en/latest/notes/instantiating.html is a bit clearer.

class MyObject(NSObject):
    init = None  # Calling MyOjbect() is not allowed

    def initWithX_y_(self, x_value, y_value):
        self = super.init()
        self.x = x_value
        self.y = y_value
        return self

value = MyValue(x=42, y=24)

One limitation is that the order or keywords is currently important. I'm not convinced that this is the right design, but it was easier to start with this limitation than to introduce it later when I run into system frameworks where the order is important. I'll probably drop the restriction during the summer, but only after I've checked the framework bindings and have hammered out a design for a similar feature for calling other methods in a nicer way.

@justvanrossum
Copy link
Author

But what about the aspect of the 10.3 update unnecessarily breaking working code?

@ronaldoussoren
Copy link
Owner

I'll revert the change that broke __init__ and will reconsider for 11.0.

@justvanrossum
Copy link
Author

Idea: only call __init__ if the class defines its own __new__?

@ronaldoussoren
Copy link
Owner

Idea: only call __init__ if the class defines its own __new__?

I like the idea, but have to check how hard it will be to implement this in the current setup. In a very real sense every class now implements its own __new__ when checking from __call__ due to the way the new functionality is implemented. That's not strictly necessary, but made it easier to ensure that pydoc works for introspecting the the signature for __new__.

That said, I already detect if a user has written the __new__ implementation for other reasons and it should be possible to do this in the implementation of __call__ as well.

@justvanrossum
Copy link
Author

For completeness sake: the __init__ change in 10.3 breaks any code that uses the cocoa-vanilla library. This includes applications like DrawBot, FontGoggles, RoboFont and many extensions written for GlyphsApp.

@ronaldoussoren
Copy link
Owner

Thanks for the reference to cocoa-vanilla, I'll test my changes with that library as well.

I shortly commit some changes that ensure that the following tests pass:

class OC_DunderInitBase(NSObject):
    # Helper for ``TestUsingDunderInit``
    def __new__(cls, *args, **kwds):
        return cls.alloc().init()

class TestUsingDunderInit(TestCase):
    # Some users have an intermediate class
    # which implements ``__new__`` to be able
    # to create Cocoa sublcasses using a similar
    # interface as normal Python subclasses, e.g.
    # with ``__init__`` for initializing the instance.
    #
    # This should continue to work.

    def test_using_dunder_init(self):
        class OC_DunderInitSub1(OC_DunderInitBase):
            def __init__(self, x, y=2):
                self.x = x
                self.y = y

        o = OC_DunderInitSub1(x=1)
        self.assertIsInstance(o, OC_DunderInitSub1)
        self.assertEqual(o.x, 1)
        self.assertEqual(o.y, 2)

        with self.assertRaises(TypeError):
            OC_DunderInitSub1()

        with self.assertRaises(TypeError):
            OC_DunderInitSub1(9, z=4)

    def test_multipe_generations(self):
        class OC_DunderInitSub2(OC_DunderInitBase):
            def __init__(self, x, y):
                self.x = x
                self.y = y

        class OC_DunderInitSub3(OC_DunderInitSub2):
            def __init__(self, x, y, z):
                super().__init__(x, y)
                self.z = z


        o = OC_DunderInitSub3(1, 2, 3)
        self.assertIsInstance(o, OC_DunderInitSub3)
        self.assertEqual(o.x, 1)
        self.assertEqual(o.y, 2)
        self.assertEqual(o.z, 3)

That is, __init__ works again when using an explicit __new__ method in a class or one of its superclasses.

ronaldoussoren added a commit that referenced this issue Jun 8, 2024
As mentioned in the changelog update a number of popular
libraries use ``__new__`` and ``__init__`` in their code
and the 10.3 change w.r.t. not calling ``__init__`` broke
those libraries.
@ronaldoussoren
Copy link
Owner

I've clicked around a little in python3 -m vanilla.test.testAll and that code appears to work with this update.

I did have a crash when using vanillaBrowser, but that seems to be related to Python 3.12 support in cocoa-vanilla:

Traceback (most recent call last):
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/test/testAll.py", line 612, in openTestCallback
    BrowserTest(self.w.drawGrid.get())
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/test/testAll.py", line 391, in __init__
    self.w.open()
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaWindows.py", line 269, in open
    self.show()
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaWindows.py", line 310, in show
    self._window.makeKeyAndOrderFront_(None)
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaBrowser.py", line 112, in outlineView_child_ofItem_
    return item.getChild(child)
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaBrowser.py", line 276, in getChild
    childObj = self.__class__(name, obj, self.object, setter)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaBrowser.py", line 246, in __init__
    self.arguments = getArguments(getattr(obj, "__init__"))
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ronald/Projects/pyobjc-8/workenv/lib/python3.12/site-packages/vanilla/vanillaBrowser.py", line 166, in getArguments
    arguments = inspect.formatargspec(*inspect.getargspec(obj))
                ^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'inspect' has no attribute 'formatargspec'. Did you mean: 'formatargvalues'?

@ronaldoussoren
Copy link
Owner

There will be a release of 10.3.1 later this weekend unless I run into unexpected issues when running the full test suite using the latest python updates.

@justvanrossum
Copy link
Author

Thank you!

@schriftgestalt
Copy link

Just for the record, the Glyphs.app python wrapper uses a pattern like this to make the objc API more pythonic:

def __GSNode__new__(typ, *args, **kwargs):
	return typ.alloc().init()

GSNode.__new__ = python_method(__GSNode__new__)

def __GSNode__init__(self, pt: Optional[NSPoint] = None, type: Optional[int] = None, x: Optional[float] = None, y: Optional[float] = None, name: Optional[str] = None) -> None:
	if pt:
		self.setPosition_(pt)
	elif x is not None and y is not None:
		self.setPosition_((x, y))
	if type:
		self.type = type
	if name:
		self.name = name

GSNode.__init__ = python_method(__GSNode__init__)

@ronaldoussoren
Copy link
Owner

There will be a release of 10.3.1 later this weekend unless I run into unexpected issues when running the full test suite using the latest python updates.

... or unless my build VM crashes. I've restarted a test run and expect to release later today.

@ronaldoussoren
Copy link
Owner

Just for the record, the Glyphs.app python wrapper uses a pattern like this to make the objc API more pythonic:

def __GSNode__new__(typ, *args, **kwargs):
	return typ.alloc().init()

GSNode.__new__ = python_method(__GSNode__new__)

def __GSNode__init__(self, pt: Optional[NSPoint] = None, type: Optional[int] = None, x: Optional[float] = None, y: Optional[float] = None, name: Optional[str] = None) -> None:
	if pt:
		self.setPosition_(pt)
	elif x is not None and y is not None:
		self.setPosition_((x, y))
	if type:
		self.type = type
	if name:
		self.name = name

GSNode.__init__ = python_method(__GSNode__init__)

That should work again in 10.3.1. The change in 10.3.1 is to only disable calling __init__ when __new__ is the generic implementation that I added in 10.3.

@ronaldoussoren
Copy link
Owner

PyObjC 10.3.1 is now available on PyPI with this fix.

@justvanrossum
Copy link
Author

justvanrossum commented Jun 11, 2024

I can confirm that my code works again with the 10.3.1 update. Thank you so much!

justinpenner added a commit to justinpenner/TalkingLeaves that referenced this issue Jun 13, 2024
10.3 breaks vanilla, 10.3.1 fixes it. GlyphsPython users will have 10.2 for now, but if user switches to another Python and has 10.3, it will crash TalkingLeaves.

See ronaldoussoren/pyobjc#610
@schriftgestalt
Copy link

I finally got to test this with my code. And it is still not working as before. I added some "convenience" to often used Cocoa classes like this:

NSMenuItem.__new__ = staticmethod(__GSObject__new__)

def __NSMenuItem__init__(self, title, callback=None, target=None, keyboard=None, modifier=0):
    # actual do stuff here, remove for clarity.

NSMenuItem.__init__ = python_method(__NSMenuItem__init__)

I get this error:

  File "_new.py", line 98, in __call__
    return self._function(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "_new.py", line 133, in __new__
    raise TypeError(
TypeError: NSMenuItem() does not accept positional arguments

And if I do

GSSuperClass.__new__ = staticmethod(__GSObject__new__)
GSSubClass.__init__ =  = python_method(__ GSSubClass__init__)

GSSubClass(someArgument)

it fails, too. I need to add:

GSSubClass.__new__ = staticmethod(__GSObject__new__)

I can deal with second problem, but it would be great to be able to amend Cocoa classes like this.

@ronaldoussoren
Copy link
Owner

I'm trying to reproduce the first problem, but so far without success. The traceback indicates that my __new__ implementation is used and not __GSObject__new__.

Is the code below similar enough to what you're doing?

from Foundation import NSURL
from objc import python_method

def override__new__(self, *args, **kwds):
    return self.alloc().initWithString_("https://www.python.org")

def override__init__(self, *args, **kwds):
    print(self, args, kwds)

NSURL.__new__ = staticmethod(override__new__)
NSURL.__init__ = python_method(override__init__)

o = NSURL()
print(o, type(o))

The second issue is a consequence of how the new feature is implemented: Every (native) class gets its own __new__ to ensure that it has a docstring that reflects the actual interface (that is, help(NSURL) includes information about the supported keyword arguments for that particular class.

It might be possible to avoid the second problem in your code by carefully calling objc.lookUpClass for all super classes and set their __new__ before doing anything that might resolve subclasses. That's inherently fragile though.

I'm afraid there's no good solution for the second problem, other then giving up on good help on my end or explicitly setting __new__ for all native classes on your end.

@schriftgestalt
Copy link

schriftgestalt commented Jun 21, 2024

I can fix the second problem on my end. I just need to copy paste that line a few times. So don’t worry about it.

NSURL is not a good sample, it would need to parse the args in __new__, here again with the NSMenuItem and intended usage (having an argument when initializing):

from AppKit import NSMenuItem
from objc import python_method

def override__new__(self, *args, **kwds):
    return self.alloc().init()

def override__init__(self, path, action=None, target=None):
    self.setTitle_(path)
    if action:
        self.setAction_(action)
    if target:
        self.setTarget_(target)

NSMenuItem.__new__ = staticmethod(override__new__)
NSMenuItem.__init__ = python_method(override__init__)

o = NSMenuItem("Menu Title")
print(o, type(o))

The NSURL could be "wrapped" like this:

from Foundation import NSURL
from objc import python_method

def override__new__(self, *args, **kwds):
    return self.alloc().initWithString_(args[0])

NSURL.__new__ = staticmethod(override__new__)

o = NSURL("https://www.python.org")
print(o, type(o))

@ronaldoussoren
Copy link
Owner

The annoying bit is that both work for me. That is, given a virtualenv named "pydotorg" in which I installed PyObjC 10.3.1 from PyPI ("pip install pyobjc"):

% ./pydotorg/bin/python repro.py
<NSMenuItem: 0x600002a79340 Menu Title> <objective-c class NSMenuItem at 0x1ed4a0930>

The current development version (what will be 11.0 later this year) behaves the same. I do get a TypeError when I remove the assignment of NSMenuItem.__new__.

@schriftgestalt
Copy link

You are right. It works when run in plain python. But it doesn't when run inside my app. I’ll dig a bit.

@schriftgestalt
Copy link

I have narrowed it down. It seems only to happen when two python files are executed in the same context. (e.g. two plugins that are based on py2app running inside my app)

Both do:

from AppKit import NSMenuItem
from myWrapper import something # this contains the above code that assigns the methods to NSMenuItem.

The first time first time all is fine, NSMenuItem is imported, and then the myWrapper is loaded and the methods are assigned. But when the second file imports NSMenuItem, the import seems to overwrite my __new__ with the default implementation. The following import from my module doesn't assign my __new__ again as that is only executed when the module is initially loaded.

typemytype added a commit to robotools/vanilla that referenced this issue Jun 24, 2024
@schriftgestalt
Copy link

I found more issues with this change that breaks scripting in Glyphs. Now some of my own classes will throw this error. And only on the second run of a script. I report back as soom as I find more details.
(using 10.3.1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants