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

Mismatch of inferred type and return type note #3428

Merged
merged 9 commits into from May 25, 2017

Conversation

Projects
None yet
3 participants
@quartox
Contributor

quartox commented May 23, 2017

Fixes #2529

Not sure about this function sitting in checker.py, might need to move it to messages.py but didn't look into that yet.

I use the context.expr.node.is_inferred to determine if the type of the variable is inferred to determine if the message is printed. Apparently is_inferred is still True even when the adding a type comment in the code. See below:

def i() -> List[Union[int, float]]:
    x = [1] # Type: List[int]
    return x

Specifically, the context.expr.node.is_inferred=True for x when I would expect it to be False.

@JukkaL JukkaL self-requested a review May 23, 2017

@JukkaL

Thanks for the PR! Did a quick review pass.

Show outdated Hide outdated mypy/checker.py
def make_inferred_type_mismatch_note(msg: str, context: Context, subtype: Type,
supertype: Type, supertype_str: str) -> str:
'''The user does not expect an error if the inferred container type is the same as the return

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

Style nit: Use """" for docstrings, and format docstrings like this:

"""Short one-line summary.

Longer descriptions that may span multiple
lines.
"""
@JukkaL

JukkaL May 23, 2017

Collaborator

Style nit: Use """" for docstrings, and format docstrings like this:

"""Short one-line summary.

Longer descriptions that may span multiple
lines.
"""
Show outdated Hide outdated mypy/checker.py
return type. This note suggests that they add a type annotation with the return type instead
of relying on the inferred type.
'''
if (msg.startswith(messages.INCOMPATIBLE_RETURN_VALUE_TYPE) and

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

Style nit: Use indentation like this (double indent for second and later lines of the condition, and only one condition per line):

    if (something and
            another and
            more):
        whatever()
@JukkaL

JukkaL May 23, 2017

Collaborator

Style nit: Use indentation like this (double indent for second and later lines of the condition, and only one condition per line):

    if (something and
            another and
            more):
        whatever()
Show outdated Hide outdated mypy/checker.py
def make_inferred_type_mismatch_note(msg: str, context: Context, subtype: Type,
supertype: Type, supertype_str: str) -> str:

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

It looks like this should be a method in messages.py for consistency.

@JukkaL

JukkaL May 23, 2017

Collaborator

It looks like this should be a method in messages.py for consistency.

Show outdated Hide outdated test-data/unit/check-functions.test
from typing import Union, Dict, List
def f() -> List[Union[int, str]]:
x = ['a']
return x # E: Incompatible return value type (got List[str], expected List[Union[int, str]]) # N: Perhaps a type annotation is missing here? Suggestion: List[Union[int, str]]

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

Use \ to split the long lines so that # N: ... is on a separate line. This makes the test cases easier to read.

@JukkaL

JukkaL May 23, 2017

Collaborator

Use \ to split the long lines so that # N: ... is on a separate line. This makes the test cases easier to read.

Show outdated Hide outdated mypy/checker.py
for subtype_arg, supertype_arg in zip(subtype.args, supertype.args):
if not is_subtype(subtype_arg, supertype_arg):
return ''
return 'Perhaps a type annotation is missing here? Suggestion: ' + supertype_str

This comment has been minimized.

@JukkaL

JukkaL May 23, 2017

Collaborator

Might be better to also mention the name of the variable to make it clear that the proposed annotation is for the variable, not for the return statement. For example, Perhaps you need a type annotation for "x"? Suggestion: List[float]

@JukkaL

JukkaL May 23, 2017

Collaborator

Might be better to also mention the name of the variable to make it clear that the proposed annotation is for the variable, not for the return statement. For example, Perhaps you need a type annotation for "x"? Suggestion: List[float]

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 24, 2017

Contributor

Made all of the fixes but moving it to messages.py

Since I use the is_subtype function from the subtypes module, moving this to messages.py creates a circular import between messages.py and subtypes.py that apparently breaks the imports.

I will try to figure out how to fix this, but might need some ideas.

Contributor

quartox commented May 24, 2017

Made all of the fixes but moving it to messages.py

Since I use the is_subtype function from the subtypes module, moving this to messages.py creates a circular import between messages.py and subtypes.py that apparently breaks the imports.

I will try to figure out how to fix this, but might need some ideas.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 24, 2017

Collaborator

moving this to messages.py creates a circular import between messages.py and subtypes.py that apparently breaks the imports.

If you really need this, then you maybe can import subtypes inside your function. Have you tried this?

Collaborator

ilevkivskyi commented May 24, 2017

moving this to messages.py creates a circular import between messages.py and subtypes.py that apparently breaks the imports.

If you really need this, then you maybe can import subtypes inside your function. Have you tried this?

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 24, 2017

Contributor

Thanks, that did it.

Contributor

quartox commented May 24, 2017

Thanks, that did it.

Show outdated Hide outdated test-data/unit/check-functions.test
def i() -> List[Union[int, float]]:
x = [1] # Type: List[int]
return x # E: Incompatible return value type (got List[int], expected List[Union[int, float]]) \
# N: Perhaps you need a type annotation for "x"? Suggestion: List[Union[int, float]]

This comment has been minimized.

@JukkaL

JukkaL May 24, 2017

Collaborator

Almost there! I'd suggest adding test cases that cover cases that are mostly like these but that don't actually trigger the new message (i.e. one of the conditions in make_inferred_type_note is false).

@JukkaL

JukkaL May 24, 2017

Collaborator

Almost there! I'd suggest adding test cases that cover cases that are mostly like these but that don't actually trigger the new message (i.e. one of the conditions in make_inferred_type_note is false).

This comment has been minimized.

@JukkaL

JukkaL May 24, 2017

Collaborator

One more thing: Can you split the test case into multiple separate test cases? It's getting pretty big now. At least cases where we generate the note and where we don't generate a note could live in separate test cases.

@JukkaL

JukkaL May 24, 2017

Collaborator

One more thing: Can you split the test case into multiple separate test cases? It's getting pretty big now. At least cases where we generate the note and where we don't generate a note could live in separate test cases.

quartox added some commits May 24, 2017

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 24, 2017

Contributor

Added more test cases and separated test cases.

Contributor

quartox commented May 24, 2017

Added more test cases and separated test cases.

@ilevkivskyi

Looks good to me now.

@ilevkivskyi ilevkivskyi merged commit a494197 into python:master May 25, 2017

2 checks passed

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

@quartox quartox deleted the quartox:inferred_mismatch_return branch May 25, 2017

carljm added a commit to carljm/mypy that referenced this pull request May 30, 2017

Merge branch 'master' into module-alias
* master: (23 commits)
  Make return type of open() more precise (#3477)
  Add test cases that delete a file during incremental checking (#3461)
  Parse each format-string component separately (#3390)
  Don't warn about returning Any if it is a proper subtype of the return type (#3473)
  Add __setattr__ support (#3451)
  Remove bundled lib-typing (#3337)
  Move version of extensions to post-release (#3348)
  Fix None slice bounds with strict-optional (#3445)
  Allow NewType subclassing NewType. (#3465)
  Add console scripts (#3074)
  Fix 'variance' label.
  Change label for variance section to just 'variance' (#3429)
  Better error message for invalid package names passed to mypy (#3447)
  Fix last character cut in html-report if file does not end with newline (#3466)
  Print pytest output as it happens (#3463)
  Add mypy roadmap (#3460)
  Add flag to avoid interpreting arguments with a default of None as Optional (#3248)
  Add type checking plugin support for functions (#3299)
  Mismatch of inferred type and return type note (#3428)
  Sync typeshed (#3449)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment