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

Simplify and tighten type aliases #3524

Merged
merged 18 commits into from Jul 7, 2017

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Jun 11, 2017

Fixes #2887
Fixes #3191
In addition this prohibits reassigning aliases, before something like this was allowed:

if random():
    Alias = Sequence[int]
else:
    Alias = Sequence[float]

def fun(arg: Alias) -> None:
    ...

Now this will say: Cannot assign multiple types to name "Alias" without an explicit "Type[...]" annotation. See #3494 for background.

Finally, this simplifies the logic in semanal.py, so that most processing of type aliases happens in one function.

@ilevkivskyi ilevkivskyi changed the title from Simplify and tighten type aliases to [WIP] Simplify and tighten type aliases Jun 15, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 15, 2017

Collaborator

Marked this as WIP, since I have found another place (in checkexpr.py) where type variables resolution should be updated.

Collaborator

ilevkivskyi commented Jun 15, 2017

Marked this as WIP, since I have found another place (in checkexpr.py) where type variables resolution should be updated.

@ilevkivskyi ilevkivskyi changed the title from [WIP] Simplify and tighten type aliases to Simplify and tighten type aliases Jun 22, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 22, 2017

Collaborator

OK, I fixed remaining points (also this PR now may help with #3355)
This is ready for review now.

Collaborator

ilevkivskyi commented Jun 22, 2017

OK, I fixed remaining points (also this PR now may help with #3355)
This is ready for review now.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 23, 2017

Member

I fear that @JukkaL will have to review this, but I ran this against our Dropbox codebase, and found some occurrences of the following pattern:

class A: ...
class B: ...
def f(a: bool) -> None:
    cls = A
    if a:
        cls = B

We get several errors on the last line:

__tmp__.py:6: error: Cannot assign to a type
__tmp__.py:6: error: Cannot assign multiple types to name "c" without an explicit "Type[...]" annotation
__tmp__.py:6: error: Incompatible types in assignment (expression has type Type[B], variable has type Type[A])

Previously this code passed.

Member

gvanrossum commented Jun 23, 2017

I fear that @JukkaL will have to review this, but I ran this against our Dropbox codebase, and found some occurrences of the following pattern:

class A: ...
class B: ...
def f(a: bool) -> None:
    cls = A
    if a:
        cls = B

We get several errors on the last line:

__tmp__.py:6: error: Cannot assign to a type
__tmp__.py:6: error: Cannot assign multiple types to name "c" without an explicit "Type[...]" annotation
__tmp__.py:6: error: Incompatible types in assignment (expression has type Type[B], variable has type Type[A])

Previously this code passed.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 23, 2017

Collaborator

@gvanrossum
I tried your example on master, and it gives an error:

Incompatible types in assignment (expression has type Type[B], variable has type Type[A])

probably B needs to inherit from A in order to be safe. The point is that previously type aliases were never created at function scope (only variables with types like Type[...]). I changed this to be in line with how aliases behave at module scope (i.e. depending on the presence of an explicit annotation).

But after you mention it is actually used like this, I think maybe it makes sense. If Jukka also agrees, then I will change this back (it is only one line). To me both behaviours make sense, on one hand explicit is better than explicit plus there will be less exceptions to explain, on another hand, it would be boring and probably un-Pythonic to put explicit annotations everywhere.

Collaborator

ilevkivskyi commented Jun 23, 2017

@gvanrossum
I tried your example on master, and it gives an error:

Incompatible types in assignment (expression has type Type[B], variable has type Type[A])

probably B needs to inherit from A in order to be safe. The point is that previously type aliases were never created at function scope (only variables with types like Type[...]). I changed this to be in line with how aliases behave at module scope (i.e. depending on the presence of an explicit annotation).

But after you mention it is actually used like this, I think maybe it makes sense. If Jukka also agrees, then I will change this back (it is only one line). To me both behaviours make sense, on one hand explicit is better than explicit plus there will be less exceptions to explain, on another hand, it would be boring and probably un-Pythonic to put explicit annotations everywhere.

@ilevkivskyi ilevkivskyi requested a review from JukkaL Jun 23, 2017

@ilevkivskyi ilevkivskyi assigned JukkaL and unassigned JukkaL Jun 23, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 23, 2017

Member

You're right, in the actual code this has class B(A). And that passes in master but still gives two errors with your PR:

__tmp__.py:6: error: Cannot assign to a type
__tmp__.py:6: error: Cannot assign multiple types to name "cls" without an explicit "Type[...]" annotation

The solution would be to start with cls: Type[A] = A which is both verbose and a bit unintuitive.

Member

gvanrossum commented Jun 23, 2017

You're right, in the actual code this has class B(A). And that passes in master but still gives two errors with your PR:

__tmp__.py:6: error: Cannot assign to a type
__tmp__.py:6: error: Cannot assign multiple types to name "cls" without an explicit "Type[...]" annotation

The solution would be to start with cls: Type[A] = A which is both verbose and a bit unintuitive.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 23, 2017

Collaborator

@gvanrossum

The solution would be to start with cls: Type[A] = A which is both verbose and a bit unintuitive.

OK, I will revert this soon. (But we need to document that type aliases should be defined at the module level, at a class and function scope they are always automatically variables with type Type[...], there is #3494 for the documentation part, I will make a separate PR).

While we are waiting for review by @JukkaL, could you please test whether this PR helps with #3355?
(There is still no definitive repro, but you said that the crash happens in your internal codebase).

Collaborator

ilevkivskyi commented Jun 23, 2017

@gvanrossum

The solution would be to start with cls: Type[A] = A which is both verbose and a bit unintuitive.

OK, I will revert this soon. (But we need to document that type aliases should be defined at the module level, at a class and function scope they are always automatically variables with type Type[...], there is #3494 for the documentation part, I will make a separate PR).

While we are waiting for review by @JukkaL, could you please test whether this PR helps with #3355?
(There is still no definitive repro, but you said that the crash happens in your internal codebase).

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 23, 2017

Member

I wish I had a repro for #3355, but it's worse -- it occasionally happens to other users when they use --quick mode, but I haven't found a way to trigger it reliably myself yet. :-(

Member

gvanrossum commented Jun 23, 2017

I wish I had a repro for #3355, but it's worse -- it occasionally happens to other users when they use --quick mode, but I haven't found a way to trigger it reliably myself yet. :-(

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 29, 2017

Collaborator

@gvanrossum As promised, I removed creation of aliases at function scope.

Collaborator

ilevkivskyi commented Jun 29, 2017

@gvanrossum As promised, I removed creation of aliases at function scope.

ilevkivskyi and others added some commits Jul 4, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 4, 2017

Collaborator

@JukkaL I fixed the merge problem, this should be ready for review again.

Collaborator

ilevkivskyi commented Jul 4, 2017

@JukkaL I fixed the merge problem, this should be ready for review again.

@ilevkivskyi ilevkivskyi referenced this pull request Jul 4, 2017

Closed

Release mypy 0.520 #3655

@JukkaL

Mostly looks good, just a bunch of minor things.

from typing import Tuple
from lib import Transform
def int_tf(m: int) -> Transform[int, str]:

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Also use reveal_type to check that the declared type resulting from Transform[x, y] is as expected.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Also use reveal_type to check that the declared type resulting from Transform[x, y] is as expected.

Show outdated Hide outdated mypy/semanal.py
# when this type alias gets "inlined", the Any is not explicit anymore,
# so we need to replace it with non-explicit Anys
res = make_any_non_explicit(res)
if isinstance(res, Instance) and not res.args and isinstance(rvalue, RefExpr):

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment that describes the purpose of this special case.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment that describes the purpose of this special case.

@@ -1643,26 +1623,59 @@ def alias_fallback(self, tp: Type) -> Instance:
fb_info.mro = [fb_info, self.object_type().type]
return Instance(fb_info, [])
def analyze_alias(self, rvalue: Expression,
allow_unnormalized: bool) -> Tuple[Optional[Type], List[str]]:

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add docstring. Explain arguments and the return value.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add docstring. Explain arguments and the return value.

Show outdated Hide outdated mypy/nodes.py
@@ -2267,6 +2268,7 @@ class SymbolTableNode:
mod_id = '' # type: Optional[str]
# If this not None, override the type of the 'node' attribute.
type_override = None # type: Optional[mypy.types.Type]
alias_tvars = None # type: Optional[List[str]]

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment.

Are these short or full names? If short ones, it would be more correct to have full names. For example, a.T and b.T could refer to different type variables -- this is very unlikely, though.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment.

Are these short or full names? If short ones, it would be more correct to have full names. For example, a.T and b.T could refer to different type variables -- this is very unlikely, though.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

It looks like these are qualified names. I added a test for this, and a comment here.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

It looks like these are qualified names. I added a test for this, and a comment here.

Show outdated Hide outdated mypy/fixup.py
@@ -88,6 +88,9 @@ def visit_symbol_table(self, symtab: SymbolTable) -> None:
if stnode is not None:
value.node = stnode.node
value.type_override = stnode.type_override
if not value.type_override and self.quick_and_dirty:

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

I don't understand this -- add a comment. Is there a test case that tests this change?

@JukkaL

JukkaL Jul 6, 2017

Collaborator

I don't understand this -- add a comment. Is there a test case that tests this change?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Now I am also not sure it is needed. It was an attempt to fix #3355, but now I have a better idea. I removed this for now, and will make a separate PR later.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Now I am also not sure it is needed. It was an attempt to fix #3355, but now I have a better idea. I removed this for now, and will make a separate PR later.

@@ -2342,6 +2346,7 @@ def serialize(self, prefix: str, name: str) -> JsonDict:
data['node'] = self.node.serialize()
if self.type_override is not None:
data['type_override'] = self.type_override.serialize()
data['alias_tvars'] = self.alias_tvars

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Is there a test case that triggers this (and the corresponding deserialization bit)?

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Is there a test case that triggers this (and the corresponding deserialization bit)?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Good catch! Added an incremental test.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Good catch! Added an incremental test.

Show outdated Hide outdated mypy/semanal.py
node.kind = TYPE_ALIAS
node.type_override = res
node.alias_tvars = alias_tvars
if isinstance(s.rvalue, IndexExpr):

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment that explains the motivation of this if statement.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add comment that explains the motivation of this if statement.

from typing import TypeVar, Sequence, Type
T = TypeVar('T')
A: Type[float] = int

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Also test assignment after initialization (it should be fine since there is an explicit annotation)?

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Also test assignment after initialization (it should be fine since there is an explicit annotation)?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Yes, added it below.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

Yes, added it below.

[case testGenericTypeAliasesImportingWithoutTypeVarError]
from a import Alias
x: Alias[int, str] # E: Bad number of arguments for type alias, expected: 1, given: 2

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add reveal_type(x) to see what type gets assigned in this case.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

Add reveal_type(x) to see what type gets assigned in this case.

Show outdated Hide outdated mypy/semanal.py
def check_and_set_up_type_alias(self, s: AssignmentStmt) -> None:
"""Check if assignment creates a type alias and set it up as needed."""
# For now, type aliases only work at the top level of a module.
# Type aliases are created only at module scope, at class and function scopes
# assignments create class valued members and local variables with type object types.

This comment has been minimized.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

This caused an error in an internal Dropbox codebase. We have code that looks like this:

from typing import Union

class A:
    B = Union[int, str]

    def f(self) -> B: # this PR makes this an invalid type
        pass

Maybe type aliases in the class body should still be supported? Alternatively, maybe anything where the rvalue is a valid type that is an IndexExpr should still be considered as a type alias, i.e. only restrict simple cases like X = Y.

@JukkaL

JukkaL Jul 6, 2017

Collaborator

This caused an error in an internal Dropbox codebase. We have code that looks like this:

from typing import Union

class A:
    B = Union[int, str]

    def f(self) -> B: # this PR makes this an invalid type
        pass

Maybe type aliases in the class body should still be supported? Alternatively, maybe anything where the rvalue is a valid type that is an IndexExpr should still be considered as a type alias, i.e. only restrict simple cases like X = Y.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

OK, now the rules a following: explicit annotation -- always a variable, otherwise:

  • module scope -- always an alias
  • class scope -- an alias for subscripted, a variable for simple class objects
  • function scope -- always a variable
@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

OK, now the rules a following: explicit annotation -- always a variable, otherwise:

  • module scope -- always an alias
  • class scope -- an alias for subscripted, a variable for simple class objects
  • function scope -- always a variable
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 7, 2017

Collaborator

@JukkaL Thanks for review! All comments should be addressed now.

Collaborator

ilevkivskyi commented Jul 7, 2017

@JukkaL Thanks for review! All comments should be addressed now.

@JukkaL

JukkaL approved these changes Jul 7, 2017

Thanks, now looks good!

@JukkaL JukkaL merged commit 70d5f45 into python:master Jul 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment