-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Detect metaclass conflicts, refs #13563 #13598
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
Conversation
@AlexWaygood do you have any suggestions about the error message? 🤔 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
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, | ||
) |
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.
Maybe you could do something like this, to improve the error message:
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, | |
) | |
subclass_metaclass = typ.metaclass_type | |
if subclass_metaclass is None: | |
return | |
for superclass in typ.mro[1:-1]: | |
superclass_metaclass = superclass.metaclass_type | |
if superclass_metaclass is None or is_named_instance( | |
superclass_metaclass, "builtins.type" | |
): | |
continue | |
if not is_subtype(subclass_metaclass, superclass_metaclass): | |
self.fail( | |
( | |
f"Metaclass conflict: metaclass {subclass_metaclass.fullname!r} " | |
f"is not a subclass of {superclass_metaclass.fullname!r}, " | |
f"the metaclass of base class {superclass.fullname!r}" | |
), | |
typ | |
) |
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've tried it and I didn't like it. Why?
Because it either reports several errors for several bases with incompatible metaclasses or reports just one (if break
is added) which is potentially confusing or we need even more complex code to accumulate all problematic base types and then raise a single one.
Let's keep it as-is for now?
If someone complains, then we can easily make this more readable and more complex.
(This is why I love coping CPython's error messages 😆 )
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.
Let's keep it as-is for now?
Sure, the PR looks pretty good right now! It was just an idea :)
Thanks everyone! 👏 |
Recreate of #13565