Implementing background infrastructure for recursive types: Part 3 #7885
Conversation
There was a problem hiding this comment.
This all seems pretty reasonable.
What is the testing situation here? Do you have an unpolished local implementation of the rest that gives some confidence that this works? I suppose I'm happy pushing ahead either way, though; it's not the end of the world if this needs further changes.
mypy/indirection.py
Outdated
| return self._visit(typs) | ||
|
|
||
| def _visit(self, typ_or_typs: Union[types.Type, Iterable[types.Type]]) -> Set[str]: | ||
| typs = [typ_or_typs] if isinstance(typ_or_typs, types.Type) else typ_or_typs | ||
| output = set() # type: Set[str] | ||
| for typ in typs: | ||
| if any(typ is s for s in self.seen): |
There was a problem hiding this comment.
It seems bad to be doing a search through every seen type so frequently. I assume it is done this way because it needs to be by identity and not by type equality, but then it would probably be better to have a set of ids.
(This is less important, I think, but would it be possible to only track seen TypeAliasTypes and to only do the check when visiting them?)
There was a problem hiding this comment.
Actually I think it is fine to use equality here, I will add unit test to double-check.
| def visit_type_alias_type(self, t: TypeAliasType) -> Type: | ||
| # This method doesn't have a default implementation for type translators, | ||
| # because type aliases are special: some information is contained in the | ||
| # TypeAlias node, and we normally don't generate new nodes. Every subclass | ||
| # must implement this depending on its semantics. |
There was a problem hiding this comment.
No reasonable way around this?
There was a problem hiding this comment.
Unfortunately no. This is kind of a fundamental limitation. Technically we can put something moderately reasonable, but then there is a high chance we will forget to override this when needed in some visitors we will add in future.
Yes, I think it would make sense to add some unit tests. |
Actually I just discovered I am missing one implementation to be able to run unit tests, I will add it and add some tests tomorrow. |
|
@msullivan @JukkaL Could one of you please take a look again? Summary of changes:
Unfortunately the typeshed test is going to fail, because the type alias expansion clean-up discovered two errors in typeshed (previously |
Yes, exactly! |
This is the last part of plumbing for recursive types (previous #7366 and #7330). Here I implement visitors and related functions.
I convinced myself that we need to only be more careful when a recursive type is checked against another recursive one, so I only special-case these. Logic is similar to how protocols behave, because very roughly type alias can be imagined as a protocol with single property:
but where
TypeAliasTypeplays role ofInstanceandTypeAliasplays role ofTypeInfo.Next two pull requests will contain some non-trivial implementation logic.