FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implementing background infrastructure for recursive types: Part 3 by ilevkivskyi · Pull Request #7885 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Implementing background infrastructure for recursive types: Part 3 - #7885

Merged
ilevkivskyi merged 18 commits into
python:masterfrom
ilevkivskyi:recursive-types
Nov 8, 2019
ConversationCommits (18)ChecksFiles changed
Merged

Implementing background infrastructure for recursive types: Part 3#7885
ilevkivskyi merged 18 commits into
python:masterfrom
ilevkivskyi:recursive-types

Conversation

ilevkivskyi commented Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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:

A = Union[T, Tuple[A[T], ...]]

class A(Protocol[T]):
    @property
    def __base__(self) -> Union[T, Tuple[A[T], ...]]: ...

but where TypeAliasType plays role of Instance and TypeAlias plays role of TypeInfo.

Next two pull requests will contain some non-trivial implementation logic.

msullivan approved these changes Nov 6, 2019
View reviewed changes

msullivan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread mypy/indirection.py Outdated
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):

msullivan Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?)

ilevkivskyi Nov 7, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually I think it is fine to use equality here, I will add unit test to double-check.

Comment thread mypy/join.py Outdated
Comment thread mypy/join.py
Comment thread mypy/meet.py
Comment thread mypy/nodes.py Outdated
Comment thread mypy/subtypes.py
Comment thread mypy/type_visitor.py
Comment on lines +237 to +241
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.

msullivan Nov 6, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No reasonable way around this?

ilevkivskyi Nov 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

JukkaL reviewed Nov 6, 2019
View reviewed changes

JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Did a quick pass. Generally looks good!

Would it make sense to at least add some non-data-driven unit tests for recursive types at this point?

Comment thread mypy/join.py Outdated

ilevkivskyi commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

Would it make sense to at least add some non-data-driven unit tests for recursive types at this point?

Yes, I think it would make sense to add some unit tests.

ilevkivskyi commented Nov 6, 2019

Copy link
Copy Markdown
Member Author

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.

ilevkivskyi commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

@msullivan @JukkaL Could one of you please take a look again?

Summary of changes:

  • Address review comments
  • Add some comments and docstrings
  • Added basic unit tests (basics, some visitors, subtyping)
  • Replaced ad-hoc type alias expansion with proper visitors
  • Shifted boundary of get_proper_type() around unions and expand_types() so that it happens later (previous location caused some conceptual difficulties)
  • Moved the shared state to TypeState where it belongs

Unfortunately the typeshed test is going to fail, because the type alias expansion clean-up discovered two errors in typeshed (previously Type[...] was not visited). Looking at it, it seems to me the check for class variables there is overly strict, we can move it from definition site to use site as we do for class methods. I will make a separate PR, and will merge this one on top.

msullivan approved these changes Nov 8, 2019
View reviewed changes

msullivan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me. Thanks!

What's next? Hooking it up in the semantic analyzer?

ilevkivskyi commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

What's next? Hooking it up in the semantic analyzer?

Yes, exactly!

ilevkivskyi mentioned this pull request Nov 8, 2019
Merged
ilevkivskyi merged commit 59617e8 into python:master Nov 8, 2019
TH3CHARLie mentioned this pull request Feb 22, 2020
Closed
hauntsaninja mentioned this pull request Nov 28, 2020
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JukkaL JukkaL left review comments
msullivan msullivan approved these changes
Michael0x2a Awaiting requested review from Michael0x2a

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL