Skip to content
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

Start using TypeAliasType in the semantic analyzer #7923

Merged
merged 16 commits into from
Nov 14, 2019

Conversation

ilevkivskyi
Copy link
Member

This PR starts using the new TypeAliasType in the semantic analyzer. This PR doesn't yet pulls the trigger to enable the recursive types, but it is now essentially one line of code away. This PR:

  • Makes type analyzer return a TypeAliasType instead of eagerly expanding the alias target.
  • Refactors TypeAliasExpr to refer to TypeAlias (sorry for the noise).
  • Makes few minor fixes to make all existing tests pass.
  • Adds few logistic changes around get_proper_type() I found necessary while playing with actual recursive types over the weekend.

Here are some strategical comments:

  • Taking into account how easy it was to make all existing tests pass, I don't think it is necessary to introduce a hidden option flag that would eagerly expand all type aliases after semantic analyzis.
    It would probably make sense to test this well locally before a public release.
  • There is a special case for no arguments generic aliases. Currently one is allowed to write L = List; x: L[int], I preserve this by using eager expansion in this special case, otherwise it would complicate the whole logic significantly. This is also mostly a legacy thing because we have built-in aliases like List = list magically added by semantic analyzer.
  • I have found that just carelessly sprinkling get_proper_type() is not a best strategy. It saves all the existing special-casing but also introduces a risk for infinite recursion. In particular, "type ops tangle" should ideally always pass on the original alias type. Unfortunately, there is no way to fix/enforce this (without having some severe performance impact). Note it is mostly fine to "carelessly" use get_proper_type() in the "front end" (like checker.py, checkexpr.py, checkmember.py etc).

Here is my plan for the next five PRs:

  1. I am going to try merging SubtypeVisitor and ProperSubtypeVisitor, there is very large amount of code duplication (there is already an issue for this).
  2. I am going to try to get rid of sametypes.py (I am going to open a new issue, see my arguments there).
  3. I am going to enable the recursive aliases and add sufficiently many tests to be sure we are safe about infinite recursion in type ops.
  4. I am going to change how named tuples and typed dicts are represented internally, currently they are stored as TypeInfos, but will be stored as TypeAlias. Essentially there will be almost no difference between A = Tuple[int, int] and A = NamedTuple('A', [('x', int), ('y', int)]). This will allow typed dicts and named tuple participate in recursive types.
  5. I am going to switch from using unbound type variables to bound type variables for generic type aliases, since now they are almost identical to TypeInfos so it IMO it really makes sense to make them uniform (and avoid confusions and code duplication in future).
    5a. Potentially as a follow-up I may add support for generic named tuples and typed dicts, since steps 4 plus 5 will make this almost trivial.

@ilevkivskyi
Copy link
Member Author

(A side note, I can potentially make fewer but larger PRs, but IMO so far the idea of small PRs worked well for type aliases, so I would prefer to continue this way.)

@ilevkivskyi
Copy link
Member Author

There is another important thing to call out, previously unions never contained another unions as items (because constructor flattened them), and some code might implicitly rely on this. IMO we should probably update these places, since maintaining this guarantee may be painful.

@ilevkivskyi
Copy link
Member Author

Yet another important thing is that this may break many plugins, so we need to announce this in #6617 when will merge this.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Great, this PR enables type alias types with fairly minor local changes. I'm no longer worried that introducing this is very risky, but I think that we should have at least one full week of internal testing before a public release.

Thank you for writing such detailed plan for the next steps! The plan looks solid to me.

def visit_type_alias_type(self, t: TypeAliasType) -> None:
super().visit_type_alias_type(t)
if t in self.seen_aliases:
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment about this special case.

@@ -27,6 +29,7 @@ def __init__(self, errors: Errors, options: Options, is_typeshed_file: bool) ->
self.scope = Scope()
# Should we also analyze function definitions, or only module top-levels?
self.recurse_into_functions = True
self.seen_aliases = set() # type: Set[TypeAliasType]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment (also explain why we need this).

mypy/types.py Outdated
@@ -180,6 +180,10 @@ def _expand_once(self) -> Type:
its public wrapper mypy.types.get_proper_type() is preferred.
"""
assert self.alias is not None
if self.alias.no_args:
assert isinstance(self.alias.target, ProperType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this? Add comment.

@@ -253,7 +253,7 @@ class TypeQuery(SyntheticTypeVisitor[T]):

def __init__(self, strategy: Callable[[Iterable[T]], T]) -> None:
self.strategy = strategy
self.seen = [] # type: List[Type]
self.seen_aliases = set() # type: Set[TypeAliasType]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whill we ever need to clear this? If the visitor is single-use, mention this in the docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants