-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add a SYMBOL_FUNCBASE_TYPES constant to avoid multiple-inheritance bug #6481
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
…sues A mypy bug (#3603) causes mypy to think that SymbolNodes cannot be FuncBases, which causes mypy-mypyc to crash in code that tests that. Currently we work around this in a couple gross ways (like turning the SymbolNode into Any). Work around it instead by testing directly against a tuple of the subtypes instead of FuncBase itself. This is the better fix promised in #6480.
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.
LG, though I'm lukewarm on how much clarity this adds...
super_info = node_node.info | ||
if isinstance(node.node, Decorator): | ||
super_info = node.node.var.info # type: Optional[TypeInfo] | ||
elif isinstance(node.node, (Var, SYMBOL_FUNCBASE_TYPES)): |
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.
So the hack now is that this is a variable and the binder doesn't know whether it's true or false?
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.
There's no hack, anymore, really. We're just explicitly enumerating the overlap between SymbolNode and FuncBase instead of using isinstance on FuncBase
cannot be a FuncBase. | ||
|
||
Instead, test against SYMBOL_FUNCBASE_TYPES, which enumerates | ||
SymbolNode subclasses that are also FuncBase subclasses. |
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.
Sounds tedious. :-(
@@ -668,6 +679,11 @@ def deserialize(cls, data: JsonDict) -> 'FuncDef': | |||
return ret | |||
|
|||
|
|||
# All types that are both SymbolNodes and FuncBases. See the FuncBase | |||
# docstring for the rationale. | |||
SYMBOL_FUNCBASE_TYPES = (OverloadedFuncDef, FuncDef) |
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 do we ensure this stays up to date? If a new class were to be added, would tests for that new class fail clearly?
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.
They would fail, I think, though I'm not sure how cleanly.
A mypy bug (#3603) causes mypy to think that SymbolNodes cannot be
FuncBases, which causes mypy-mypyc to crash in code that tests
that.
Currently we work around this in a couple gross ways (like turning the
SymbolNode into Any). Work around it instead by testing directly
against a tuple of the subtypes instead of FuncBase itself.
This is the better fix promised in #6480.