Shoobx / mypy-zope

Plugin for mypy to support zope.interface
MIT License
39 stars 13 forks source link

InterfaceClass subclass not recognized as interface #21

Closed jaraco closed 3 years ago

jaraco commented 4 years ago

The project foolscap relies on zope.interface and creates a RemoteInterface as an instance of a subclass of zope.interface.interface.InterfaceClass. When tested against mypy with this plugin, the interface isn't recognized as such an results in "Method must have at least one argument" failed validations.

As an example:

draft $ cat > mypy.ini
[mypy]
ignore_missing_imports = True
plugins=mypy_zope:plugin
draft $ cat > example.py
from foolscap.api import RemoteInterface

class Api(RemoteInterface):
    def open():
        "open something"
draft $ pip-run -q mypy foolscap mypy-zope -- -m mypy example.py
example.py:5: error: Method must have at least one argument
Found 1 error in 1 file (checked 1 source file)

I've attempted to create an even more minimal example that doesn't involve foolscap, but I've been unsuccessful:

draft $ cat > example.py
from zope.interface import interface

RemoteInterface = interface.InterfaceClass("RemoteInterface")

class Api(RemoteInterface):
    def open():
        "open something"
draft $ pip-run -q mypy mypy-zope -- -m mypy example.py
example.py:7: error: Variable "example.RemoteInterface" is not valid as a type
example.py:7: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
example.py:7: error: Invalid base class "RemoteInterface"
example.py:8: error: Method must have at least one argument
Found 3 errors in 1 file (checked 1 source file)

I don't understand why

Variable "example.RemoteInterface" is not valid as a type

The note there isn't particularly helpful, as I'm creating neither an alias nor a variable. I'm creating a proper type and in the same way that foolscap does, so why does this example error where foolscap doesn't?

And surely the example error isn't valid until mypy recogognizes RemoteInterface correctly.

Can you help to determine a solution here to support foolscap.api.RemoteInterface subclasses within mypy?

kedder commented 4 years ago

I don't understand why

Variable "example.RemoteInterface" is not valid as a type

Mainly because RemoteInterface looks like an instance of a class (if you don't know about the runtime magic behind InterfaceClass). And instances of a class are not valid as a type.

mypy-zope is simply don't know how InterfaceClass behaves at runtime and treats it as a regular class. I think it should be possible to teach mypy-zope to treat such expressions, but it is simply not there.

What is supported is class declarations, like:

class RemoteInterface(interface.Interface):
    pass

class Api(RemoteInterface):
    def open():
        "open something"

I'm not completely sure that is equivalent to your case though.

I can't give you a recipe how to implement the support for this use case, but perhaps it would involve adding a special case in get_function_hook() and returning an instance of a type when zope.interface.InterfaceClass is called.

Another hack to try is to add a special __new__ signature to the stub of InterfaceClass in src/zope-stubs/interface/interface.pyi.

jaraco commented 4 years ago

I don't understand why

Variable "example.RemoteInterface" is not valid as a type

Mainly because RemoteInterface looks like an instance of a class... And instances of a class are not valid as a type.

But the same is true of RemoteInterface in foolscap, and the error isn't emitted in my first example. Even if I copy the full implementation of RemoteInterfaceClass and RemoteInterface from foolscap to my example, the error continues. I wish I understood why the implementation in foolscap doesn't trigger this error. Regardless, that issue is secondary to my concern. It would be nice to have a small reproducer for the issue, but the primary issue is that in foolscap, the RemoteInterface isn't recognized as an InterfaceClass instance (and allowing omission of self).

Oh, maybe I know - since I'm only running mypy on example.py, it's not checking anything in foolscap, and that's why the error isn't emitted.

I'll try your recommendations for addressing the foolscap RemoteInterface.

jaraco commented 4 years ago

perhaps it would involve adding a special case in get_function_hook() and returning an instance of a type when zope.interface.InterfaceClass is called.

This approach doesn't have any effect in the foolscap case, so I'm not sure it's relevant.

Another hack to try is to add a special new signature to the stub of InterfaceClass in src/zope-stubs/interface/interface.pyi.

I explored this approach, but quickly realized that there's no suitable return type for __new__:

class InterfaceClass(type, Element, InterfaceBase, Specification):
    ....
    def __new__(self, name: Any, bases: Any = ..., attrs: Optional[Any] = ..., __doc__: Optional[Any] = ..., __module__: Optional[Any] = ...) -> Interface: ...

I tried that line, but at the point where __new__ is defined, Interface is not yet defined, and cannot be until InterfaceClass is defined. Moreover, Interface isn't the correct return type. What __new__ returns is an instance of InterfaceClass and Interface is just one such instantiation.

I did try an alternative syntax for the RemoteInterfaceClass creation:

from zope.interface import interface

class RemoteInterfaceClass(interface.InterfaceClass):
    pass

class RemoteInterface(metaclass=RemoteInterfaceClass):
    pass

class Api(RemoteInterface):
    def open():
        "open something"

Running mypy-zope on this produces:

draft $ pip-run -q mypy-zope -- -m mypy example.py
example.py:13: error: Method must have at least one argument
Found 1 error in 1 file (checked 1 source file)

This error seems to capture the limitation without foolscap.

Do you have any suggestions for addressing this limitation?

jaraco commented 4 years ago

An even simpler implementation triggers the same error:

from zope.interface import interface

class RemoteInterface(metaclass=interface.InterfaceClass):
    pass

class Api(RemoteInterface):
    def open():
        "open something"
jaraco commented 4 years ago

I've found with this diff:

mypy-zope master $ git diff
diff --git a/src/mypy_zope/plugin.py b/src/mypy_zope/plugin.py
index 6a28b5c..2d8bdd7 100644
--- a/src/mypy_zope/plugin.py
+++ b/src/mypy_zope/plugin.py
@@ -258,7 +258,11 @@ class ZopeInterfacePlugin(Plugin):
         self, fullname: str
     ) -> Optional[Callable[[ClassDefContext], None]]:
         # print(f"get_metaclass_hook: {fullname}")
-        return None
+        def analyze_metaclass(ctx: ClassDefContext) -> Callable[[ClassDefContext], None]:
+            if ctx.cls.metaclass.name == 'InterfaceClass':
+                md = self._get_metadata(ctx.cls.info)
+                md["is_interface"] = True
+        return analyze_metaclass

     def get_base_class_hook(
         self, fullname: str

Running the checks against that example no longer fail.

It's not a proper fix, but starts to work toward a solution. I couldn't figure out how to resolve that ctx.cls.metaclass to zope.interface.interface.InterfaceClass. And it still doesn't address the fact that InterfaceClass() can't be called with parameters to construct a new Interface type.

Any help you can lend in filling in the gaps and adding support for this use case would be most appreciated.

kedder commented 4 years ago

"Method must have at least one argument" error indicates that the interface is not recognized as interface. There is a hack that adds "self" argument to all the methods of the interface, so that further mypy checks would not emit this error. See _adjust_interface_function.

Currently the plugin has a special treatment for zope.interface.interface.Interface: it is recognized as a base for all interfaces. interface.InterfaceClass("RemoteInterface") creates another kind of interfaces and it is not recognized.

However, zope.interface.interface.Interface itself is is also defined as

class Interface(metaclass=InterfaceClass): ...

So perhaps it is possible to generalize this so that. And actually your patch makes sense from that point of view. The only thing is missing here: the base zope.interface.interface.Interface gets special treatment: a fake constructor is created for that class to support adaptation pattern (see analyze_interface_base)

jaraco commented 4 years ago

Running the checks against that example no longer fail.

I'm no longer able to replicate this observation. I think the observation I'd encountered may have been due to a false negative from a cached result.

jaraco commented 4 years ago

Aha. I had forgotten to enable the plugin. Enabling the plugin (which apparently can only be done by creating a config file), I can once again replicate the success.

kedder commented 3 years ago

Closing after merging of #24