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

Add union math for intelligent indexing #6558

Merged

Conversation

@Michael0x2a
Copy link
Collaborator

commented Mar 17, 2019

This pull request hacks on supports for union math when using Unions of Literals to index into tuples, NamedTuples, and TypedDicts.

It also fixes a bug I apparently introduced. Currently, mypy correctly reports an error with this code:

class Test(TypedDict):
    foo: int

t: Test

# Error: int can't be assigned a str value
t.setdefault("foo", "unrelated value")

...but does not report an error with:

key: Literal["foo"]
t.setdefault(key, "unrelated value")

This diff should make mypy report an error in both cases.

Resolves #6262.

@ilevkivskyi
Copy link
Collaborator

left a comment

The general idea looks good, but I will not have time to make a detailed review before vacation. I hope others will review this.

@Michael0x2a Michael0x2a force-pushed the Michael0x2a:add-union-math-for-intelligent-indexing branch from 57caca5 to d198f38 Mar 31, 2019

@Michael0x2a Michael0x2a force-pushed the Michael0x2a:add-union-math-for-intelligent-indexing branch from d198f38 to e134241 Jun 8, 2019

@Michael0x2a Michael0x2a referenced this pull request Jul 2, 2019

ilevkivskyi added a commit that referenced this pull request Jul 4, 2019

Support method plugin hooks on unions (#6560)
Fixes #6117
Fixes #5930

Currently both our plugin method hooks don't work with unions. This PR fixes this with three things:
* Moves a bit of logic from `visit_call_expr_inner()` (which is a long method already) to `check_call_expr_with_callee_type()` (which is a short method).
* Special-cases unions in `check_call_expr_with_callee_type()` (normal method calls) and `check_method_call_by_name()` (dunder/operator method calls).
* Adds some clarifying comments and a docstring.

The week point is interaction with binder, but IMO this is the best we can have for now. I left a comment mentioning that check for overlap should be consistent in two functions.

In general, I don't like special-casing, but I spent several days thinking of other solutions, and it looks like special-casing unions in couple more places is the only reasonable way to fix unions-vs-plugins interactions.

This PR may interfere with #6558 that fixes an "opposite" problem, hopefully they will work together unmodified, so that accessing union of literals on union of typed dicts works. Whatever PR lands second, should add a test for this.
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

Now that #6560 is merged, could you please add tests to check interactions between the two features? Then I will take a look at this.

Michael0x2a added some commits Mar 17, 2019

Add union math for intelligent indexing
This pull request hacks on supports for union math when using Unions of
Literals to index into tuples, NamedTuples, and TypedDicts.

It also fixes a bug I apparently introduced. Currently, mypy correctly
reports an error with this code:

    class Test(TypedDict):
        foo: int

    t: Test

    # Error: int can't be assigned a str value
    t.setdefault("foo", "unrelated value")

...but does not report an error with:

    key: Literal["foo"]
    t.setdefault(key, "unrelated value")

This diff should make mypy report an error in both cases.

Resolves #6262.

@Michael0x2a Michael0x2a force-pushed the Michael0x2a:add-union-math-for-intelligent-indexing branch from e134241 to be7210f Jul 4, 2019

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

@ilevkivskyi -- ok, I added in a test testing a few basic interactions with TypedDicts. It didn't seem like this PR and #6560 really interfered with one another (as we hoped, I didn't have to make any code changes), so I stuck to just adding some basic sanity checks. LMK if you want something more in-depth.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks! Looks good, I just have few suggestions for more docs and possible refactoring to reduce code (or rather logic) duplication.

Could you please also check this on internal code bases before merging? (I didn't do this with my unions PR and it caused some fallout.)

@@ -2607,40 +2617,59 @@ def nonliteral_tuple_index_helper(self, left_type: TupleType, index: Expression)
else:
return union

def _get_value(self, index: Expression) -> Optional[int]:
def _get_values(self, index: Expression) -> Optional[List[int]]:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jul 4, 2019

Collaborator

Please document this method. It is not new, but now it is sufficiently non-trivial to require a docstring with examples. Also make it non-private, essentially this is the same as try_getting_str_literals() but for integers instead of strings.


return left_type.slice(begin, stride, end)
items = [] # type: List[Type]
for b, e, s in itertools.product(begin, end, stride):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jul 4, 2019

Collaborator

The use of product would be clearer with a minimal example, like if there are two unions each with two elements (and stride is the same), the results would be a union with four elements.

key_names.append(key_type.value)
else:
self.msg.typeddict_key_must_be_string_literal(td_type, index)
return AnyType(TypeOfAny.from_error)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jul 4, 2019

Collaborator

Would it be practical to refactor this, so that this uses try_getting_str_literals()? It looks like there is some code duplication. (If yes, the function should be probably moved to nodes.py and imported both here and in the plugin.)

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Jul 9, 2019

Author Collaborator

Unfortunately, no -- this segment of code permits unicode keys but the try_getting_str_literals section doesn't.

IMO the correct thing to do is (a) modify try_getting_str_literals so it also allows unicode keys and (b) convert this entire function into a plugin.

But I think making this change fully correct is partly blocked on #6123, so I decided to defer the refactor for now.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

@Michael0x2a The release branch will be cut tomorrow morning (SF time). Is this PR ready?

@Michael0x2a Michael0x2a merged commit e7ddba1 into python:master Jul 9, 2019

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-union-math-for-intelligent-indexing branch Jul 9, 2019

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 2019

@ilevkivskyi -- sorry, it took me a bit to work out some quirks with my setup so I could start testing this against our internal codebases.

But I think (?) I've confirmed this PR introduces no new errors, so I went ahead and merged it in.

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.