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 and fails in forward references #3952

Merged
merged 43 commits into from Sep 27, 2017

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Sep 13, 2017

Fixes #3340
Fixes #3419
Fixes #3674
Fixes #3685
Fixes #3799
Fixes #3836
Fixes #3881
Fixes #867
Fixes #2241
Fixes #2399
Fixes #1701
Fixes #3016
Fixes #3054
Fixes #2762
Fixes #3575
Fixes #3990

Currently, forward references don't work with anything apart from classes, for example this doesn't work:

x: A
A = NamedTuple('A', [('x', int)])

The same situation is with TypedDicts, NewTypes, and type aliases. The root problem is that these synthetic types are neither detected in first pass, nor fixed in third pass. In certain cases this can lead to crashes (first six issues above are various crash scenarios). I fix these crashes by applying some additional patches after third pass. Here is the summary of the PR:

  • New simple wrapper type ForwardRef with only one field link is introduced (with updates to type visitors)
  • When an unknown type is found in second pass, the corresponding UnboundType is wrapped in ForwardRef, it is given a "second chance" in third pass.
  • After third pass I record the "suspicious" nodes, where forward references and synthetic types have been encountered and append patches (callbacks) to fix them after third pass. Patches use the new visitor TypeReplacer (which is the core of this PR).

Here are two problems that I encountered:

  • Third pass (both in semanal.py and in typeanal.py) was more "shallow" than the second one, some visitor methods were literally pass. It was necessary to update these to match the "depth" of the second pass.
  • Now third pass has a link to second pass analyzer, self.sem, same as for first pass. It would be nice to refactor the passes since all three share some code/functionality, there is already #3459 to track this.

NOTE: self-referential types are still not properly supported, but now we give a reasonable error for this, not a crash, and they still can be used to certain extent, for example:

class MyNamedTuple(NamedTuple):
    parent: 'MyNamedTuple'

def get_parent(nt: MyNamedTuple) -> MyNamedTuple:
    return nt.parent

x: MyNamedTuple
reveal_type(x.parent)
reveal_type(x[0])

results in

main:2: error: Recursive types not fully supported yet, nested types replaced with "Any"
main:9: error: Revealed type is 'Tuple[Any, fallback=__main__.MyNamedTuple]
main:10: error: Revealed type is 'Tuple[Any, fallback=__main__.MyNamedTuple]

Proper support of recursive types would be much harder (mostly due to (de-)serialization that would require something similar to type_ref), there is a separate issue for this #731.

@JukkaL sorry it took a bit longer than expected because of CPython sprint last week.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 13, 2017

Collaborator

An important comment, for some reason forward references between files still don't work properly (but at least they don't crash).

Collaborator

ilevkivskyi commented Sep 13, 2017

An important comment, for some reason forward references between files still don't work properly (but at least they don't crash).

@ilevkivskyi ilevkivskyi changed the title from Fix crashes and fails in forward references to [WIP] Fix crashes and fails in forward references Sep 13, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 13, 2017

Collaborator

This is ready for review, but I marked it WIP since I have found one more crash, will fix it shortly.

Collaborator

ilevkivskyi commented Sep 13, 2017

This is ready for review, but I marked it WIP since I have found one more crash, will fix it shortly.

Ivan Levkivskyi
Fix type_override; add tests for recursive aliases; fix Callable TODO…
…; Better error in case something is still missing
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 13, 2017

Collaborator

OK, I have fixed some remaining problems and TODO's, also added more tests.

Collaborator

ilevkivskyi commented Sep 13, 2017

OK, I have fixed some remaining problems and TODO's, also added more tests.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 14, 2017

Collaborator

Thank you very much for implementing this! Based on a quick pass, it looks like this fixes many long-standing issues in a clean way. Forward reference handling will be also be useful for fine-grained incremental checking, which we are planning to continue working on later this year. Special thanks for writing many test cases.

I'll try to do a full review by mid next week. I'll also run this against internal Dropbox codebases to see if there are regressions (or performance issues).

Also, there's now a merge conflict. Can you fix it?

Collaborator

JukkaL commented Sep 14, 2017

Thank you very much for implementing this! Based on a quick pass, it looks like this fixes many long-standing issues in a clean way. Forward reference handling will be also be useful for fine-grained incremental checking, which we are planning to continue working on later this year. Special thanks for writing many test cases.

I'll try to do a full review by mid next week. I'll also run this against internal Dropbox codebases to see if there are regressions (or performance issues).

Also, there's now a merge conflict. Can you fix it?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 14, 2017

Collaborator

Also, there's now a merge conflict. Can you fix it?

Fixed.

Special thanks for writing many test cases.

I am glad you like the tests. But this is a subtle area, if you have ideas for more tests, then I will be grateful (I am sure this PR does not fix all issues with forward references, for example across files as I mentioned above, but this could be a good start).

Collaborator

ilevkivskyi commented Sep 14, 2017

Also, there's now a merge conflict. Can you fix it?

Fixed.

Special thanks for writing many test cases.

I am glad you like the tests. But this is a subtle area, if you have ideas for more tests, then I will be grateful (I am sure this PR does not fix all issues with forward references, for example across files as I mentioned above, but this could be a good start).

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 19, 2017

Collaborator

I ran this against internal Dropbox repos and found one minor difference. Previously this code didn't generate an error:

from typing import AnyStr, Dict

def f():   # Note no annotation
    x = {}  # type: Dict[str, AnyStr]

Now it generates this error:

t.py:4: error: Invalid type "typing.AnyStr"

Also I there seems to be no significant performance impact, which is good.

I'll continue tomorrow with a more detailed review.

Collaborator

JukkaL commented Sep 19, 2017

I ran this against internal Dropbox repos and found one minor difference. Previously this code didn't generate an error:

from typing import AnyStr, Dict

def f():   # Note no annotation
    x = {}  # type: Dict[str, AnyStr]

Now it generates this error:

t.py:4: error: Invalid type "typing.AnyStr"

Also I there seems to be no significant performance impact, which is good.

I'll continue tomorrow with a more detailed review.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 20, 2017

Collaborator

@JukkaL I started addressing your comments, I will try to finish before the end of this week.

While making changes you requested, I remembered something important I found before: third pass calls is_subtype and is_same_types. I think this is not the right place to do this, and may be dangerous. Currently I overcome this problem with a hack, but I think we should move these calls to a later stage (either callbacks or type checker) in this PR or in a separate PR.

Collaborator

ilevkivskyi commented Sep 20, 2017

@JukkaL I started addressing your comments, I will try to finish before the end of this week.

While making changes you requested, I remembered something important I found before: third pass calls is_subtype and is_same_types. I think this is not the right place to do this, and may be dangerous. Currently I overcome this problem with a hack, but I think we should move these calls to a later stage (either callbacks or type checker) in this PR or in a separate PR.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Sep 22, 2017

Collaborator

Looks like travis flaked. I restarted that job.

Collaborator

ethanhs commented Sep 22, 2017

Looks like travis flaked. I restarted that job.

ilevkivskyi added some commits Sep 22, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 22, 2017

Collaborator

@JukkaL I think I have now addressed all your comments. This is now ready for further review.

Collaborator

ilevkivskyi commented Sep 22, 2017

@JukkaL I think I have now addressed all your comments. This is now ready for further review.

@gvanrossum gvanrossum referenced this pull request Sep 25, 2017

Closed

Release 0.530 planning #4009

5 of 5 tasks complete
@JukkaL

Thanks for the updates! This seems pretty close to ready. I left a bunch of minor comments.

Show outdated Hide outdated test-data/unit/check-namedtuple.test
Show outdated Hide outdated test-data/unit/check-namedtuple.test
Show outdated Hide outdated test-data/unit/check-statements.test
Show outdated Hide outdated test-data/unit/check-statements.test
Show outdated Hide outdated test-data/unit/check-typeddict.test
Show outdated Hide outdated mypy/semanal.py
Show outdated Hide outdated mypy/typeanal.py
Show outdated Hide outdated test-data/unit/check-namedtuple.test
Show outdated Hide outdated mypy/semanal.py
Show outdated Hide outdated mypy/types.py
@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 26, 2017

Collaborator

Most of my comments from the last round are shown as outdated because I screwed up things a little. I think that they should mostly be still relevant though.

Collaborator

JukkaL commented Sep 26, 2017

Most of my comments from the last round are shown as outdated because I screwed up things a little. I think that they should mostly be still relevant though.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 26, 2017

Collaborator

Most of my comments from the last round are shown as outdated because I screwed up things a little. I think that they should mostly be still relevant though.

By last round you mean few minutes ago ago or few days ago? I see almost everything as outdated, I assume you mean few minutes ago, I received an e-mail from GitHub, so that I can see these comments.

Collaborator

ilevkivskyi commented Sep 26, 2017

Most of my comments from the last round are shown as outdated because I screwed up things a little. I think that they should mostly be still relevant though.

By last round you mean few minutes ago ago or few days ago? I see almost everything as outdated, I assume you mean few minutes ago, I received an e-mail from GitHub, so that I can see these comments.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 27, 2017

Collaborator

@JukkaL I think I now implemented all your recent comments. In addition I fixed one more minor crash on MRO being None for NewTypes and improved tests. This includes:

  • Added more reveal_types here and there.
  • Used shorted and/or less distracting names.
  • Fixed formatting to make tests more compact.
Collaborator

ilevkivskyi commented Sep 27, 2017

@JukkaL I think I now implemented all your recent comments. In addition I fixed one more minor crash on MRO being None for NewTypes and improved tests. This includes:

  • Added more reveal_types here and there.
  • Used shorted and/or less distracting names.
  • Fixed formatting to make tests more compact.

@JukkaL JukkaL merged commit a611b11 into python:master Sep 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 27, 2017

Collaborator

Thanks for the updates! Glad to see so many bugs fixed.

Collaborator

JukkaL commented Sep 27, 2017

Thanks for the updates! Glad to see so many bugs fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment