Refactor type aliases by ilevkivskyi · Pull Request #5221 · python/mypy

ilevkivskyi · 2018-06-15T13:25:40Z

This refactors type aliases by introducing a dedicated symbol node TypeAlias, and removing all ad-hoc attributes from SymbolTableNode. This simplifies the overall logic, and also the fine grained dependency tracking (since now aliases have names).

Most of the logic is straightforward. Few comments:

It looks like .type_override was also used for some overload hacks, I replaced those with a proper fix.
Somehow my changes exposed a small ImportedName bug, I can't find a simple repro (typeshed check test was failing), but the fix is straightforward.
In some tests revealed type don't have * anymore, I think this is more consistent.
There few other small changes in tests, I think they are harmless.

JelleZijlstra · 2018-06-15T14:34:57Z

        return Instance(node, [AnyType(TypeOfAny.special_form)] * len(node.defn.type_vars))

+    def add_builtin_aliases(self, tree: MypyFile) -> None:
+        # For historical reasons, the aliases like `List = list` are not defined


We could just define those in typeshed actually.

I would keep this out of this PR. Will pytype an other be OK with this? (There are potential problems with circular dependencies). If yes we can easily do this later, fortunately with this refactoring it is just a matter of removing this patching step (before normalize_type_alias was all over the semanal code).

I believe other type checkers don't use typing.pyi from typeshed; they have their own versions.

But I agree that this PR is doing enough things already. :)

Currently fails terribly, but maybe python/mypy#5221 will fix it.

JelleZijlstra · 2018-06-15T15:19:13Z

I found a new crash using this PR's code:

$ cat bin/usesta.py 
from typing import List, TypeVar
T = TypeVar('T')

A = List[T]

from hasta import A as A
$ mypy --show-traceback bin/usesta.py 
bin/usesta.py:6: error: Cannot find module named 'hasta'
bin/usesta.py:6: note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help)
bin/usesta.py:6: error: INTERNAL ERROR -- please report a bug at https://github.com/python/mypy/issues version: 0.620+dev-9459b3a74f0f1fefe28556e67db6a5eec2f98f8c
Traceback (most recent call last):
  File "/Users/jelle/py/venvs/venv37/bin/mypy", line 11, in <module>
    load_entry_point('mypy', 'console_scripts', 'mypy')()
  File "/Users/jelle/py/mypy/mypy/__main__.py", line 7, in console_entry
    main(None)
  File "/Users/jelle/py/mypy/mypy/main.py", line 91, in main
    res = type_check_only(sources, bin_dir, options, flush_errors, fscache)  # noqa
  File "/Users/jelle/py/mypy/mypy/main.py", line 148, in type_check_only
    fscache=fscache)
  File "/Users/jelle/py/mypy/mypy/build.py", line 177, in build
    flush_errors, fscache)
  File "/Users/jelle/py/mypy/mypy/build.py", line 282, in _build
    graph = dispatch(sources, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 2436, in dispatch
    process_graph(graph, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 2729, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/jelle/py/mypy/mypy/build.py", line 2845, in process_stale_scc
    graph[id].semantic_analysis()
  File "/Users/jelle/py/mypy/mypy/build.py", line 2047, in semantic_analysis
    self.manager.semantic_analyzer.visit_file(self.tree, self.xpath, self.options, patches)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 296, in visit_file
    self.accept(d)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 3241, in accept
    node.accept(self)
  File "/Users/jelle/py/mypy/mypy/nodes.py", line 317, in accept
    return visitor.visit_import_from(self)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 1394, in visit_import_from
    self.add_unknown_symbol(as_id or id, imp, is_import=True)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 1492, in add_unknown_symbol
    self.add_symbol(name, SymbolTableNode(GDEF, var), context)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 3154, in add_symbol
    self.name_already_defined(name, context, existing)
  File "/Users/jelle/py/mypy/mypy/semanal.py", line 3206, in name_already_defined
    elif node and node.line != -1:
AttributeError: line
bin/usesta.py:6: : note: use --pdb to drop into pdb

(It crashes the same way if it can find hasta.)

ilevkivskyi · 2018-06-15T16:01:56Z

@JelleZijlstra Thanks! TBH I am surprised there is only one crash like this, line and column are never set on TypeAlias in this PR. Will fix this now.

JukkaL

Thanks for refactoring this! This makes the type alias implementation much cleaner, and also saves some memory since a bunch of attributes are no longer needed. Apparently a few minor bugs were fixed in as well, which is nice.

Left a bunch of minor comments (mostly nits).

JukkaL · 2018-06-18T10:12:53Z

        graph[id].fix_suppressed_dependencies(graph)
+    if 'typing' in scc:
+        # For historical reasons we need to manually add typing aliases
+        # for built-in generic collections, see docstring for details.


Update comment to be more explicit about which docstring this refers to.

JukkaL · 2018-06-18T10:14:40Z

+        """Type check a type application (expr[type, ...]).
+
+        There are two different options here, depending on whether expr refers
+        to a type alias or directly to a generic class. In first case we need


Grammar nit: "In the first case ..."

JukkaL · 2018-06-18T10:15:23Z

+        to a type alias or directly to a generic class. In first case we need
+        to use a dedicated function typeanal.expand_type_aliases. This
+        is due to the fact that currently type aliases machinery uses
+        unbound type variables, while normal generics use bound ones,


Grammar nit: use period or semicolon (e.g. "... bound ones; see ...").

JukkaL · 2018-06-18T10:20:40Z

+            A = reveal_type(List[T])
+            reveal_type(A)
+
+        will reveal the same type `def (...) -> builtins.list[Any]`.


This is a bit unclear. What does "same" mean here? If both of the reveal_types reveal the same types, it would be better to rewrite this as "Both reveal_type instances will reveal the same type ...".

Also, it's worth briefly discussing why the type is list[Any] and not list[T] (because of implicit Any for missing type args?).

JukkaL · 2018-06-18T10:21:55Z

+        """
+        return self.alias_type_in_runtime(alias.type, alias.tvars, alias.no_args, alias)
+
+    def alias_type_in_runtime(self, target: Type, alias_tvars: List[str],


The name of this method is a bit unclear. Maybe rename to alias_type_in_runtime_context, for example?

JukkaL · 2018-06-18T12:52:45Z

 UA = Union[T, int]

-cs = CA[str] + 1 # E: Unsupported left operand type for + ("Type alias to Callable")
+cs = CA + 1 # E: Unsupported left operand type for + ("Type alias to Callable")


Should we still test also CA[str] + 1, or it no longer relevant?

It is less relevant because it will fail with something different like "Type application doesn't target a class" in all three cases. But I will add one more test for this.

JukkaL · 2018-06-18T12:52:54Z

 reveal_type(cs) # E: Revealed type is 'Any'

-ts = TA[str]() # E: "Type alias to Tuple" not callable
+ts = TA() # E: "Type alias to Tuple" not callable


Similar to above.

JukkaL · 2018-06-18T12:53:02Z

 reveal_type(ts) # E: Revealed type is 'Any'

-us = UA[str].x # E: "Type alias to Union" has no attribute "x"
+us = UA.x # E: "Type alias to Union" has no attribute "x"


Similar to above.

JukkaL · 2018-06-18T12:54:40Z

 reveal_type(another) # E: Revealed type is 'y.G[y.G*[builtins.int]]'
 reveal_type(second) # E: Revealed type is 'y.G[builtins.str*]'
-reveal_type(last) # E: Revealed type is 'y.G[y.G*]'
+reveal_type(last) # E: Revealed type is 'y.G[y.G*[Any]]'


Looks like there was a bug that you fixed here?

Looks like there was a bug that you fixed here?

Yes, but it was unintentional :-)

JukkaL · 2018-06-18T13:10:15Z

 [out]
 ==
-c.py:2: error: Argument 1 has incompatible type "int"; expected "str"
+c.py:2: error: Argument 1 to "A" has incompatible type "int"; expected "str"


Nice, we can generate a better error message here.

ilevkivskyi · 2018-06-22T20:45:54Z

Running testpr discovered few bugs here. Fixing them now.

ilevkivskyi · 2018-06-23T00:21:02Z

@JukkaL Thanks for review! I have addressed all comments and fixed some bugs.
This is now ready for review again.

JukkaL · 2018-06-25T12:49:43Z


 -- the order of errors is different with cache
-[case testImportOnTopOfAlias2-skip-cache]
+[case testImportOnTopOfAlias2]


Good to have more test cases enabled.

JukkaL

Looks good, thanks for all the fixes! Only one remaining comment. Feel free to merge once you've addressed it and once you've tested this with internal Dropbox codebases. This will greatly simplify future work with type aliases.

JukkaL · 2018-06-25T12:53:27Z

+            if alias_definition:
+                return AnyType(TypeOfAny.special_form)
+            # This type is invalid in most runtime contexts.
+            kind = (' to Callable' if isinstance(item, CallableType) else


The error message logic would better live in mypy.messages.

Ivan Levkivskyi added 21 commits June 6, 2018 17:58

Start removing some old type alias stuff

e99a5a6

Continue removing some type alias stuff

90e8110

Return TypeAliasExpr; some clean-ups

441bd17

Start working on normalization

31d007c

Merge remote-tracking branch 'upstream/master' into refactor-aliases

fdee785

More fixes

aabfcd4

Revert some runtime logic changes

ed50401

Few more fixes

048b782

Some more fixes

e3a6f3f

Merge remote-tracking branch 'upstream/master' into refactor-aliases

979a759

More test fixes

30d137f

Limit TypeAliasExpr to only r.h.s. of type alias definition

4906cf2

Merge remote-tracking branch 'upstream/master' into refactor-aliases

7c94273

Fix remaining tests

7070e5a

Some more refactoring

440392b

More equivalent treatment of stub files; minor fixes

0ceafb7

Minor fixes formatting

ab9925c

Simplify dependency management

bb42843

Simplify visit_index

e2e166a

Add docstrings and comments

3223653

Fix typo in a docstring

9459b3a

JelleZijlstra reviewed Jun 15, 2018

View reviewed changes

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jun 15, 2018

attempt to not special-case type aliases in typing.pyi

ba7a494

Currently fails terribly, but maybe python/mypy#5221 will fix it.

Preserve alias definition line

63d3f5f

JukkaL reviewed Jun 18, 2018

View reviewed changes

Ivan Levkivskyi added 3 commits June 22, 2018 19:07

Code review: part one

f340632

Merge remote-tracking branch 'upstream/master' into refactor-aliases

b24f636

Code review: part two

490d11a

Ivan Levkivskyi added 6 commits June 23, 2018 00:12

Better name resolution for aliases

26e553b

Allow type aliases to tuples and Any in runtime context

9af332c

Update tests

2af5c7c

Allow importing names over type aliases

bf3f973

Minor fixes

8593d92

One more corner case

85febc9

JukkaL reviewed Jun 25, 2018

View reviewed changes

JukkaL approved these changes Jun 25, 2018

View reviewed changes

Ivan Levkivskyi added 3 commits June 25, 2018 15:03

Move error formatting to messages.py

41c2561

Merge remote-tracking branch 'upstream/master' into refactor-aliases

62e0fd8

Resolve original definition for type aliases

78d668d

ilevkivskyi merged commit 6d8d50c into python:master Jun 25, 2018

ilevkivskyi deleted the refactor-aliases branch June 25, 2018 17:18

glyph mentioned this pull request Jul 13, 2018

type alias to union is invalid in runtime context #5354

Closed

JukkaL mentioned this pull request Nov 1, 2018

Order dependent bug in fine-grained generic alias handling #4903

Closed

Conversation

ilevkivskyi commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented Jun 15, 2018

Uh oh!

ilevkivskyi commented Jun 15, 2018

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Jun 22, 2018

Uh oh!

ilevkivskyi commented Jun 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants