Signature enforcement doesn't allow edge cases with **kwargs

[cboots]

I'm happy to see signature enforcement feature, cool stuff, but it broke how I was doing inheritance in my library. Maybe I'm wrong to do this, but it's valid python and it seems to be a missed edge case.

Here's a toy example that exposes the issue. In this design pattern the base class has a concrete set of optional parameters that I want to be able to add to in the future without updating every subclass to add the same optional arguments and defaults. The base class is the source of truth for this behavior.

Subclasses can add their own unique parameters to the signature, and pass the remainder on to the superclass without requiring knowledge of the signature or duplication of the default parameters.

from overrides import overrides, EnforceOverrides

class A(EnforceOverrides):

   def methoda(self, x=0):
       print(x)

class B(A):

   # this fails validation because x is missing
   @overrides
   def methoda(self, y=1, **kwargs):
       print(y)
       super().methoda(**kwargs)

However, this results in this stack trace:

  File "C:\ProgramData\Anaconda\lib\site-packages\overrides\enforce.py", line 155, in __new__
    ensure_signature_is_compatible(base_class_method, value)

  File "C:\ProgramData\Anaconda\lib\site-packages\overrides\enforce.py", line 34, in ensure_signature_is_compatible
    ensure_all_args_defined_in_sub(super_sig, sub_sig)

  File "C:\ProgramData\Anaconda\lib\site-packages\overrides\enforce.py", line 47, in ensure_all_args_defined_in_sub
    raise TypeError(f"`{name}` is not present.")

TypeError: `x` is not present.

As a side note, it took me a bit to track down the issue because the TypeError didn't specify which method was missing the parameter "x". That would be a nice enhancement to the message to include the function name.

I found I could get around the issue by changing the base class to use **kwargs, but I'd prefer not to have to do that for readability.

from overrides import overrides, EnforceOverrides


class A(EnforceOverrides):

    def methoda(self, **kwargs):
        x = kwargs.get("x", 0)
        print(x)

class B(A):

    @overrides
    def methoda(self, y=1, **kwargs):
        print(y)
        super().methoda(**kwargs)

[mkorpela]

Thanks @cboots extremely interesting edge case and good point about getting the error message improved!

[mkorpela]

So named and positional arguments should be checked separately.

[ashwin153]

@cboots I think the problem is that x can be passed as a positional or keyword argument in A, but only as a keyword argument in B. In other words, a.methoda(1) is a valid call on A but b.methoda(1) does something completely different on B. Changing the signature of A.methodA to def methoda(self, *, x=0) resolves this ambiguity. The error message can definitely be improved though.

class A(EnforceOverrides):
    def methoda(self, *, x=0):
        pass

class B(A):
    @overrides
    def methoda(self, y=1, **kwargs):
        pass

[brentyi]

Mentioned in another issue, but there's also some relevant discussion on enforcement considerations in python/mypy#6709.

[ashwin153]

To summarize the discussion, the mypy contributors agree that this is an invalid override but are worried about breaking too many people if they start checking it now. It will; however, check this if --strict. I think that overrides has the opportunity to be strict by default, because it has much fewer users than mypy. What do you guys think?

[mkorpela]

I think named and positional arguments should be checked separately.

[mkorpela]

From original example

# positional checking
A().methoda()
B().methoda()
# -> ok
A().methoda(1)
B().methoda(1)
# -> ok -> positionals match

# named checking
A().methoda(x=3)
B().methoda(x=3)
# -> ok -> named match

# -> B.methoda signature overrides A.methoda signature

[mkorpela]

Added as a test to master ac8012f

[mkorpela]

Fixed after dc3b449

[mkorpela]

@cboots we now have 5.0.0b0 out. This should fix the issues that you have reported.
Could you test this pre-release version to ensure that everything works?