mkorpela / overrides

A decorator to automatically detect mismatch when overriding a method
Apache License 2.0
261 stars 32 forks source link

If overriden method has no __module__ assume _is_same_module is False #102

Closed tjsmart closed 2 years ago

tjsmart commented 2 years ago

First off, thank you for this awesome package!

This PR, intends to resolve a minor bug I describe below. I originally encountered the issue while attempting to use overrides with a class from PySide6 (Qt framework package).

But luckily I was able to reproduce the issue more simply using a builtin:

>>> from overrides import overrides
>>> class Foo(int):
...     @overrides
...     def bit_length(self):
...         pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in Foo
  File "/Users/tjsmart/Work/python/overrides/overrides/overrides.py", line 88, in overrides
    return _overrides(method, check_signature, check_at_runtime)
  File "/Users/tjsmart/Work/python/overrides/overrides/overrides.py", line 114, in _overrides
    _validate_method(method, super_class, check_signature)
  File "/Users/tjsmart/Work/python/overrides/overrides/overrides.py", line 135, in _validate_method
    ensure_signature_is_compatible(super_method, method, is_static)
  File "/Users/tjsmart/Work/python/overrides/overrides/signature.py", line 90, in ensure_signature_is_compatible
    same_main_module = _is_same_module(sub_callable, super_callable)
  File "/Users/tjsmart/Work/python/overrides/overrides/signature.py", line 57, in _is_same_module
    mod2 = callable2.__module__.split(".")[0]
AttributeError: 'method_descriptor' object has no attribute '__module__'

For some reason, unknown to me, not all methods in python have a module. It sort of makes sense for a builtin method like int.bit_length. But I don't know why it happens for other packages (such as PySide6).

I think the solution I proposed is self explanatory but please let me know if I should include more details.

FYI, this is my first contribution to an open source project. Please, let me know if I missed any checks!

mkorpela commented 2 years ago

Thank you @tjsmart for the contribution!

mkorpela commented 2 years ago

Seems that Python 3.6 still needs some work.

tjsmart commented 2 years ago

Curious if the build failure in 3.6 relates to https://github.com/mkorpela/overrides/issues/89.

In python3.6 inspecting the signature of a builtin throws a value error:

$ python
Python 3.6.9 (default, Jun 29 2022, 11:45:57)
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> inspect.signature(int.bit_length)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/inspect.py", line 3065, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped)
  File "/usr/lib/python3.6/inspect.py", line 2815, in from_callable
    follow_wrapper_chains=follow_wrapped)
  File "/usr/lib/python3.6/inspect.py", line 2273, in _signature_from_callable
    skip_bound_arg=skip_bound_arg)
  File "/usr/lib/python3.6/inspect.py", line 2097, in _signature_from_builtin
    raise ValueError("no signature found for builtin {!r}".format(func))
ValueError: no signature found for builtin <method 'bit_length' of 'int' objects>

In python3.9 (presumably similar for other python3.7+), it retrieves the correct signature:

$ python
Python 3.9.7 (v3.9.7:1016ef3790, Aug 30 2021, 16:39:15)
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import inspect
>>> inspect.signature(int.bit_length)
<Signature (self, /)>

Ideally, we would be able to retrieve the signature correctly in 3.6. I'm not sure how to do that, but I'm happy to help take a look into that.

Alternatively, (less ideal solution) user's of 3.6 would need to specify check_signature=False which is the current workaround. But at least we gain the benefit of signature checking for 3.7+.

@mkorpela, any suggestions on how to proceed?

mkorpela commented 2 years ago

Pass the signature check if ValueError is thrown from inspect. Then test both cases, where override is valid and faulty. Also test Conditionally (if Python 3.6 else if greater) for this specific case.

mkorpela commented 2 years ago

Thank you for the contribution. I try to get this released this week.

tjsmart commented 2 years ago

Thanks @mkorpela!