[OLD] Add more robust support for detecting partially overlapping types #5280

Michael0x2a · 2018-06-26T06:18:52Z

This pull request adds more robust support for detecting partially overlapping types. Specifically, it detects overlaps with...

TypedDicts
Tuples
Unions
Typevars
Generic types containing variations of the above

This pull request adds more robust support for detecting partially overlapping types. Specifically, it detects overlaps with... 1. TypedDicts 2. Tuples 3. Unions 4. Typevars 5. Generic types containing variations of the above

Michael0x2a · 2018-06-26T06:42:17Z

Some additional notes:

I feel like this PR is almost certainly going to break something -- I would most definitely test this on an internal codebase.
There was one edge case I couldn't figure out. Basically, with this PR, mypy will correctly (?) treat the following overload as being safe:
```
T = TypeVar('T')

@overload
def f(x: int) -> int: ...
@overload
def f(x: T) -> T: ...
def f(x): ...
```
However, the following is incorrectly (?) treated as being unsafely partially overlapping:
```
class Wrapper(Generic[T]):
    @overload
    def g(self, x: int) -> int: ...
    @overload
    def g(self, x: T) -> T: ...
    def g(self, x): ...
```
The root issue appears to be that unify_generic_callable was able to correctly infer that T could be of type int when we check f's second signature against the first.

The function, however, was not able to do the same when checking g's second signature against the first: 'self' is implicitly of type Wrapper[T], and we end up inferring an additional type constraint T <: T which, for some reason, makes it so that we never infer T = int.

I'm not really sure how to get around this issue: I tried a bunch of hacks/workarounds (e.g. filtering out the T <: T constraint), and none of them seemed to quite handle all the edge cases.

ilevkivskyi

Thanks! I have some minor comments, and few larger comments:

It seems to me there are too many functions: is_overlapping, is_partially_overlapping, and is_overlapping_erased. So the question do we need the one that checks for partial overlap? It seems to me the logic is as following: we first check for unsafe overlap because first arg types are subtypes of second arg types, then we check for "will never match" (the opposite), at the third step we can only get partial overlaps. So it is fine to have a simpler function that doesn't care whether overlap is partial or full. I think this can allow to simplify the code and also we will have only two functions: normal overlap and erased overlap.
You renamed existing function and added a new function with old name, I think it would be safer to keep the name of existing function, and choose a new name for the new function (is_overlapping_nonerased?).
It seems to me the erased one is not so erased, in the sense it doesn't match the comment that says it corresponds to runtime behavior (e.g. because of tuples). But it is probably not for this PR.

ilevkivskyi · 2018-07-05T18:15:42Z

mypy/checker.py

@@ -3630,37 +3633,119 @@ def are_argument_counts_overlapping(t: CallableType, s: CallableType) -> bool:

 def is_unsafe_overlapping_overload_signatures(signature: CallableType,
                                              other: CallableType) -> bool:
-    """Check if two overloaded function signatures may be unsafely overlapping.
+    """Check if two overloaded signatures are unsafely overlapping, ignoring partial overlaps.

    We consider two functions 's' and 't' to be unsafely overlapping both if


"both if" -> "if both"

ilevkivskyi · 2018-07-05T18:21:34Z

mypy/checker.py

+                                                        other: CallableType) -> bool:
+    """Check if two overloaded signatures are unsafely overlapping, ignoring partial overlaps.
+
+    We consider two functions 's' and 't' to be unsafely overlapping both if


"both if" -> "if both" here too.

ilevkivskyi · 2018-07-05T18:27:00Z

mypy/checker.py

+    return typ.copy_modified(variables=new_variables)
+
+
+class TypeVarExtractor(TypeQuery[Set[TypeVarType]]):


We already have TypeVariableQuery in typeanal.py, so I am thinking maybe we can combine these two (maybe there are other). Or at least move these visitors to a separate utility module.

ilevkivskyi · 2018-07-05T18:28:37Z

mypy/meet.py

@@ -49,41 +53,175 @@ def narrow_declared_type(declared: Type, narrowed: Type) -> Type:
    return narrowed


-def is_partially_overlapping_types(t: Type, s: Type) -> bool:
-    """Returns 'true' if the two types are partially, but not completely, overlapping.
+def get_possible_variants(typ: Type) -> List[Type]:


I think this function deserves a docstring (in particular it is not clear at this point why we need both typ.items and typ).

ilevkivskyi · 2018-07-05T18:30:12Z

mypy/meet.py

+        return typ.items + [typ]
+    elif isinstance(typ, Overloaded):
+        # Note: doing 'return typ.items() + [typ]' makes mypy
+        # infer a too-specific return type of List[CallableType]


I think it is a bug actually, mypy shuould have used the return context List[Type] to correctly infer the type (or maybe the signature of list.__add__ is not good enough?) Could you please open an issue with a simple repro?

ilevkivskyi · 2018-07-05T18:32:29Z

mypy/meet.py

-        return isinstance(t, Instance) and t.type.fullname() == 'builtins.object'
+def is_partially_overlapping(left: Type, right: Type) -> bool:
+    # We should never encounter these types
+    illegal_types = (UnboundType, PartialType, ErasedType, DeletedType)


UnboundType should be definitely not in this list. It is known that semanal.py "leaks" some of those. Also I am not very keen on adding this assert. Maybe we can do this later when we are more sure about this code.

ilevkivskyi · 2018-07-05T18:43:31Z

mypy/subtypes.py

@@ -920,6 +920,7 @@ def unify_generic_callable(type: CallableType, target: CallableType,
        c = mypy.constraints.infer_constraints(
            type.ret_type, target.ret_type, return_constraint_direction)
        constraints.extend(c)
+


Stray whitespace?

ilevkivskyi · 2018-07-05T20:00:55Z

test-data/unit/check-overloading.test

+# Same root issue: why does the additional constraint bound T <: T
+# cause the constraint solver to not infer T = object like it did in the
+# first example?
+@overload


I think the root cause here is invariance, mypy adds two constrains from Dummy: T' <: T and T <: T' (see code in ConstraintBuilder.visit_instance), at least this is the only difference I see between two functions.

Note, I use prime to distinguish between two variables with the same name, assuming this comment in subtypes.py:

# ...because generating and solving # constraints for the variables of L to make L a subtype of R # (below) treats type variables on the two sides as independent.

@ilevkivskyi -- before I continue working on these TODOs, I just want to check in with you and see if this is something you think we should actually fix.

For context, here's a sample program -- we get the same output whether we're using mypy master or this PR:

from typing import * T = TypeVar('T') # Example 1: No error reported @overload def f1(x: int) -> int: ... @overload def f1(x: T) -> T: ... def f1(*args, **kwargs): pass # Example 2: Overload is reported as being unsafe class Wrapper(Generic[T]): @overload def f3(self, x: int) -> int: ... @overload def f3(self, x: T) -> T: ... def f3(self, *args, **kwargs): pass # Example 3: Overload is reported as being unsafe class Dummy(Generic[T]): pass @overload def f2(d: Dummy[T], x: int) -> int: ... @overload def f2(d: Dummy[T], x: T) -> T: ... def f2(*args, **kwargs): pass

Basically, the issue is that example 1 typechecks (as expected) but example 2 doesn't, which seems like surprising and unintuitive behavior.

I think the root issue is that mypy internally represents example 2 in a way that's basically indistinguishable from example 3. Mypy then tries and fails to unify together the two generics due to the invariance issue you pointed out.

So, before I spend more time on this, I wanted to check in and ask a few questions:

Do you think example 3 is actually unsafely overlapping? I'm having some difficultly determining whether or not mypy is actually correct here.

(One thing to note is that the behavior of example 3 is technically undefined since the first overload is using just a single typevar. That said, modifying the signature to fix that doesn't change the error message.)

Do you think it's worth trying to modify mypy so it doesn't report an error on example 2?

Here are my short opinions about two questions:

I think the third example is fine (as well as other two).

I think it is a bug, i.e. mypy should not report an error there, but it is probably a low-priority one. So I propose to just go ahead with whatever is easier and then we can try fixing this if someone will complain.

This commit is a WIP which I am pushing now for testing purposes. I will rebase/push another commit with more polishing/more exposition a bit later.

Michael0x2a · 2018-07-06T08:42:28Z

So, progress report:

I think the reason why I went with a function for checking specifically partial overlaps was because it initially seemed to me as if it would require less code/be more flexible to use. I then never ended up revisiting that decision as my implementation evolved.

But yeah, I think I agree: we can get rid of is_partially_overlapping/eventually combine all three methods together.
That said, I think this definitely isn't going to make the release cutoff -- the PR/related changes probably needs another day or two of work on my end, and I won't have time until the weekend. (In particular, running both PRs on our internal repos generated a handful of errors I need to investigate, I haven't had a chance to take another look at the constraint generation algorithms yet, it looks like I probably need to land a patch to make operators slightly more sane first and/or make more changes to typeshed...)

Sorry about that :(

ilevkivskyi · 2018-07-06T14:38:16Z

That said, I think this definitely isn't going to make the release cutoff

I think this is fine, the next release is just 3-4 weeks away.

Sorry about that :(

No problems at all! Thanks for all the work on overloads, this is already a big improvement.

Fixes python#5351

This is mostly so we can get python/typeshed#2338, which unblocks python#5370.

An unintended side-effect of silencing errors in site packages and typeshed by default is that our tests no longer flag changes to mypy that end up breaking typeshed in some way. (For example, see https://github.com/python/mypy/pulls/5280 which *really* should not be passing, at least as of time of writing.) This pull request will add the `--no-silence-site-packages` flag to any test suites that directly or indirectly test typeshed. Specifically, I believe this PR ought to add the flag to any tests triggered by `testsamples.py`, `testselfcheck.py`, and `testpythoneval.py`.

ilevkivskyi · 2018-07-21T16:34:31Z

This PR gets a bit too large. Is it possible to split it in parts. Also I am not sure I understand how some changes are necessary (in particular all the changes to subtypes visitors). Is it possible to split this PR in parts (especially if it fixes unrelated bugs triggered by the main change)?

Unfortunately it looks like I will not have time to review this before my vacation, but I will do this first thing I am back.

Michael0x2a · 2018-07-21T17:05:44Z

@ilevkivskyi -- I can work on splitting up these changes, yeah. I agree that this is starting to become a bit large.

That said, I don't think this is 100% ready to review yet. It looks like I need to fiddle around with operator overloads first in order to make these changes pass the tests, so I plan on submitting a separate PR for that first.

Anyways, enjoy your vacation -- you definitely deserve one! And thank you for putting in the time to review/help me land these changes!

ilevkivskyi · 2018-07-21T17:12:29Z

so I plan on submitting a separate PR for that first.

Great idea!

Anyways, enjoy your vacation -- you definitely deserve one! And thank you for putting in the time to review/help me land these changes!

Thanks! :-)

(Although you did most of the hard work)

ilevkivskyi · 2018-08-14T19:40:29Z

Closing in favor of #5476

ilevkivskyi self-requested a review July 3, 2018 10:50

ilevkivskyi self-assigned this Jul 3, 2018

Michael0x2a mentioned this pull request Jul 4, 2018

Release 0.620 planning #5314

Closed

ilevkivskyi reviewed Jul 5, 2018

View reviewed changes

Michael0x2a added 2 commits July 5, 2018 21:46

Merge branch 'master' into overloads-partially-overlapping-types

d91b0d6

WIP commit

1f17aff

This commit is a WIP which I am pushing now for testing purposes. I will rebase/push another commit with more polishing/more exposition a bit later.

ilevkivskyi mentioned this pull request Jul 6, 2018

Allow overloading based on TypedDict structure #3618

Closed

Michael0x2a and others added 13 commits July 17, 2018 12:39

Merge branch 'master' into overloads-partially-overlapping-types

41cda3e

Refactor and add some comments

4cdeab7

Add distutils and encodings as stdlib modules (python#5372)

813ddf7

Fixes python#5351

Sync typeshed (python#5373)

9bb0477

This is mostly so we can get python/typeshed#2338, which unblocks python#5370.

WIP: Try switching to using relevant_items()

ee86385

WIP: try modifying interaction with overloads, classes, and invariance

4cad672

WIP: Allow overlapping type checks to disable promotions

b554023

Merge branch 'master' into overloads-partially-overlapping-types

4059f60

WIP: Make checks less sensitive to 'Any' weirdness

18305f2

WIP: Refine and clean up overlapping meets logic

bb6fbc5

Add unit tests to cover a failure fixed by previous commit

55a01cf

Merge branch 'master' into overloads-partially-overlapping-types

022e75b

Michael0x2a added 3 commits August 4, 2018 17:14

Merge branch 'master' into overloads-partially-overlapping-types

d559f43

WIP: refactor operator code

7e2eb9b

Fix typo in test

f098191

Michael0x2a added 10 commits August 8, 2018 15:18

Merge branch 'master' into overloads-partially-overlapping-types

b57c39a

WIP: Try adding more precise support for reversable operators

539ed6a

WIP: operators refinement (incomplete)

b5e42d3

Merge branch 'master' into overloads-partially-overlapping-types

7eb85dd

WIP: Modify Any fallback from reverse operators

e12fa0b

Merge branch 'master' into overloads-partially-overlapping-types

6702982

WIP: Fix last failing tests?

e564df3

Add missing return

4e3c8b8

WIP: Misc cleanup

71437c4

Misc cleanup

99d48b7

Michael0x2a changed the title ~~Add more robust support for detecting partially overlapping types~~ [OLD] Add more robust support for detecting partially overlapping types Aug 14, 2018

Michael0x2a mentioned this pull request Aug 14, 2018

Add partial overload checks #5476

Merged

ilevkivskyi closed this Aug 14, 2018

Michael0x2a mentioned this pull request Aug 20, 2018

Moving an overloaded generic function into a class can sometimes introduce "incompatible return type" errors #5510

Closed

Michael0x2a deleted the overloads-partially-overlapping-types branch August 27, 2018 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OLD] Add more robust support for detecting partially overlapping types #5280

[OLD] Add more robust support for detecting partially overlapping types #5280

Michael0x2a commented Jun 26, 2018

Michael0x2a commented Jun 26, 2018

ilevkivskyi left a comment

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

ilevkivskyi Jul 5, 2018

Michael0x2a Jul 18, 2018

ilevkivskyi Aug 2, 2018

Michael0x2a commented Jul 6, 2018 •

edited

Loading

ilevkivskyi commented Jul 6, 2018

ilevkivskyi commented Jul 21, 2018

Michael0x2a commented Jul 21, 2018

ilevkivskyi commented Jul 21, 2018

ilevkivskyi commented Aug 14, 2018

		return typ.copy_modified(variables=new_variables)


		class TypeVarExtractor(TypeQuery[Set[TypeVarType]]):

[OLD] Add more robust support for detecting partially overlapping types #5280

[OLD] Add more robust support for detecting partially overlapping types #5280

Conversation

Michael0x2a commented Jun 26, 2018

Michael0x2a commented Jun 26, 2018

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael0x2a commented Jul 6, 2018 • edited Loading

ilevkivskyi commented Jul 6, 2018

ilevkivskyi commented Jul 21, 2018

Michael0x2a commented Jul 21, 2018

ilevkivskyi commented Jul 21, 2018

ilevkivskyi commented Aug 14, 2018

Michael0x2a commented Jul 6, 2018 •

edited

Loading