-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Allow overloads in source files, not just stubs #2603
Changes from 12 commits
e59f264
bb5fdae
ecb5f73
435751d
7b3c08d
6296587
91b1726
dbc770c
80fc979
045be3e
95555e6
c12a9fd
143692a
04ce906
a191620
b24d2ec
9fa66f4
c6c6d03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
import sys | ||
|
||
from typing import Tuple, Union, TypeVar, Callable, Sequence, Optional, Any, cast, List, Set | ||
from mypy.sharedparse import special_function_elide_names, argument_elide_name | ||
from mypy.sharedparse import ( | ||
special_function_elide_names, argument_elide_name, | ||
) | ||
from mypy.nodes import ( | ||
MypyFile, Node, ImportBase, Import, ImportAll, ImportFrom, FuncDef, OverloadedFuncDef, | ||
MypyFile, Node, ImportBase, Import, ImportAll, ImportFrom, FuncDef, | ||
OverloadedFuncDef, OverloadPart, | ||
ClassDef, Decorator, Block, Var, OperatorAssignmentStmt, | ||
ExpressionStmt, AssignmentStmt, ReturnStmt, RaiseStmt, AssertStmt, | ||
DelStmt, BreakStmt, ContinueStmt, PassStmt, GlobalDecl, | ||
|
@@ -211,12 +214,18 @@ def as_block(self, stmts: List[ast3.stmt], lineno: int) -> Block: | |
|
||
def fix_function_overloads(self, stmts: List[Statement]) -> List[Statement]: | ||
ret = [] # type: List[Statement] | ||
current_overload = [] | ||
current_overload = [] # type: List[OverloadPart] | ||
current_overload_name = None | ||
# mypy doesn't actually check that the decorator is literally @overload | ||
for stmt in stmts: | ||
if isinstance(stmt, Decorator) and stmt.name() == current_overload_name: | ||
if (isinstance(stmt, Decorator) | ||
and stmt.name() == current_overload_name): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should double-check that one of the decorators is actually And if there are decorators but none of them are "overload" and the name matches, this should be considered the implementation. And there should be tests for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't check that here -- we don't know until the semantic analysis step. There's code there to compensate for that. Making sure we have a test for all the various cases of that and that they give solid error messages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Yes, it gives you the redefinition error message, correctly) |
||
current_overload.append(stmt) | ||
elif (isinstance(stmt, FuncDef) | ||
and stmt.name() == current_overload_name | ||
and stmt.name() is not None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would make more sense as checking whether current_overload_name is not None before even calling isinstance() or comparing stmt.name() to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, yeah. |
||
ret.append(OverloadedFuncDef(current_overload + [stmt])) | ||
current_overload = [] | ||
current_overload_name = None | ||
else: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there's a missing case here: a Decorator that's not an overload but does match the current overload name. That would indicate a decorated implementation, which I think we ought to support. |
||
if len(current_overload) == 1: | ||
ret.append(current_overload[0]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -368,21 +368,26 @@ def fullname(self) -> str: | |
return self._fullname | ||
|
||
|
||
OverloadPart = Union['FuncDef', 'Decorator'] | ||
|
||
|
||
class OverloadedFuncDef(FuncBase, SymbolNode, Statement): | ||
"""A logical node representing all the variants of an overloaded function. | ||
|
||
This node has no explicit representation in the source program. | ||
Overloaded variants must be consecutive in the source file. | ||
""" | ||
|
||
items = None # type: List[Decorator] | ||
items = None # type: List[OverloadPart] | ||
impl = None # type: Optional[OverloadPart] | ||
|
||
def __init__(self, items: List['Decorator']) -> None: | ||
def __init__(self, items: List['OverloadPart']) -> None: | ||
self.items = items | ||
self.impl = None | ||
self.set_line(items[0].line) | ||
|
||
def name(self) -> str: | ||
return self.items[0].func.name() | ||
return self.items[0].name() | ||
|
||
def accept(self, visitor: StatementVisitor[T]) -> T: | ||
return visitor.visit_overloaded_func_def(self) | ||
|
@@ -393,12 +398,17 @@ def serialize(self) -> JsonDict: | |
'type': None if self.type is None else self.type.serialize(), | ||
'fullname': self._fullname, | ||
'is_property': self.is_property, | ||
'impl': None if self.impl is None else self.impl.serialize() | ||
} | ||
|
||
@classmethod | ||
def deserialize(cls, data: JsonDict) -> 'OverloadedFuncDef': | ||
assert data['.class'] == 'OverloadedFuncDef' | ||
res = OverloadedFuncDef([Decorator.deserialize(d) for d in data['items']]) | ||
res = OverloadedFuncDef([ | ||
cast(OverloadPart, SymbolNode.deserialize(d)) | ||
for d in data['items']]) | ||
if data.get('impl') is not None: | ||
res.impl = cast(OverloadPart, SymbolNode.deserialize(data['impl'])) | ||
if data.get('type') is not None: | ||
res.type = mypy.types.Type.deserialize(data['type']) | ||
res._fullname = data['fullname'] | ||
|
@@ -535,6 +545,9 @@ def name(self) -> str: | |
def accept(self, visitor: StatementVisitor[T]) -> T: | ||
return visitor.visit_func_def(self) | ||
|
||
def get_body(self) -> Optional['Block']: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this used? I only see this definition (and the one below) but no call sites. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... Thank you. Cruft from a previous way of doing it. |
||
return self.body | ||
|
||
def serialize(self) -> JsonDict: | ||
# We're deliberating omitting arguments and storing only arg_names and | ||
# arg_kinds for space-saving reasons (arguments is not used in later | ||
|
@@ -582,6 +595,7 @@ class Decorator(SymbolNode, Statement): | |
func = None # type: FuncDef # Decorated function | ||
decorators = None # type: List[Expression] # Decorators, at least one # XXX Not true | ||
var = None # type: Var # Represents the decorated function obj | ||
type = None # type: mypy.types.Type | ||
is_overload = False | ||
|
||
def __init__(self, func: FuncDef, decorators: List[Expression], | ||
|
@@ -594,6 +608,9 @@ def __init__(self, func: FuncDef, decorators: List[Expression], | |
def name(self) -> str: | ||
return self.func.name() | ||
|
||
def get_body(self) -> Optional['Block']: | ||
return self.func.body | ||
|
||
def fullname(self) -> str: | ||
return self.func.fullname() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,9 @@ | |
UnicodeLit, FloatLit, Op, Indent, Keyword, Punct, LexError, ComplexLit, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file has disappeared, see #2977. |
||
EllipsisToken | ||
) | ||
from mypy.sharedparse import special_function_elide_names, argument_elide_name | ||
from mypy.sharedparse import ( | ||
special_function_elide_names, argument_elide_name, | ||
) | ||
from mypy.nodes import ( | ||
MypyFile, Import, ImportAll, ImportFrom, FuncDef, OverloadedFuncDef, | ||
ClassDef, Decorator, Block, Var, OperatorAssignmentStmt, Statement, | ||
|
@@ -900,14 +902,15 @@ def parse_block(self, allow_type: bool = False) -> Tuple[Block, Type]: | |
return node, type | ||
|
||
def try_combine_overloads(self, s: Statement, stmt: List[Statement]) -> bool: | ||
if isinstance(s, Decorator) and stmt: | ||
fdef = s | ||
n = fdef.func.name() | ||
if isinstance(stmt[-1], Decorator) and stmt[-1].func.name() == n: | ||
stmt[-1] = OverloadedFuncDef([stmt[-1], fdef]) | ||
if isinstance(s, (FuncDef, Decorator)) and stmt: | ||
n = s.name() | ||
last = stmt[-1] | ||
if (isinstance(last, (FuncDef, Decorator)) | ||
and last.name() == n): | ||
stmt[-1] = OverloadedFuncDef([last, s]) | ||
return True | ||
elif isinstance(stmt[-1], OverloadedFuncDef) and stmt[-1].name() == n: | ||
stmt[-1].items.append(fdef) | ||
elif isinstance(last, OverloadedFuncDef) and last.name() == n: | ||
last.items.append(s) | ||
return True | ||
return 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.
I can get this to crash. Here's a hint on the repro:
According to pdb impl_type is
Any
; defn.impl is a Decorator.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.
Thank you, adding a test and a fix. The fix treats an
Any
-typed implementation as fine no matter what.