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

Partial method enhancement #462

Merged
merged 14 commits into from
May 3, 2024
1 change: 1 addition & 0 deletions changes/148.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Objective-C methods with multiple identical argument names can now be called using the "__" keyword and any suffix.
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions changes/453.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
With "interleaved" syntax, the order of method arguments are now strictly checked.
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions changes/461.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Updated the exception message when calling Objective-C methods with invalid keyword arguments to include selector names.
Copy link
Member

Choose a reason for hiding this comment

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

The error message has been improved when an Objective-C selector cannot be found matching the provided arguments.

80 changes: 36 additions & 44 deletions src/rubicon/objc/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,28 +246,34 @@ def __repr__(self):
return f"{type(self).__qualname__}({self.name_start!r})"

def __call__(self, receiver, first_arg=_sentinel, **kwargs):
if first_arg is ObjCPartialMethod._sentinel:
# Ignore parts of argument names after "__".
order = [argname.split("__")[0] for argname in kwargs]
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
args = [arg for arg in kwargs.values()]

if first_arg is self._sentinel:
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
if kwargs:
# This will be deleted when #26 is introduced.
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
raise TypeError("Missing first (positional) argument")

args = []
rest = frozenset()
rest = (*order,)
Copy link
Member

Choose a reason for hiding this comment

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

This is technically correct, but isn't intuitive to read. I'd suggest creating the original order as a tuple comprehension; then this can be just rest = order, and the version in the else branch is rest = ("",) + order

else:
args = [first_arg]
# Add "" to rest to indicate that the method takes arguments
rest = frozenset(kwargs) | frozenset(("",))
args.insert(0, first_arg)
rest = ("", *order)

try:
name, order = self.methods[rest]
name = self.methods[rest]
except KeyError:
if first_arg is self._sentinel:
specified_sel = self.name_start
else:
specified_sel = f"{self.name_start}:{':'.join(kwargs.keys())}:"
available_sels = [repr(sel) for sel in self.methods.values()]
raise ValueError(
f"No method was found starting with {self.name_start!r} and with keywords {set(kwargs)}\n"
f"Known keywords are:\n"
+ "\n".join(repr(keywords) for keywords in self.methods)
)
f"Invalid selector {specified_sel!r}. Available selectors are: "
Copy link
Member

Choose a reason for hiding this comment

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

A minor cosmetic detail, but the quotes added by !r aren't needed here, because the identifier can't ever have spaces or anything else that might be ambiguous.

f"{', '.join(available_sels)}"
) from None

meth = receiver.objc_class._cache_method(name)
args += [kwargs[name] for name in order]

return meth(receiver, *args)


Expand Down Expand Up @@ -1035,28 +1041,11 @@ def __getattr__(self, name):

The "interleaved" syntax is usually preferred, since it looks more
similar to normal Objective-C syntax. However, the "flat" syntax is also
fully supported. Certain method names require the "flat" syntax, for
example if two arguments have the same label (e.g.
``performSelector:withObject:withObject:``), which is not supported by
Python's keyword argument syntax.

.. warning::

The "interleaved" syntax currently ignores the ordering of its
keyword arguments. However, in the interest of readability, the
keyword arguments should always be passed in the same order as they
appear in the method name.

This also means that two methods whose names which differ only in
the ordering of their keywords will conflict with each other, and
can only be called reliably using "flat" syntax.

As of Python 3.6, the order of keyword arguments passed to functions
is preserved (:pep:`468`). In the future, once Rubicon requires
Python 3.6 or newer, "interleaved" method calls will respect keyword
argument order. This will fix the kind of conflict described above,
but will also disallow specifying the keyword arguments out of
order.
fully supported. If two arguments have the same label (e.g.
``performSelector:withObject:withObject:``), you can use ``__`` in the
keywords like ``performSelector(..., withObject__1=...,
withObject__2=...)``. The parts after ``__`` can be anything you can use
as argument names.
qqfunc marked this conversation as resolved.
Show resolved Hide resolved
"""
# Search for named instance method in the class object and if it
# exists, return callable object with self as hidden argument.
Expand Down Expand Up @@ -1090,7 +1079,7 @@ def __getattr__(self, name):
else:
method = None

if method is None or set(method.methods) == {frozenset()}:
if method is None or set(method.methods) == {()}:
# Find a method whose full name matches the given name if no partial
# method was found, or the partial method can only resolve to a
# single method that takes no arguments. The latter case avoids
Expand Down Expand Up @@ -1654,20 +1643,23 @@ def _load_methods(self):
name = libobjc.method_getName(method).name.decode("utf-8")
self.instance_method_ptrs[name] = method

first, *rest = name.split(":")
# Selectors end in a colon iff the method takes arguments.
# Because of this, rest must either be empty (method takes no arguments)
# or the last element must be an empty string (method takes arguments).
assert not rest or rest[-1] == ""
# Selectors end with a colon if the method takes arguments.
if name.endswith(":"):
first, *rest, _ = name.split(":")
# Insert an empty string in order to indicate that the method
# takes a first argument as a positional argument.
rest.insert(0, "")
rest = tuple(rest)
else:
first = name
rest = ()

try:
partial = self.partial_methods[first]
except KeyError:
partial = self.partial_methods[first] = ObjCPartialMethod(first)

# order is rest without the dummy "" part
order = rest[:-1]
partial.methods[frozenset(rest)] = (name, order)
partial.methods[rest] = name

# Set the list of methods for the class to the computed list.
self.methods_ptr = methods_ptr
Expand Down
7 changes: 7 additions & 0 deletions tests/objc/Example.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,13 @@ extern NSString *const SomeGlobalStringConstant;
+(NSUInteger) overloaded;
+(NSUInteger) overloaded:(NSUInteger)arg1;
+(NSUInteger) overloaded:(NSUInteger)arg1 extraArg:(NSUInteger)arg2;
+(NSUInteger) overloaded:(NSUInteger)arg extraArg1:(NSUInteger)arg1 extraArg2:(NSUInteger)arg2;
+(NSUInteger) overloaded:(NSUInteger)arg extraArg2:(NSUInteger)arg2 extraArg1:(NSUInteger)arg1;
+(NSUInteger) overloaded:(NSUInteger)arg orderedArg1:(NSUInteger)arg1 orderedArg2:(NSUInteger)arg2;
+(NSUInteger) overloaded:(NSUInteger)arg duplicateArg:(NSUInteger)arg1 duplicateArg:(NSUInteger)arg2;

+(void) dummyForException:(NSUInteger)arg;
+(void) dummyForException:(NSUInteger)arg1 arg:(NSUInteger)arg2;

+(struct complex) doStuffWithStruct:(struct simple)simple;

Expand Down
30 changes: 30 additions & 0 deletions tests/objc/Example.m
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,36 @@ +(NSUInteger) overloaded:(NSUInteger)arg1 extraArg:(NSUInteger)arg2
return arg1 + arg2;
}

+(NSUInteger) overloaded:(NSUInteger)arg extraArg1:(NSUInteger)arg1 extraArg2:(NSUInteger)arg2
{
return 1;
}

+(NSUInteger) overloaded:(NSUInteger)arg extraArg2:(NSUInteger)arg2 extraArg1:(NSUInteger)arg1
{
return 2;
}

+(NSUInteger) overloaded:(NSUInteger)arg orderedArg1:(NSUInteger)arg1 orderedArg2:(NSUInteger)arg2
{
return 0;
}

+(NSUInteger) overloaded:(NSUInteger)arg duplicateArg:(NSUInteger)arg1 duplicateArg:(NSUInteger)arg2
{
return arg1 + arg2;
Copy link
Member

Choose a reason for hiding this comment

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

This won't confirm if arg1 and arg2 are being handled in the right order, as the addition is commutative. Something like 2*arg1 + 3*arg2 (or even arg1 - arg2) would let us confirm that arg1 and arg2 are being handled in the right order.

}

+(void) dummyForException:(NSUInteger)arg
{
// Dummy method for testing exception
}
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to have these extra methods just to demonstrate we can't call them correctly. The test will work just as well by calling Example.overloaded(0, notAnArgument=10). It would be arguably better to use overloaded:... , because there are more options, and some of them are ambiguous.


+(void) dummyForException:(NSUInteger)arg1 arg:(NSUInteger)arg2
{
// Dummy method for testing exception
}

+(struct complex) doStuffWithStruct:(struct simple)simple
{
return (struct complex){
Expand Down
25 changes: 25 additions & 0 deletions tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,31 @@ def test_partial_method_lots_of_args(self):
)
self.assertEqual(buf.value.decode("utf-8"), pystring)

def test_partial_method_arg_order(self):
Example = ObjCClass("Example")

self.assertEqual(Example.overloaded(0, extraArg1=0, extraArg2=0), 1)
self.assertEqual(Example.overloaded(0, extraArg2=0, extraArg1=0), 2)
Copy link
Member

Choose a reason for hiding this comment

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

This test confirms we're calling the right method, but doesn't verify that the arguments are passed in as expected. Ideally the result should incorporate the values from the arguments.


with self.assertRaises(ValueError):
Example.overloaded(0, orderedArg2=0, orderedArg1=0)
qqfunc marked this conversation as resolved.
Show resolved Hide resolved

def test_partial_method_duplicate_arg_names(self):
Example = ObjCClass("Example")
self.assertEqual(
Example.overloaded(0, duplicateArg__a=14, duplicateArg__b=24),
14 + 24,
)

def test_partial_method_exception(self):
Example = ObjCClass("Example")
with self.assertRaisesRegex(
ValueError,
"Invalid selector 'dummyForException:invalid:'. Available selectors are: "
"'dummyForException:', 'dummyForException:arg:'",
):
Example.dummyForException(None, invalid=None)

def test_objcmethod_str_repr(self):
"""Test ObjCMethod, ObjCPartialMethod, and ObjCBoundMethod str and repr"""

Expand Down
Loading