python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.18k stars 2.78k forks source link

Offer a simpler way to resolve metaclass conflicts #14033

Open robsdedude opened 1 year ago

robsdedude commented 1 year ago

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

robsdedude commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/python/typeshed/issues/1595

robsdedude commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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: https://github.com/python/mypy/blob/dbcbb3f5c3ef791c98088da0bd1dfa6cbf51f301/mypy/checker.py#L2496-L2523
  4. Tests: https://github.com/python/mypy/blob/dbcbb3f5c3ef791c98088da0bd1dfa6cbf51f301/test-data/unit/check-classes.test#L6821-L6852 To run them: pytest -k TEST_NAME
robsdedude commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

robsdedude commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.