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

Fix crashes in synthetic types #3322

Merged
merged 14 commits into from May 24, 2017

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented May 4, 2017

UPDATE: see #3322 (comment) below.

Fixes #3308

This PR adds better processing of "synthetic" types (NewType, NamedTuple, TypedDict) to the third pass and moves some processing from the second to the third pass. As @JukkaL noticed, this is still not perfect, since some MROs are recomputed in the third pass. I think the right way to fix this is just use unions in this case, the point is that users will practically never see these fallbacks, and this will also allow us to have better type expansions for fallbacks of generic tuple types. @JukkaL what do you think?

Additional fourth pass seems to be an overkill for this. Although, I have a big project in my future plans (probably after we are more or less done with protocols): providing better support for forward references (there are a dozen similar issues about this) and recursive types. Even for this fourth pass seems to be a bit an overkill, and I would rather proceed as Jukka proposed in option (3) in #1701 (comment)

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL May 4, 2017

Collaborator

Using unions as fallbacks for typed dicts has the problem that the union could be arbitrarily complex for a typed dict with multiple nested typed dicts, etc. It's actually just the kind of situation where unions may not be a good option. My current thinking is basically like this:

  • If there are generally at most 2 or 3 items, a union is probably okay. Examples: inferring Optional[x]; 1 if cond() else 'x' -> infer Union[int, str] (the latter doesn't work yet though).
  • If there can be a large number of items, a join (which doesn't usually produce unions) would be better. For example, we shouldn't infer unions from list expressions [x, y, ...] since there could be dozens of items, each with a slightly different type.

Also, we use is_subtype checks during the third pass of semantic analysis (in typeanal.py) and these are also probably not good. A simple approach that wouldn't perhaps require a full new pass would be to build a list of deferred lambdas/closures during pass 3 that would perform the final checks/computations without having to do AST traversal. Then pass 4 would just call these closures. However, even this might not be enough -- we may have to perform multiple runs of certain passes, I think. For example, we can't necessarily calculate a join until we have all the fallbacks, and we can't calculate fallbacks until we can do joins, so finding the right order of these operations can be hard. We could perhaps detect incomplete types and defer operations on them to another pass.

Collaborator

JukkaL commented May 4, 2017

Using unions as fallbacks for typed dicts has the problem that the union could be arbitrarily complex for a typed dict with multiple nested typed dicts, etc. It's actually just the kind of situation where unions may not be a good option. My current thinking is basically like this:

  • If there are generally at most 2 or 3 items, a union is probably okay. Examples: inferring Optional[x]; 1 if cond() else 'x' -> infer Union[int, str] (the latter doesn't work yet though).
  • If there can be a large number of items, a join (which doesn't usually produce unions) would be better. For example, we shouldn't infer unions from list expressions [x, y, ...] since there could be dozens of items, each with a slightly different type.

Also, we use is_subtype checks during the third pass of semantic analysis (in typeanal.py) and these are also probably not good. A simple approach that wouldn't perhaps require a full new pass would be to build a list of deferred lambdas/closures during pass 3 that would perform the final checks/computations without having to do AST traversal. Then pass 4 would just call these closures. However, even this might not be enough -- we may have to perform multiple runs of certain passes, I think. For example, we can't necessarily calculate a join until we have all the fallbacks, and we can't calculate fallbacks until we can do joins, so finding the right order of these operations can be hard. We could perhaps detect incomplete types and defer operations on them to another pass.

@ilevkivskyi ilevkivskyi changed the title from Fix crashes in synthetic types to [WIP] Fix crashes in synthetic types May 4, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 4, 2017

Collaborator

I am marking this as work-in-progress until we resolve the question of third/fourth pass, also I have discovered some spurious errors with this PR in --strict-optional mode, because of incompatibility of tuple fallbacks.

Collaborator

ilevkivskyi commented May 4, 2017

I am marking this as work-in-progress until we resolve the question of third/fourth pass, also I have discovered some spurious errors with this PR in --strict-optional mode, because of incompatibility of tuple fallbacks.

Ivan Levkivskyi added some commits May 9, 2017

Ivan Levkivskyi
Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 9, 2017

Collaborator

@JukkaL
I removed the controversial part from this PR, now it only fixes crashes reported in #3308

The rest can be fixed later in a separate PR when we will decide what to do with fourth pass.

Collaborator

ilevkivskyi commented May 9, 2017

@JukkaL
I removed the controversial part from this PR, now it only fixes crashes reported in #3308

The rest can be fixed later in a separate PR when we will decide what to do with fourth pass.

@ilevkivskyi ilevkivskyi changed the title from [WIP] Fix crashes in synthetic types to Fix crashes in synthetic types May 9, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 23, 2017

Member

Can you resolve the merge conflict?

Member

gvanrossum commented May 23, 2017

Can you resolve the merge conflict?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 23, 2017

Collaborator

Can you resolve the merge conflict?

Resolved.

Collaborator

ilevkivskyi commented May 23, 2017

Can you resolve the merge conflict?

Resolved.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

@gvanrossum I have fixed the merge conflict, do you have any comments on this?

Collaborator

ilevkivskyi commented May 24, 2017

@gvanrossum I have fixed the merge conflict, do you have any comments on this?

@gvanrossum

Really, all looks fine, just style nits. Thanks!

@@ -731,6 +731,7 @@ class ClassDef(Statement):
info = None # type: TypeInfo # Related TypeInfo
metaclass = '' # type: Optional[str]
decorators = None # type: List[Expression]
analyzed = None # type: Optional[Expression]

This comment has been minimized.

@gvanrossum

gvanrossum May 24, 2017

Member

Does this need an update to serialize() (perhaps in the comment)?

@gvanrossum

gvanrossum May 24, 2017

Member

Does this need an update to serialize() (perhaps in the comment)?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

I think we don't need to serialize this (since it is needed only for semantic analysis). I updated the comment.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

I think we don't need to serialize this (since it is needed only for semantic analysis). I updated the comment.

Show outdated Hide outdated mypy/semanal.py
Show outdated Hide outdated mypy/semanal.py
Show outdated Hide outdated test-data/unit/check-classes.test
y1 = NM(x=[])
reveal_type(x) # E: Revealed type is 'TypedDict(x=builtins.list[Any], _fallback=__main__.TD)'
reveal_type(x1) # E: Revealed type is 'TypedDict(x=builtins.list[Any], _fallback=typing.Mapping[builtins.str, builtins.list[Any]])'
reveal_type(y) # E: Revealed type is 'Tuple[builtins.list[Any], fallback=__main__.NM]'

This comment has been minimized.

@gvanrossum

gvanrossum May 24, 2017

Member

No reveal for y1?

@gvanrossum

gvanrossum May 24, 2017

Member

No reveal for y1?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

Added it.

@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

Added it.

[out]
-- The two tests below will not crash after
-- https://github.com/python/mypy/issues/3319 is fixed

This comment has been minimized.

@gvanrossum

gvanrossum May 24, 2017

Member

Please remember to remove the -skip then.

@gvanrossum

gvanrossum May 24, 2017

Member

Please remember to remove the -skip then.

Show outdated Hide outdated test-data/unit/check-classes.test
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

@gvanrossum Thanks for review! I fixed all points. (For some reasons Travis is now much slower than AppVeyor.)

Collaborator

ilevkivskyi commented May 24, 2017

@gvanrossum Thanks for review! I fixed all points. (For some reasons Travis is now much slower than AppVeyor.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 24, 2017

Member

Will merge once tests pass. Perhaps Travis is squeezing the python project because the sprints cause many runs for many repos in the project..
.?

Member

gvanrossum commented May 24, 2017

Will merge once tests pass. Perhaps Travis is squeezing the python project because the sprints cause many runs for many repos in the project..
.?

@gvanrossum gvanrossum merged commit ccd95e6 into python:master May 24, 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