Convert macOS backend to ARC#31855
Conversation
|
Overall this looks like a good direction to me. I would say that most people who have written/contributed to the macos backend are not objc developers, so we (at least myself) are likely just not aware of the objc conventions and patterns. Please feel free to correct things you think would make this easier to understand/follow. Your explanations so far have been really great, so thank you for that and the links for references, I've been learning myself! I think we should bump our minimum supported deployment target to 10.14 at a minimum for our new releases. It has been unsupported by Apple for nearly 5 years now. Python 3.12-13 already have a 10.13 (3.14 has a 10.15) minimum when building wheels, so we might not even be getting 10.12 support even if we're trying to declare it. Apple Silicon requires 11+ If this makes the code more reliable and easier to maintain in the future, we should do it now IMO. Moving all in on ARC and not requiring the extra manual reference counting macros looks much nicer to me. Some links on stats from other main Python discussions for reference as well, showing this is a really small number of people to support on that old of an XCode / OS. |
10.12 and 10.13 have broken installers, as well. They are installable, but require jumping through flaming hoops: There are also some new methods added in Mojave (10.14) related to View's device_scale math, so bumping to 10.14 may have some additional benefits. |
PR summary
This pull request enables Automatic Reference Counting for the macOS backend.
Closes #31797, #31798, and #29076
I'm still testing this PR on macOS 10.12, macOS 14, and macOS 26. I wanted to start the PR process early so that others could review my general direction and give opinions.
Note
I'm trying to follow existing coding styles already present in
_macosx.m. However, this pull request converts some Objective-C instance variables (ivars) to Objective-C properties, which adds an underscore to the ivar name.I'd like to convert more of these in the future, especially in View where there is a
@publicivar (ivar scoping attributes like@publichaven't been used for a very long time).If there are any concerns, or if I'm overstepping, please let me know!
ARC C Struct Compatibility
As we support compiling on macOS 10.12, we cannot use ARC objects within C structures. Support for this was added in Xcode 10 / macOS 10.14. See WWDC 2018 Session 409 for more information about this:
Archived Video (Direct 1.3GB download)
Archived Slides
As a workaround, I added a new macro called
STORE_OBJC_OBJECTwhich manually performs reference counting when storing into structs allocated with tp_alloc.The basic pattern for dealing with Python/Obj-C objects and using this macro is as follows:
If, someday, our minimum target for building matplotlib becomes macOS 10.14 / Xcode 10, then the following changes would occur:
This assumes that
tp_alloc()is usingPyType_GenericAlloc, which zero-fills allocated memory.Paired +alloc and -init
This PR correctly pairs each call to
+allocwith an immediate call to-init…. See #31798 for more information on this.Ownership Overview
Based on my previous experience working with language bridging, I think the following ownership structure makes sense:
Each PyObject should own an Obj-C object via a strong reference.
The Obj-C object, if needed, should have a weakly-assigned back-pointer to the PyObject. This back-pointer should be set in
Foo_initand must be cleared inFoo_dealloc.In modern Objective-C, you would use a
readwrite+assign@propertyfor the back-pointer variable:@property (nonatomic, readwrite, assign) PyObject *myPythonObject;or simply:
@property (nonatomic) PyObject *myPythonObject;This will automatically create a backing
_myPythonObjectivar as well as a-setMyPythonObject:method.FigureManager and Window
As mentioned in my last PR, FigureManager and Window strongly reference each other. I believe this is for historical reasons, likely to prevent a crash.
I made Window weakly-reference FigureManager to follow the pattern established in other objects and haven't experienced any issues. That said, I'm still wrapping my head around the interactions between Python and the main NSRunLoop.
I changed Window's raw
managerivar to a property. This synthesized asetManager:method and eliminated the need for declaring our own-init…method.There is a new
FigureManager__closeAndClearWindow()method called fromFigureManager_destroyandFigureManager_dealloc. It needs to be called from both since Windows are fairly heavyweight objects and we should probably release the memory as soon as they are closed. This method zeros-out back-pointers.NavigationToolbar2Handler
The
-init…method is no longer needed since the pointer toNavigationToolbar2is now areadwriteproperty.There was a potential crash if the
NavigationToolbar2was freed and thenNavigationToolbar2Handlertried to access it. The back-pointer needs to be cleared inNavigationToolbar2_deallocto prevent this.Modern practice is to place additional ivars directly on the
@implementationblock. You can also use@propertysyntax and make themreadonly. I did the latter here so we could get rid of the@interfaceivars.View
-[View initWithFrame:]needs to check that the call tosupersucceeds. In the very-rare case that-[NSView initWithFrame:]returns nil, we were trying to dereference a NULL pointer. Theif (self = [super init…]) {pattern is standard practice.It's also standard practice for
-init…methods to useinstancetypeas the return type. This is mostly to handle subclassing, so it's not actually needed here, but I figured that it was best to change it.Clearing a weak back-pointer in
-[View dealloc]isn't doing what it appears to do. These always need to be cleared by the owning object in the owning object'sdealloc/ThePyObject_deallocmethods.The rest of the changes involve removing
setCanvas:, as it is synthesized, and using the synthesized ivar for thecanvas@propertyinstead of the old directly-declared one.Timer
I fixed #29076 while I was testing Timer invalidation and ownership.
Timer__timer_stop_implhad to be moved soTimer__timer_startcould call it.AI Disclosure
tp_allocworks and if it zero-fills memory.PR checklist