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.

Sign up for GitHub

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

Force cache eviction when instance classes don't match. #540

Closed
wants to merge 1 commit into from
Closed

Force cache eviction when instance classes don't match. #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/539.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
An edge case of the instance cache has been fixed. This edge case would cause collection types (such as NSMutableDictionary) to *not* have their Python helper wrappers applied.
25 changes: 18 additions & 7 deletions src/rubicon/objc/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,6 @@ def type_for_objcclass(objcclass):
This method is mainly intended for internal use by Rubicon, but is exposed
in the public API for completeness.
"""

if isinstance(objcclass, ObjCClass):
objcclass = objcclass.ptr

Expand Down Expand Up @@ -893,15 +892,27 @@ class or a metaclass, an instance of :class:`ObjCClass` or
# it's the correct class.
#
# Refs #249.
#
# We also need to evict the cache when the instance *class*
# doesn't match. This can happen when an ObjCInstance subclass
# (like ObjCMutableDictionary) has been used - an ObjCInstance
# will respond to the same selectors as ObjCMutableDictionary,
# but it won't have the helper methods (like __setitem__) that
# make the wrapper useful.
Copy link
Member

@samschott samschott Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this explanation bit hard to understand, especially the 1st sentence "when the instance class doesn't match". Are we talking about Python or ObjC classes here? I gather from the remaining text and the implementation that you mean Python classes.

I'll leave a suggested rewrite, but feel free to reject or adapt. I understand that 'easy to understand' is very subjective :)

We also evict the cached instance when its Python type does not
match what we want to use to represent the given object in Python.
For example, we want to represent a NSDictionary instance as
ObjCDictInstance to add Python dict API methods (like __setitem__).
If we find a ObjCInstance instead in the cache, we evict it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that's a better explanation of this particular implementation; what I'm trying to work out (in response to @mhsmith's queries) is whether there is a better way to frame the original cache hit based on the actual ObjC class data. It feels like there should be... but I've been going mad trying to work out what that formulation would be. All the obvious stuff seems to have weird edge case failures.

#
# Refs #539.
#
if cls == ObjCInstance or isinstance(cls, ObjCInstance):
cached_class_name = cached.objc_class.name
current_class_name = libobjc.class_getName(
libobjc.object_getClass(object_ptr)
).decode("utf-8")
current_class = libobjc.object_getClass(object_ptr)
current_class_name = libobjc.class_getName(current_class).decode(
"utf-8"
)
current_type = type_for_objcclass(current_class)
if (
current_class_name != cached_class_name
and not current_class_name.endswith(f"_{cached_class_name}")
):
type(cached) is not current_type
or current_class_name != cached_class_name
) and not current_class_name.endswith(f"_{cached_class_name}"):
# There has been a cache hit, but the object is a
# different class, treat this as a cache miss. We don't
# *just* look for an *exact* class name match, because
Expand Down