More principled approach for callable vs callable inference #15910

ilevkivskyi · 2023-08-18T22:45:52Z

Fixes #702 (one of the oldest open issues)

The approach is quite simple, I essentially replicate the logic from subtyping check, while replacing each is_subtype() call with infer_constraints() call. Note that we don't have various options available in constraints.py so I use all checks, even those that may be skipped with some strictness flags (so we can infer as many constraints as possible). Depending on the output of mypy_primer we can try to tune this.

Note that while I was looking at subtyping code, I noticed couple inconsistencies for ParamSpecs, I added TODOs for them (and updated some existing TODOs). I also deleted some code that should be dead code after my previous cleanup.

Among inconsistencies most notably, subtyping between Parameters uses wrong (opposite) direction. Normally, Parameters entity behaves covariantly (w.r.t. types of individual arguments) as a single big argument, like a tuple plus a map. But then this entity appears in a contravariant position in Callable. This is how we handle it in constraints.py, join.py, meet.py etc. I tried to fix the left/right order in visit_parameters(), but then one test failed (and btw same test would also fail if I would try to fix variance in visit_instance()). I decided to leave this for separate PR(s).

cc @A5rocks

A5rocks

Mostly questions!

A5rocks · 2023-08-19T00:36:43Z

mypy/constraints.py

+    This function essentially extracts four steps from are_parameters_compatible() in
+    subtypes.py that involve subtype checks between argument types. We keep the argument
+    matching logic, but ignore various strictness flags present there, and checks that
+    do not involve subtyping. Then in place of every subtype check we put an infer_constrains()


A5rocks · 2023-08-19T00:38:07Z

mypy/constraints.py

+    # Numbering of steps below matches the one in are_parameters_compatible() for convenience.
+    # Phase 1a: compare star vs star arguments.
+    if left_star is not None and right_star is not None:
+        res.extend(infer_directed_constraints(left_star.typ, right_star.typ, direction))


I think the TODO in infer_directed_constraints only applies here? (Unless TypeVarMapping or whatever gets added)

Probably yes, but TBH I didn't think much about this.

A5rocks · 2023-08-19T00:41:59Z

mypy/constraints.py

+    """Infer constraints between two arguments using direction between original callables."""
+    if isinstance(left, (ParamSpecType, UnpackType)) or isinstance(
+        right, (ParamSpecType, UnpackType)
+    ):


I'm not sure if this extra logic is necessary normally: the only place these can pop up is *args and **kwargs. Err well, maybe not given current lax validation 😅.

Also I think the function name should make incorporate "argument" somehow...?

FWIW some tests failed without this, and we can never do anything useful anyway, so we can just always skip.

A5rocks · 2023-08-19T00:43:48Z

mypy/subtypes.py

-        if isinstance(self.right, (Parameters, CallableType)):
-            right = self.right
-            if isinstance(right, CallableType):
-                right = right.with_unpacked_kwargs()


I can't tell why you removed support for callable types here. I guess that might be handled elsewhere?

We should normally never compare Callable vs Parameters, and if we do they can never be subtype of each other, in the same sense as int is never a sub- (or super-) type of (int) -> int.

A5rocks · 2023-08-19T00:46:55Z

mypy/types.py

@@ -1602,7 +1599,7 @@ def copy_modified(
            variables=variables if variables is not _dummy else self.variables,
        )

-    # the following are copied from CallableType. Is there a way to decrease code duplication?
+    # TODO: here is a lot of code duplication with Callable type, fix this.


I guess I should write this historical note here in case it helps or something: I originally wanted to make every Callable have a Parameters attribute, meaning it doesn't hold its own args anymore. That seemed like too much work though! ... maybe these can inherit instead?

There are three possible ways to fix this:

Delete Parameters and use CallableType instead. (But I don't like this way, because it would be a hack.)

Make arg_types etc. properties on CallableType that forward values from its parameters attribute. This way I guess it will be much less work, but may have visible performance impact.

Indeed, both can inherit these methods from say ArgumentHelpers mixin.

Ideally I think we should try second option, measure performance, if it is OK, go with it, if it is bad, try option 3.

ilevkivskyi · 2023-08-19T10:50:13Z

Looking at mypy_primer output, steam.py error looks correct, but Expression one looks like a bug, and most likely preexisting one, since argument counts in overloads do not match o_O.

ilevkivskyi · 2023-08-19T10:54:19Z

Yep, a simple repro not involving type vars:

@overload
def d(x: int) -> int: ...
@overload
def d(f: int, *, x: int) -> str: ...
def d(x): ...

Let's see if there is an easy fix to avoid regressions.

JukkaL

Great to have this finally working as expected! It's interesting how long we managed without proper types inference for callables. Looks good, just one comment about possible additional test cases.

JukkaL · 2023-08-21T17:58:23Z

test-data/unit/check-inference.test

+# Note: order of names is different w.r.t. protocol
+def g(*, y: int, x: str) -> None: pass
+reveal_type(f(g))  # N: Revealed type is "Tuple[builtins.str, builtins.int]"
+[builtins fixtures/list.pyi]


Also test a few cases where we shouldn't be inferring constraints, e.g. positional-only vs keyword-only?

OK, added such tests (note btw if there are no constraints, it now means they can never by subtypes, so there will be also an error in each test).

github-actions · 2023-08-21T21:54:22Z

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/commands/commands.py:909: error: Overloaded function implementation cannot satisfy signature 3 due to inconsistencies in how they use type variables  [misc]

More principled approach for callable vs callable inference

13ff43d

ilevkivskyi requested review from jhance, JukkaL and hauntsaninja August 18, 2023 22:45

This comment has been minimized.

Sign in to view

A5rocks approved these changes Aug 19, 2023

View reviewed changes

ilevkivskyi added 4 commits August 19, 2023 12:17

Address CR

3898f79

Fix partial overlap in overlapping overload checks

4fe5688

More realistic test case

1f3af72

Update a TODO

1dadd7d

This comment has been minimized.

Sign in to view

JukkaL approved these changes Aug 21, 2023

View reviewed changes

Add more tests

cb35b17

ilevkivskyi merged commit 7141d6b into python:master Aug 21, 2023
17 checks passed

ilevkivskyi deleted the fix-callable-inference branch August 21, 2023 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More principled approach for callable vs callable inference #15910

More principled approach for callable vs callable inference #15910

ilevkivskyi commented Aug 18, 2023

This comment has been minimized.

A5rocks left a comment

A5rocks Aug 19, 2023

A5rocks Aug 19, 2023

ilevkivskyi Aug 19, 2023

A5rocks Aug 19, 2023

ilevkivskyi Aug 19, 2023

A5rocks Aug 19, 2023

ilevkivskyi Aug 19, 2023

A5rocks Aug 19, 2023

ilevkivskyi Aug 19, 2023

ilevkivskyi commented Aug 19, 2023

ilevkivskyi commented Aug 19, 2023

This comment has been minimized.

JukkaL left a comment

JukkaL Aug 21, 2023

ilevkivskyi Aug 21, 2023

github-actions bot commented Aug 21, 2023

More principled approach for callable vs callable inference #15910

More principled approach for callable vs callable inference #15910

Conversation

ilevkivskyi commented Aug 18, 2023

This comment has been minimized.

A5rocks 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

ilevkivskyi commented Aug 19, 2023

ilevkivskyi commented Aug 19, 2023

This comment has been minimized.

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2023