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

Offer a simpler way to resolve metaclass conflicts #14033

Open
robsdedude opened this issue Nov 8, 2022 · 14 comments · May be fixed by #17713
Open

Offer a simpler way to resolve metaclass conflicts #14033

robsdedude opened this issue Nov 8, 2022 · 14 comments · May be fixed by #17713

Comments

@robsdedude
Copy link

Feature

Consider the following minimal example

assert type(tuple) is type

class MyMeta(type): pass

class MyTuple(tuple, metaclass=MyMeta): pass

As of 0.990, mypy complains about this

error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]

Same if I use

class MyMeta(tuple.__class__): pass

or

class MyMeta(type(tuple)): pass

instead

Pitch

I propose 2 alternatives to help users deal with this

  • Teach mypy that tuple.__class__ is type and that this is fine (preferred)
  • Tell users to use # typing: ignore to resolve such cases.
    Technically, this is already possible today, but I suggest 2 improvements:
    • https://mypy.readthedocs.io/en/stable/metaclasses.html#gotchas-and-limitations-of-metaclass-support only lists what mypy can't handle. It does not talk about how to work around it => add a note telling users to # typing: ignore[misc] it. I'd like to mention that this feels pretty broad of an exception and might hide other real problems. Since this is an area where mypy might be overly restrictive and a workaround might be necessary, it should maybe be in a less broad category.
    • This is the more important point to me: this comment needs to be added to every subclass of MyTuple which can be frustratingly tedious. Maybe there should be away to tell mypy "ok, you might not understand this, but trust me that this class MyTuple has a valid metaclass, assume so for all child classes".
@robsdedude
Copy link
Author

robsdedude commented Nov 8, 2022

As I played further with it, I found that annotating class MyMeta(type(tuple)): pass with # typing: ignore[misc] allows me to not have to annotate every child class. That's something, I guess. But at least the docs should tell people about this workaround. Ideally, as said, it shouldn't even be necessary.

@sobolevn
Copy link
Member

sobolevn commented Nov 8, 2022

Right now tuple is defined as class tuple(Sequence[_T_co], Generic[_T_co]): (and it is not just tuple, other types are affected as well)

So, it has ABCMeta metaclass according to the typeshed 🤔
I don't think we can remove it.

Your ideas seem reasonable. Do you want to work on it? :)

@AlexWaygood
Copy link
Member

Right now tuple is defined as class tuple(Sequence[_T_co], Generic[_T_co]): (and it is not just tuple, other types are affected as well)

So, it has ABCMeta metaclass according to the typeshed 🤔 I don't think we can remove it.

This problem was previously discussed in python/typeshed#1595

@robsdedude
Copy link
Author

Do you want to work on it? :)

I'm happy to contribute. a) I haven't contributed to mypy before, so I wouldn't exactly know where to start, guidance welcome b) Can you elaborate on "Your ideas seem reasonable"? I'm not sure which of the two approaches I mentioned you'd like to see me implement. If the first one: I wouldn't know where to start looking. Judging by what AlexWaygood wrote, this seems to not be solvable in mypy, but typshed which again would need extended typing features in Python itself... so that seems a little unfeasible to me?

I'm happy to jump into a chat somewhere to talk about the details, if you want to help me getting started.

@sobolevn
Copy link
Member

sobolevn commented Nov 8, 2022

only lists what mypy can't handle. It does not talk about how to work around it => add a note telling users to # typing: ignore[misc] it. I'd like to mention that this feels pretty broad of an exception and might hide other real problems. Since this is an area where mypy might be overly restrictive and a workaround might be necessary, it should maybe be in a less broad category.

There are two cool ideas in this:

  1. We can totally list this as a known problem in the docs
  2. We can use a separate error code for metaclass issues

@robsdedude you can schedule a meeting if you want: https://calendly.com/sobolevn/ Don't worry that it says 2hours (we can do much faster). I will guide you through the contribution process :)

Or drop me an email: mail @ sobolevn.me if that's easier for you.

@sobolevn
Copy link
Member

sobolevn commented Nov 9, 2022

@robsdedude meeting notes:

  1. Error codes: https://github.com/python/mypy/blob/master/mypy/errorcodes.py
  2. Error messages: https://github.com/python/mypy/blob/master/mypy/message_registry.py
  3. Metaclass check:

    mypy/mypy/checker.py

    Lines 2496 to 2523 in dbcbb3f

    def check_metaclass_compatibility(self, typ: TypeInfo) -> None:
    """Ensures that metaclasses of all parent types are compatible."""
    if (
    typ.is_metaclass()
    or typ.is_protocol
    or typ.is_named_tuple
    or typ.is_enum
    or typ.typeddict_type is not None
    ):
    return # Reasonable exceptions from this check
    metaclasses = [
    entry.metaclass_type
    for entry in typ.mro[1:-1]
    if entry.metaclass_type
    and not is_named_instance(entry.metaclass_type, "builtins.type")
    ]
    if not metaclasses:
    return
    if typ.metaclass_type is not None and all(
    is_subtype(typ.metaclass_type, meta) for meta in metaclasses
    ):
    return
    self.fail(
    "Metaclass conflict: the metaclass of a derived class must be "
    "a (non-strict) subclass of the metaclasses of all its bases",
    typ,
    )
  4. Tests:
    [case testMetaclassConflict]
    class MyMeta1(type): ...
    class MyMeta2(type): ...
    class MyMeta3(type): ...
    class A(metaclass=MyMeta1): ...
    class B(metaclass=MyMeta2): ...
    class C(metaclass=type): ...
    class A1(A): ...
    class E: ...
    class CorrectMeta(MyMeta1, MyMeta2): ...
    class CorrectSubclass1(A1, B, E, metaclass=CorrectMeta): ...
    class CorrectSubclass2(A, B, E, metaclass=CorrectMeta): ...
    class CorrectSubclass3(B, A, metaclass=CorrectMeta): ...
    class ChildOfCorrectSubclass1(CorrectSubclass1): ...
    class CorrectWithType1(C, A1): ...
    class CorrectWithType2(B, C): ...
    class Conflict1(A1, B, E): ... # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    class Conflict2(A, B): ... # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    class Conflict3(B, A): ... # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    class ChildOfConflict1(Conflict3): ... # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    class ChildOfConflict2(Conflict3, metaclass=CorrectMeta): ...
    class ConflictingMeta(MyMeta1, MyMeta3): ...
    class Conflict4(A1, B, E, metaclass=ConflictingMeta): ... # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    class ChildOfCorrectButWrongMeta(CorrectSubclass1, metaclass=ConflictingMeta): # E: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
    ...
    To run them: pytest -k TEST_NAME

@robsdedude
Copy link
Author

Digging further into it and working on improving the error experience (including the message) as discussed in our meeting, I'm looking at testMetaclassSubclassSelf and in particular ChildOfConflict1. My opinion on this is that, while mypy is (most of the time, anyway) correct in assuming that this class has a metaclass conflict, I don't think the error helps the user at all, because this is not where the error originates (which would be the class definition of Conflict3). I think, this behavior of mypy just a) pollutes the output, making it harder for the user to find the place in the code they need to fix and b) should mypy be incorrect about some assumptions (like with some built-in types), it's not enough to just mute the single place of failure (Conflict3) but every child class of it as well, which can be very tedious.

What do you think about only emitting a metaclass conflict at the place it's introduced at?

This poses some technical difficulty as it's unclear, which metaclass mypy should assume for a class it considered having a metaclass conflict. But I think this can be resolved by traversing the mro and accepting the first explicitly declared metaclass as truth while falling back to type if there is none (which I believe can't actually happen).

Thoughts on this proposal?

@erictraut
Copy link

erictraut commented Nov 11, 2022

@sobolevn, you said "So, it has ABCMeta metaclass according to the typeshed". Where is ABCMeta introduced? Does mypy assume that all Protocol classes implicitly have ABCMeta as a metaclass? Or perhaps it assumes this for protocol classes that have one or more @abstractmethod? Pyright doesn't make this assumption, so it doesn't report a metaclass conflict in the code sample at the top of this thread. I mention this because it could be a simple solution if mypy were not to assume that ABCMeta is the metaclass for protocols.

@robsdedude
Copy link
Author

Another thing to discuss: changing the error code is a breaking change. For users that have silenced mypy complaining about metaclass conflicts (# type: ignore[misc]) this change would make their typecheck start to fail. So I don't know how you handle these kind of things.

Moreover, I noticed that changing the error code didn't make any tests fail. So it appears there are no tests for those, are there?

@sobolevn
Copy link
Member

In my opinion changing codes from misc to some_exact_code is a good thing.

Because it adds more context to it. And changing failing places is rather easy for end users.
But, I am open for other opinions.

@sobolevn
Copy link
Member

@erictraut yes, this is something I will investigate. Thanks!

@robsdedude
Copy link
Author

robsdedude commented Nov 14, 2022

For the record: I found that mypy 0.990 happily accepts

class M1(type): pass
class M2(type): pass
class Mx(M1, M2): pass

class A1(metaclass=M1): pass
class A2(A1): pass

class B1(metaclass=M2): pass

class C1(metaclass=Mx): pass

class D(A2, B1, C1): pass

While the interpreter is, rightfully so, not amused.

Traceback (most recent call last):
  File "foo.py", line 12, in <module>
    class D(A2, B1, C1): pass
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

I'll see if I can also address this bug as it is very much related.

Note for myself: class D(A2, C1, B1): pass however, is fine.

@robsdedude
Copy link
Author

Reading Python's documentation it seems mypy should actually be right and Python should accept class D(A2, B1, C1): pass. So I belief this is either a bug in the interpreter or the docs aren't quite accurate.

Can someone shed some light on which one is the case?

@erictraut
Copy link

I think the interpreter is correct here. Mypy should flag D as a metaclass conflict. (FWIW, pyright reports an error here.)

As you noted, if you change the order of the base classes for D and move C1 before B1, the problem is eliminated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants