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

Check code after inferred UninhabitedType #4059

Merged
merged 2 commits into from Oct 20, 2017

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Oct 4, 2017

Fixes #3994

Currently code after inferred UninhabitedType is silently skipped, for example no errors are shown in this code:

T = TypeVar('T')
def f(x: List[T]) -> T:
    ...

class C:
    x = f([])
    def m(self) -> str:
        return 42 # No error!

if bool():
    f([])
    1 + ''  # No error!

There are other scenarios in tests. With this PR errors are shown as expected, since UninhabitedTypes resulting from ambiguous inference don't influence binder.

@ilevkivskyi ilevkivskyi referenced this pull request Oct 9, 2017

Closed

Release 0.540 planning #4084

@JukkaL JukkaL self-assigned this Oct 11, 2017

@JukkaL

Thanks for fixing this! It's important to fix false negatives, since we want users to be able to trust type checking results. I have just one small comment.

Show outdated Hide outdated mypy/types.py
Ivan Levkivskyi
@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 11, 2017

Collaborator

This generates a bunch of new errors in Dropbox internal codebases. Since this fixes false negatives, they may just be issues in our code, but I'll have to verify it.

Collaborator

JukkaL commented Oct 11, 2017

This generates a bunch of new errors in Dropbox internal codebases. Since this fixes false negatives, they may just be issues in our code, but I'll have to verify it.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 11, 2017

Collaborator

This generates a bunch of new errors in Dropbox internal codebases. Since this fixes false negatives, they may just be issues in our code, but I'll have to verify it.

I think this PR shouldn't bring anything false positive, but will be interested to see your findings.

Collaborator

ilevkivskyi commented Oct 11, 2017

This generates a bunch of new errors in Dropbox internal codebases. Since this fixes false negatives, they may just be issues in our code, but I'll have to verify it.

I think this PR shouldn't bring anything false positive, but will be interested to see your findings.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 12, 2017

Collaborator

This PR seems to cause an invalid type to be inferred for the following code:

from typing import Any

class C(dict):
    def f(self, x: Any) -> None:
        y = super(C, x).__new__(x)
        reveal_type(y)  # None, which is unexpected

Previously the reveal_type line was skipped. The underlying issue doesn't seem new though, and similar problems also happen on master.

This works unexpectedly also on master (even when using --strict-optional):

from typing import TypeVar

T = TypeVar('T')

def f() -> T: pass

x = f()
reveal_type(x)  # None

The issue seems rare enough that it doesn't block merging this, but it would be nice to at least understand what is going on.

Collaborator

JukkaL commented Oct 12, 2017

This PR seems to cause an invalid type to be inferred for the following code:

from typing import Any

class C(dict):
    def f(self, x: Any) -> None:
        y = super(C, x).__new__(x)
        reveal_type(y)  # None, which is unexpected

Previously the reveal_type line was skipped. The underlying issue doesn't seem new though, and similar problems also happen on master.

This works unexpectedly also on master (even when using --strict-optional):

from typing import TypeVar

T = TypeVar('T')

def f() -> T: pass

x = f()
reveal_type(x)  # None

The issue seems rare enough that it doesn't block merging this, but it would be nice to at least understand what is going on.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Oct 12, 2017

Collaborator

The issue seems rare enough that it doesn't block merging this, but it would be nice to at least understand what is going on.

I don't know if it is related or not, but I have seen some problems in meet.py with None and UninhabitedType. Another possible suspect is in partial None types, I remember looking at it recently I didn't really like how NoneTyp vs UninhabitedType is treated, I will check tomorrow morning if this can be fixed quickly, if not, then I propose to merge this and take a look at this problem later (you already opened an issue about this).

Collaborator

ilevkivskyi commented Oct 12, 2017

The issue seems rare enough that it doesn't block merging this, but it would be nice to at least understand what is going on.

I don't know if it is related or not, but I have seen some problems in meet.py with None and UninhabitedType. Another possible suspect is in partial None types, I remember looking at it recently I didn't really like how NoneTyp vs UninhabitedType is treated, I will check tomorrow morning if this can be fixed quickly, if not, then I propose to merge this and take a look at this problem later (you already opened an issue about this).

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 12, 2017

Collaborator
Collaborator

JukkaL commented Oct 12, 2017

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Oct 13, 2017

Collaborator

Now that #4112 was merged, the motivating example generates an error (though not all errors that we'd like) so this seems a little less urgent now. I may postpone this until the next release.

Collaborator

JukkaL commented Oct 13, 2017

Now that #4112 was merged, the motivating example generates an error (though not all errors that we'd like) so this seems a little less urgent now. I may postpone this until the next release.

@JukkaL

JukkaL approved these changes Oct 20, 2017

@JukkaL JukkaL merged commit 1673e27 into python:master Oct 20, 2017

2 checks passed

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

JukkaL added a commit that referenced this pull request Oct 20, 2017

JukkaL added a commit that referenced this pull request Oct 20, 2017

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