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

Refine parent type when narrowing "lookup" expressions #7917

Merged
merged 9 commits into from Nov 13, 2019

Conversation

@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Nov 9, 2019

This diff adds support for the following pattern:

from typing import Enum, List
from enum import Enum

class Key(Enum):
    A = 1
    B = 2

class Foo:
    key: Literal[Key.A]
    blah: List[int]

class Bar:
    key: Literal[Key.B]
    something: List[str]

x: Union[Foo, Bar]
if x.key is Key.A:
    reveal_type(x)  # Revealed type is 'Foo'
else:
    reveal_type(x)  # Revealed type is 'Bar'

In short, when we do x.key is Key.A, we "propagate" the information we discovered about x.key up one level to refine the type of x.

We perform this propagation only when x is a Union and only when we are doing member or index lookups into instances, typeddicts, namedtuples, and tuples. For indexing operations, we have one additional limitation: we must use a literal expression in order for narrowing to work at all. Using Literal types or Final instances won't work; See #7905 for more details.

To put it another way, this adds support for tagged unions, I guess.

This more or less resolves #7344. We currently don't have support for narrowing based on string or int literals, but that's a separate issue and should be resolved by #7169 (which I resumed work on earlier this week).

This diff adds support for the following pattern:

```python
from typing import Enum, List
from enum import Enum

class Key(Enum):
    A = 1
    B = 2

class Foo:
    key: Literal[Key.A]
    blah: List[int]

class Bar:
    key: Literal[Key.B]
    something: List[str]

x: Union[Foo, Bar]
if x.key is Key.A:
    reveal_type(x)  # Revealed type is 'Foo'
else:
    reveal_type(x)  # Revealed type is 'Bar'
```

In short, when we do `x.key is Key.A`, we "propagate" the information
we discovered about `x.key` up one level to refine the type of `x`.

We perform this propagation only when `x` is a Union and only when we
are doing member or index lookups into instances, typeddicts,
namedtuples, and tuples. For indexing operations, we have one additional
limitation: we *must* use a literal expression in order for narrowing
to work at all. Using Literal types or Final instances won't work;
See #7905 for more details.

To put it another way, this adds support for tagged unions, I guess.

This more or less resolves #7344.
We currently don't have support for narrowing based on string or int
literals, but that's a separate issue and should be resolved by
#7169 (which I resumed work
on earlier this week).
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Nov 9, 2019

A few more notes:

  • I verified this PR produces no new errors in the larger of our two internal codebases. I got lazy and didn't check what happened on the smaller, sorry.
  • I'm planning on submitting docs for this in a separate PR, if that's all right.
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! This is a really great feature (and will be especially useful for people familiar with TypeScript). I just have one larger comment, and few small ones.

test-data/unit/check-narrowing.test Show resolved Hide resolved
test-data/unit/check-narrowing.test Show resolved Hide resolved
reveal_type(x.key) # N: Revealed type is 'Literal[__main__.Key.A]'
reveal_type(x) # N: Revealed type is 'Union[__main__.Object1, Any]'
else:
# TODO: Is this a bug? Should we skip inferring Any for singleton types?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

I think we have some special-casing for None, but generally yes, unions with Any and singletons is indeed problematic.

test-data/unit/check-narrowing.test Outdated Show resolved Hide resolved
@@ -21,7 +21,8 @@ class function: pass
class ellipsis: pass

# We need int and slice for indexing tuples.
class int: pass
class int:
def __neg__(self) -> 'int': pass

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

I assume you need this for a test like if isinstance(t[-1], int): ..., but I can't find this test.

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Nov 11, 2019

Author Collaborator

This was because of the one-line change in checkexpr.py: since we're deducing a type for index expressions now, a few unrelated tests about tuples and negative indices started failing.

if isinstance(expr, StrExpr):
return [expr.value]

# TODO: See if we can eliminate this function and call the below one directly

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

What kind of problems does this cause?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Nov 11, 2019

Author Collaborator

Not sure, I didn't try. It seemed like the kind of refactoring that was out-of-scope for this PR and is something we're already sort of tracking in #6138 and #6123.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

OK, let's leave this out for a separate PR.

mypy/checker.py Show resolved Hide resolved
# Next, try using this information to refine the parent type, if applicable.
# Note that we currently refine just the immediate parent.
#
# TODO: Should we also try recursively refining any parents of the parents?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

I think we should. This was one of the first things I thought of when looking at the PR. Supporting only immediate parents may look arbitrary to a user and potentially cause confusions. Imagine a simple situation:

class Model(Generic[T]):
    attr: T
class A:
    model: Model[int]
class B:
    model: Model[str]
x: Union[A, B]:
if isinstance(x.model.attr, int):
    ...  # I would want 'x' to be an 'A' here

I think we should just cycle up until the expression is NameExpr (IIRC all bindable expressions root there). I don't think this will cause any performance impact (unless you have a proof of the opposite).

Michael0x2a added 2 commits Nov 11, 2019
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Nov 11, 2019

@ilevkivskyi -- Ok, this should be ready for a second look!

I added support for cycling up all parents, as suggested.

Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! Here are few more suggestions.

from mypy import state, errorcodes as codes
from mypy.traverser import has_return_statement, all_return_statements
from mypy.errorcodes import ErrorCode

T = TypeVar('T')

T_contra = TypeVar('T_contra', contravariant=True)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

I don't think the new line above is needed.

We perform this kind of "parent narrowing" for member lookup expressions and indexing
expressions into tuples, namedtuples, and typeddicts. This narrowing is also performed
only once, for the immediate parents of any "lookup" expressions in `new_types`.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

This is now outdated.

reveal_type(x.key) # N: Revealed type is 'Union[Any, Literal[__main__.Key.B]]'
reveal_type(x) # N: Revealed type is 'Union[__main__.Object1, __main__.Object2, Any]'

[case testNarrowingParentsHierarchy]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 11, 2019

Collaborator

Just in case I would add a test involving "double index" like if x['model']['kind'] is ...: ... for nested typed dicts.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 11, 2019

@Michael0x2a also note mypyc build failed.

@Michael0x2a Michael0x2a reopened this Nov 12, 2019
@Michael0x2a Michael0x2a merged commit 648d99a into python:master Nov 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Michael0x2a Michael0x2a deleted the Michael0x2a:add-parent-refinement branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.