-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Support metaclasses #2475
Support metaclasses #2475
Conversation
(No tests yet) |
Looks reasonable -- thanks for working on this! One thing wasn't clear: does this follow the MRO in search of the metaclass? Also, we may also want to check for incompatible metaclasses in the MRO, but that can be a separate issue and doesn't need to be covered here. |
(I only did a quick pass, not a full review.) |
Hey @elazarg, are you still planning to work on this? |
test-data/unit/semanal-errors.test
Outdated
[out] | ||
main:2: error: Name 'abc.Foo' is not defined | ||
main:2: error: Invalid metaclass 'abc.Foo' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one is preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that abc.Foo does not exist, I prefer "Name 'abc.Foo' is not defined" -- if I were to see the other I would assume that there is an abc.Foo that was unacceptable as a metaclass. (Typos and large lists of imports might obscure that it's not defined at all.)
Yes, sorry. Todo:
Also, I should look for other places where the fallback is needed, other than for iterable. (I must note that the tests for fallbackness feels awkward and error prone) |
I think this is ready for a review. |
def __iter__(self) -> Iterator[int]: yield 1 | ||
|
||
class Good(metaclass=GoodMeta): pass | ||
for _ in Good: pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this works, shouldn't list(C)
work too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just assumed it will work. Turns out it didn't, but after handling the fallback in the solver it does.
mypy/nodes.py
Outdated
|
||
def calculate_metaclass_type(self) -> 'Optional[mypy.types.Instance]': | ||
if self.mro is None: | ||
# XXX why does this happen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a sample that triggers this condition?
Presumably the clue is the comment in __init__
on line 1940 above. Maybe it has something to do with the MRO being calculated twice -- the first time (in semanal.py on line 850) it may fail if there's an import cycle causing a base class being a forward reference; the second time on line 3128 in ThirdPass, called from build.py line 1440. (There's also a call from fixup.py, but I think that's not what you're seeing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out it happens during deserialization in incremental mode: the type of a direct base class may be None. (I should have looked into it long time ago. Sorry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will remove the call from deserialize, since it shouldn't be done yet - it should be done from fixup.py. Does it sound reasonable?
Ah, yes, that sounds right. Also my intuition was 180 degrees off here,
sorry! :-)
|
mypy/semanal.py
Outdated
self.fail("Invalid metaclass '%s'" % defn.metaclass, defn) | ||
return | ||
inst = fill_typevars(sym.node) | ||
assert isinstance(inst, Instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can trigger this assert as follows:
class M(Tuple[int]): pass
class C(metaclass=M): pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be an error then. Do you agree? I don't want to go silently to a fallback,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can add the check to line 911.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also look into #1771? Possibly as a follow-up PR?
mypy/nodes.py
Outdated
candidates = {s.declared_metaclass for s in self.mro} - {None} | ||
for c in candidates: | ||
if all(other.type in c.type.mro for other in candidates): | ||
return c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How sure are you that the answer does not depend on the order of candidates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all. Perhaps redundant tests checks are better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have replaced the set with a simple list
candidates = [s.declared_metaclass for s in self.mro if s.declared_metaclass is not None]
mypy/semanal.py
Outdated
self.fail("Invalid metaclass '%s'" % defn.metaclass, defn) | ||
return | ||
inst = fill_typevars(sym.node) | ||
assert isinstance(inst, Instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can add the check to line 911.
|
||
class A(metaclass=M): pass | ||
|
||
reveal_type(A.x) # E: Revealed type is 'builtins.int' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems redundant, it's the same as in the previous test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed it in the last commit but it still appears here.
I will try to look into #1771 later. But it feels like it will require a hack (or a very deep change). |
OK, let's put off #1771 until a separate PR. (That's why I reopened it
anyways.) I will look at the rest now.
|
Thanks. Great work! |
Fix an issue in #2475 - metaclasses are not calculated for subclasses (this is needed for Enum).
Reopening #2392 (itself a reopen of #2365).
Store the type of the metaclass in TypeInfo, and use it when possible, instead of builtins.type.
I assume metaclasses always inherit from type.
This PR will help fixing #2305 and #741, but the fix is not part of this PR.