Skip to content
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

Narrow types after 'in' operator #4072

Merged
merged 6 commits into from
Oct 13, 2017
Merged

Conversation

ilevkivskyi
Copy link
Member

Fixes #4071

This is separated from #4070 plus added various tests.

ilevkivskyi added a commit to ilevkivskyi/mypy that referenced this pull request Oct 8, 2017
ilevkivskyi added a commit to ilevkivskyi/mypy that referenced this pull request Oct 8, 2017
@JukkaL
Copy link
Collaborator

JukkaL commented Oct 12, 2017

As discussed in #4099 (comment), let's only do this for optional types, since other cases can be unsafe because of operator overloading, and these other cases seem pretty rare anyway.

@ilevkivskyi
Copy link
Member Author

OK, I will do this later this evening or tomorrow morning.

ilevkivskyi added a commit to ilevkivskyi/mypy that referenced this pull request Oct 13, 2017
@ilevkivskyi
Copy link
Member Author

@JukkaL

let's only do this for optional types

Done!

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! Just a few minor comments.

mypy/checker.py Outdated
@@ -2874,6 +2874,27 @@ def remove_optional(typ: Type) -> Type:
return typ


def builtin_item_type(tp: Type) -> Optional[Type]:
# This is only OK for built-in containers, where we know the behavior of __contains__.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docstring.

[case testNarrowTypeAfterInDict]
# flags: --strict-optional
from typing import Dict, Optional
x: Dict[str, str]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a different type for values to make sure that this works against the key type.

# flags: --strict-optional
from typing import List, Optional

x: List[int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for optional item type (e.g. List[Optional[int]]).

@ilevkivskyi
Copy link
Member Author

@JukkaL Thanks for review! I implemented the changes in the new commit.

# flags: --strict-optional
from typing import List, Optional

x: List[Optional[int]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another special case: What if the container is a nested one, say List[List[x]], where x might be Any? How do we deal with various item types, such as List[int] and List[Any]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) If the container is List[Any], then we do nothing (there is a test for this).
b) If the container is List[List[Any]] we narrow the type (provided there is an overlap, this is consistent with how == currently treated), for example:

x: Optional[int]
lst: Optional[List[int]]
nested: List[List[Any]]
if lst in nested:
    reveal_type(lst) # List[int]
if x in nested:
    reveal_type(x) # Optional[int]

There is already a test for non-overlapping items int vs str. I will add one more test for other (nested) types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added one more test.

@JukkaL JukkaL merged commit b875205 into python:master Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants