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

Infer better type for call to overload with Any actual arguments #3917

Merged
merged 4 commits into from Sep 5, 2017

Conversation

Projects
None yet
2 participants
@JukkaL
Collaborator

JukkaL commented Sep 5, 2017

If the overload return type is ambiguous and there is an Any actual
argument that seems to cause the ambiguity, infer Any as the type of
the call expression instead of some precise type. Previously we
sometimes inferred a precise type even though the type was ambiguous,
resulting in false positives.

This is not quite the most correct implementation since we sometimes
incorrectly assume ambiguity caused by Any even though none actually
exists. This is acceptable since this only affects calls with Any
arguments that are imprecise anyway. Also, the correct logic would be
quite complex, hard to test and probably not worth it, at least right
now.

Fixes #3662.

Infer better type for call to overload with Any actual arguments
If the overload return type is ambiguous and there is an `Any` actual
argument that seems to cause the ambiguity, infer `Any` as the type of
the call expression instead of some precise type. Previously we
sometimes inferred a precise type even though the type was ambiguous,
resulting in false positives.

This is not quite the most correct implementation since we sometimes
incorrectly assume ambiguity caused by `Any` even though none actually
exists. This is acceptable since this only affects calls with `Any`
arguments that are imprecise anyway. Also, the correct logic would be
quite complex, hard to test and probably not worth it, at least right
now.

Fixes #3662.
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

Somehow it seems to me that inferring the widest return type (if there is one) instead of Any might be better, although I see that this still can cause spurious errors if this return type will appear in a contravariant position. So all in all I think this is fine for now, since this problem blocks #3300 which is important.

Collaborator

ilevkivskyi commented Sep 5, 2017

Somehow it seems to me that inferring the widest return type (if there is one) instead of Any might be better, although I see that this still can cause spurious errors if this return type will appear in a contravariant position. So all in all I think this is fine for now, since this problem blocks #3300 which is important.

Show outdated Hide outdated mypy/checkexpr.py
reveal_type(f(1, a, a)) # E: Revealed type is 'builtins.int'
reveal_type(f('', a, a)) # E: Revealed type is 'builtins.object'
# Like above, but use keyword arguments.
reveal_type(f(y=1, z='', x=a)) # E: Revealed type is 'Any'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would add one more test where the order of arguments is shuffled, just to be sure that the matching logic works right.

@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

I would add one more test where the order of arguments is shuffled, just to be sure that the matching logic works right.

This comment has been minimized.

@JukkaL

JukkaL Sep 5, 2017

Collaborator

Added another call where arguments are shuffled

@JukkaL

JukkaL Sep 5, 2017

Collaborator

Added another call where arguments are shuffled

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Sep 5, 2017

Collaborator

The widest type may also generate false positives, since the wider type may be missing some attributes defined in the narrower type. Also, if a user creates an invariant container such as a list based on the result, it won't be compatible if the expected type is the narrower one.

Collaborator

JukkaL commented Sep 5, 2017

The widest type may also generate false positives, since the wider type may be missing some attributes defined in the narrower type. Also, if a user creates an invariant container such as a list based on the result, it won't be compatible if the expected type is the narrower one.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

Also, if a user creates an invariant container such as a list based on the result, it won't be compatible if the expected type is the narrower one.

Yes, this is more important that contravariant, since invariant containers are abundant.

Collaborator

ilevkivskyi commented Sep 5, 2017

Also, if a user creates an invariant container such as a list based on the result, it won't be compatible if the expected type is the narrower one.

Yes, this is more important that contravariant, since invariant containers are abundant.

@ilevkivskyi ilevkivskyi merged commit b724edc into master Sep 5, 2017

3 checks passed

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

@ilevkivskyi ilevkivskyi deleted the any-with-overload branch Sep 5, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Sep 5, 2017

Collaborator

OK, now we can go with #3300

Collaborator

ilevkivskyi commented Sep 5, 2017

OK, now we can go with #3300

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