-
-
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
Fix crashes and fails in forward references #3952
Changes from 33 commits
45e5931
cb4caa5
1cdc980
260ef02
a58a217
950a022
411b24d
b9b8528
48d6de4
ec45441
ac32ed4
3fb3019
f9b1320
cf014b8
665236b
9a318aa
757fbd9
4502ce2
3b39d40
c8b28fe
9f92b0f
9779103
b914bdb
3568fdb
54d9331
b9ddacc
5bfe9ca
a2912e9
10c65b8
21dfbfe
f2ddbcd
03597ee
649ef32
83f8907
13c7176
79b10d6
321a809
076c909
c1a63ec
97e6f47
8f52654
6edd078
514b8bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1375,6 +1375,26 @@ def deserialize(cls, data: JsonDict) -> Type: | |
return TypeType.make_normalized(deserialize_type(data['item'])) | ||
|
||
|
||
class ForwardRef(Type): | ||
"""Class to wrap forward references to other types.""" | ||
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. Add a longer description since this is a little tricky. For example, give an example where this is useful (say, a forward reference to a |
||
link = None # type: Type # the wrapped type | ||
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. Style nit: Use two spaces before the non-type comment and capitalize |
||
|
||
def __init__(self, link: Type) -> None: | ||
self.link = link | ||
|
||
def accept(self, visitor: 'TypeVisitor[T]') -> T: | ||
return visitor.visit_forwardref_type(self) | ||
|
||
def serialize(self): | ||
if isinstance(self.link, UnboundType): | ||
name = self.link.name | ||
if isinstance(self.link, Instance): | ||
name = self.link.type.name() | ||
else: | ||
name = self.link.__class__.__name__ | ||
assert False, "Internal error: Unresolved forward reference to {}".format(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. Add comment about why we shouldn't get here (all forward references should be resolved and removed during semantic analysis). |
||
|
||
|
||
# | ||
# Visitor-related classes | ||
# | ||
|
@@ -1450,6 +1470,9 @@ def visit_partial_type(self, t: PartialType) -> T: | |
def visit_type_type(self, t: TypeType) -> T: | ||
pass | ||
|
||
def visit_forwardref_type(self, t: ForwardRef) -> T: | ||
raise RuntimeError('Internal error: unresolved forward reference') | ||
|
||
|
||
class SyntheticTypeVisitor(TypeVisitor[T]): | ||
"""A TypeVisitor that also knows how to visit synthetic AST constructs. | ||
|
@@ -1552,6 +1575,9 @@ def visit_overloaded(self, t: Overloaded) -> Type: | |
def visit_type_type(self, t: TypeType) -> Type: | ||
return TypeType.make_normalized(t.item.accept(self), line=t.line, column=t.column) | ||
|
||
def visit_forwardref_type(self, t: ForwardRef) -> Type: | ||
return t | ||
|
||
|
||
class TypeStrVisitor(SyntheticTypeVisitor[str]): | ||
"""Visitor for pretty-printing types into strings. | ||
|
@@ -1707,6 +1733,9 @@ def visit_ellipsis_type(self, t: EllipsisType) -> str: | |
def visit_type_type(self, t: TypeType) -> str: | ||
return 'Type[{}]'.format(t.item.accept(self)) | ||
|
||
def visit_forwardref_type(self, t: ForwardRef) -> str: | ||
return '~{}'.format(t.link.accept(self)) | ||
|
||
def list_str(self, a: List[Type]) -> str: | ||
"""Convert items of an array to strings (pretty-print types) | ||
and join the results with commas. | ||
|
@@ -1786,6 +1815,9 @@ def visit_overloaded(self, t: Overloaded) -> T: | |
def visit_type_type(self, t: TypeType) -> T: | ||
return t.item.accept(self) | ||
|
||
def visit_forwardref_type(self, t: ForwardRef) -> T: | ||
return t.link.accept(self) | ||
|
||
def visit_ellipsis_type(self, t: EllipsisType) -> T: | ||
return self.strategy([]) | ||
|
||
|
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 is kind of hacky -- we call the second pass of semantic analysis from the third pass of type analysis, without having all the normal context for the second pass set up. This causes some errors:
Another example that doesn't work because of this issue:
Not sure what's the best way to approach this. We perhaps don't want to set up the full context for second pass during the third pass, as this can add a lot of extra complexity. A simpler workaround would be to not support forward references with type arguments, and give an error such as
Forward reference to "A" not supported
. This at least would fix the current error messages, which are confusing.Also, I'd prefer if we didn't depend on
mypy.semanal
here. A better approach would be to remove the cyclic import tomypy.semanal
and callTypeAnalyser
directly here.TypeAnalyserPass3
would take the lookup functions etc. as arguments instead of getting them indirectly fromSemanticAnalyzer
. The rationale is that this makes it explicit what context the third pass depends on, and it will make the code clearer. Also, we won't depend on the internals ofSemanticAnalyzer
.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, this is one of the hard parts. I will try to see what is the minimal context needed, basically we just need
lookup
to work in third pass. If it will be too hard, I will just prohibit things likeF[<something>]
, whereF
is a forward reference, with a clearer error message.