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

Add expected type in incompatible arg errors #3515

Merged
merged 9 commits into from Jul 11, 2017

Conversation

Projects
None yet
4 participants
@miedzinski
Contributor

miedzinski commented Jun 9, 2017

Fixes #3499.

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Jun 9, 2017

Contributor

There is one test failing because we are using format_simple everywhere. Should we skip expected types when dealing with FunctionLike or use format?

Contributor

miedzinski commented Jun 9, 2017

There is one test failing because we are using format_simple everywhere. Should we skip expected types when dealing with FunctionLike or use format?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 9, 2017

Member

I'd start by not adding the "; expected blah" part to the error if format_simple() returns something empty.

Member

gvanrossum commented Jun 9, 2017

I'd start by not adding the "; expected blah" part to the error if format_simple() returns something empty.

@miedzinski

This comment has been minimized.

Show comment
Hide comment
@miedzinski

miedzinski Jun 9, 2017

Contributor

Do we really want to do that? Even without this patch mypy prints malformed messages, e.g.

(mypy-dev) ~/src/mypy [master] λ cat x.py 
from typing import Dict, Callable
def f() -> str:
    return 'foo'
d: Dict[str, Callable[..., int]] = {'bar': f}
(mypy-dev) ~/src/mypy [master] λ mypy x.py 
x.py:4: error: Dict entry 0 has incompatible type "str":

Notice the : at the end. And this doesn't happen with Callables only (which I understand may get long), but even with Type.

I'd go for using format method, just like MessageBuilder.invalid_index_type does.

Contributor

miedzinski commented Jun 9, 2017

Do we really want to do that? Even without this patch mypy prints malformed messages, e.g.

(mypy-dev) ~/src/mypy [master] λ cat x.py 
from typing import Dict, Callable
def f() -> str:
    return 'foo'
d: Dict[str, Callable[..., int]] = {'bar': f}
(mypy-dev) ~/src/mypy [master] λ mypy x.py 
x.py:4: error: Dict entry 0 has incompatible type "str":

Notice the : at the end. And this doesn't happen with Callables only (which I understand may get long), but even with Type.

I'd go for using format method, just like MessageBuilder.invalid_index_type does.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 9, 2017

Collaborator

Using format seems reasonable to me.

Collaborator

JukkaL commented Jun 9, 2017

Using format seems reasonable to me.

@ilevkivskyi

Thanks! This is almost ready to be merged, just two small comments.

# don't increase verbosity unless there is need to do so
from mypy.subtypes import is_subtype
if is_subtype(key_type, expected_key_type):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 25, 2017

Collaborator

Dict is invariant both in key and value types. Therefore is_subtype is probably not what you want here, maybe is_same_type from sametypes.py?

@ilevkivskyi

ilevkivskyi Jun 25, 2017

Collaborator

Dict is invariant both in key and value types. Therefore is_subtype is probably not what you want here, maybe is_same_type from sametypes.py?

expected_value_type_str = self.format(expected_value_type)
else:
value_type_str, expected_value_type_str = self.format_distinctly(
value_type, expected_value_type)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 25, 2017

Collaborator

I would add few new tests checking that this verbosity logic is followed correctly in different scenarios (taking into account mentioned invariance).

@ilevkivskyi

ilevkivskyi Jun 25, 2017

Collaborator

I would add few new tests checking that this verbosity logic is followed correctly in different scenarios (taking into account mentioned invariance).

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 11, 2017

Collaborator

@miedzinski After some thinking, it looks like your initial design was actually right and it is OK to use is_subtype in the context of inference for dict literals. Also the added tests don't trigger the new error message (this is related). So that I would restore the is_subtype and rewrite the added tests like you proposed on gitter.

Sorry for troubles.

Collaborator

ilevkivskyi commented Jul 11, 2017

@miedzinski After some thinking, it looks like your initial design was actually right and it is OK to use is_subtype in the context of inference for dict literals. Also the added tests don't trigger the new error message (this is related). So that I would restore the is_subtype and rewrite the added tests like you proposed on gitter.

Sorry for troubles.

miedzinski added some commits Jul 11, 2017

@ilevkivskyi

As I understand the pre-release code freeze is over, so that I will merge this when tests pass.

@ilevkivskyi ilevkivskyi merged commit c8bfb65 into python:master Jul 11, 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