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

mypy doesn't understand `six.with_metaclass` #1764

Closed
Diggsey opened this Issue Jun 29, 2016 · 10 comments

Comments

Projects
None yet
7 participants
@Diggsey

Diggsey commented Jun 29, 2016

Example:

import six


class TestBase(object):
    pass


class TestMeta(type):
    pass


class Test(six.with_metaclass(TestMeta, TestBase)):
    pass

In this case, mypy returns error: Invalid base class. However, in my larger project it actually caused mypy to crash while checking a type derived from the Test analogue. Unfortunately I wasn't able to reproduce this in a small example, but switching to @six.add_metaclass fixed the problem.

@rowillia

This comment has been minimized.

Contributor

rowillia commented Sep 2, 2016

@gvanrossum Do you believe this could be solved with the hypothetical plugin system in #1240 or is six core enough it would make sense to bake understanding into mypy?

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Sep 2, 2016

@TRManderson

This comment has been minimized.

Contributor

TRManderson commented Feb 8, 2017

This should probably be looked at again now that #2475 has been merged

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 11, 2017

More issues:

  • six.add_metaclass causes a runtime error
from typing import *
from typing import GenericMeta

import six

class _DestroyableMeta(type): pass

@six.add_metaclass(_DestroyableMeta)
class Destroyable:
    pass

T_co = TypeVar('T_co', bound='Destroyable', covariant=True)

class ArcMeta(GenericMeta, _DestroyableMeta):
    pass

@six.add_metaclass(ArcMeta)
class Arc(Generic[T_co], Destroyable):
    pass

This passes with mypy -2 but running with python2.7 gives

Traceback (most recent call last):
  File "t.py", line 18, in <module>
    class Arc(Generic[T_co], Destroyable):
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/typing.py", line 1008, in __new__
    self = super(GenericMeta, cls).__new__(cls, name, bases, namespace)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/typing.py", line 104, in __new__
    return super(TypingMeta, cls).__new__(cls, str(name), bases, namespace)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/abc.py", line 87, in __new__
    cls = super(ABCMeta, mcls).__new__(mcls, name, bases, namespace)
TypeError: Error when calling the metaclass bases
    metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
  • six.with_metaclass causes a mypy error (it gives both Invalid base class and also no longer recognizes the Generic base class). (Exact repro TBD, these are errors from our production code.)
    x.py:32: error: Invalid base class
    x.py:34: error: "Arc" expects no type arguments, but 1 given
    x.py:60: error: "Arc" expects no type arguments, but 1 given
    x.py:81: error: "Arc" expects no type arguments, but 1 given

@JukkaL JukkaL added the feature label May 11, 2017

@JukkaL

This comment has been minimized.

Collaborator

JukkaL commented May 11, 2017

This passes with mypy -2 but running with python2.7 gives

Maybe this should be filed in the typing issue tracker?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 12, 2017

Maybe this should be filed in the typing issue tracker?

I think it is not something we can fix. six.add_metaclass applies to already created class. But the class can't actually be created because of an obvious metaclass conflict (this also fails the same way in 3.6).

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 13, 2017

Thinking about this more, Ivan is of course right that six.add_metaclass doesn't work for multiple inheritance. There are really several separate mypy bugs here:

  • It completely ignores the @six.add_metaclass(mcls) decorator; it should interpret this as equivalent to __metaclass__ = mcls in the body for PY2, or the metaclass=mcls keyword argument to the class statement in PY3. (It's still useful for single inheritance.)
  • It doesn't recognize six.with_metaclass(mcls, base, ...) in the "base classes" position of a class statement -- this is the only solution that works at runtime for PY2 and PY3 with multiple inheritance.

I intend to fix both by hard-coding checks for these two six functions in those positions.

There's also the issue that mypy accepted this, which doesn't work at runtime:

@six.add_metaclass(ArcMeta)
class Arc(Generic[T_co], Destroyable):

To address this we ought to duplicate Python's metaclass compatibility check. I think that's a lower priority though.

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 13, 2017

Related: mypy basically ignores class decorators (#3135). I agree that the six functions merit special treatment. (Or maybe Jukka's proposed plugin architecture from #3299 could be sufficient?)

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 13, 2017

I suspect the plugin architecture won't work here because metaclasses are computed before type checking begins.

This was referenced May 15, 2017

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented May 15, 2017

FWIW I forked issue #3365 (with lower priority) so we can focus here on adding support for six.with_metaclass. The ancillary issue can be deferred as well.

(BTW Thanks to @TRManderson for laying the groundwork here -- it was pretty easy to special-case six.with_metaclass and make it look like the PY-version-specific form is used.)

gvanrossum added a commit that referenced this issue May 17, 2017

Support six.with_metaclass (#3364)
Fixes #1764.

This doesn't add support for @six.add_metaclass(M) -- if that's required we'll add it later (we should open a separate issue for that).

It does support generics (at least in the base classes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment