Function compatibility rewrite #2521

sixolet · 2016-12-02T02:41:39Z

Before, the logic for determining whether a function was an Any-permissive subtype of another function compared their argument types in order, with some special provision for *args. It did not handle keyword-only arguments, or **kwargs, or argument names being different, or any other such situation.

This PR introduces an algorithm for is_callable_subtype that pairs up corresponding arguments by both name and position, and makes sure the argument in the ostensible subtype is "more general". It also deals with what happens when an arg in one function corresponds to different args in the other function when you try to pair it up by name vs. when you try to pair it up by argument position.

Note that specifically for the case of determining whether a function is a valid override, we ignore the names of positional arguments; too many people regularly pay no attention to the names of positional arguments of functions in subtypes in code they write, even if it's not a technically perfect python subtype. We do error in the case the names of positional arguments are transposed (ex. argument foo is the first argument in one function, but the second argument in the other), as this is likely to be an actual mistake.

We pay attention to the names of positional arguments for assignment statements and argument accepting.

sixolet · 2016-12-02T02:51:04Z

I am aware that typechecking typeshed fails. In the last week, we added the werkzeug library to typeshed, which is not clean according to the new rules.

python/typeshed@021b162
python/typeshed#530

There are two main sources of the typechecking problems:

The collection datatypes in werkzeug need to be parameterized for the mixins to work out right (I think)
There's one mixin/base type pair where the base type has a function freeze(**kwargs) and the mixin has a function freeze(arg_name=...). These are incompatible; freeze(**kwargs) would be a subtype of freeze(*, arg_name=...), but as it is they're incomparable.

Sidenote: this code stops complaining about freeze when I make the mixin one keyword-only. I find this counterintuitive, and I'm curious whether there's a bug in how the MRO interacts with the overriding rules for functions.

EDIT: aha, it looks like the method it will find in the MRO will be the one in the base class and not the mixin. The one in the base class then is a subtype of the one in the mixin, so the subtyping rules stop complaining; it's a valid instance of base and an valid instance of mixin. Ok, but I don't think that's what the writer intended -- maybe the writer intended methods to get resolved from the mixin before the base?

sixolet · 2016-12-02T02:59:16Z

There is future work that builds on this change that expands the FormalArgument abstraction into being how the formal arguments of CallableType are represented internally, and making "argument kinds" either obsolete or only relevant to actual parameters at callsites (and removing the need for optional ones at all)

JukkaL · 2016-12-02T10:34:28Z

Thanks for the PR! This will take some careful study to review.

Switching the signature of freeze(...) to have the argument keyword-only seems like a reasonable fix. Can you create a typeshed PR for this?

Are the typeshed changes related?

sixolet · 2016-12-03T02:11:32Z

This change itself shouldn't update typeshed.

JukkaL

Thanks for implementing this -- this fixes a bunch of problematic edge cases and paves the way for more expressive callable type annotations. I wrote some minor comments. I'll need to reason through various corner cases still.

JukkaL · 2016-12-08T23:50:24Z

test-data/unit/check-functions.test

+ee_var = everything
+ee_var = everywhere
+
+ee_var = specific_1 # The difference between Callable[[...], blah] and one with a *args: Any, **kwargs: Any is that the ... goes loosely both ways.


Extra [] around ... in comment: should be Callable[..., blah].

JukkaL · 2016-12-08T23:52:00Z

test-data/unit/check-functions.test

+ee_var = everywhere
+
+ee_var = specific_1 # The difference between Callable[[...], blah] and one with a *args: Any, **kwargs: Any is that the ... goes loosely both ways.
+ee_def = specific_1 # E: Incompatible types in assignment (expression has type Callable[[int, str], None], variable has type Callable[[Any, Any], None])


The error message is confusing (missing indication of * and ** in the variable).

JukkaL · 2016-12-09T00:15:29Z

mypy/types.py

@@ -542,6 +543,13 @@ def deserialize(cls, data: JsonDict) -> 'FunctionLike':
 _dummy = object()  # type: Any


+FormalArgument = NamedTuple('FormalArgument', [
+    ('name', str),


Use Optional[...] for types of items that can be None (for documentation, these aren't checked yet).

JukkaL · 2016-12-09T00:32:50Z

mypy/subtypes.py

+    return True
+
+
+def is_left_more_general(


I wonder if this could have a more descriptive name, since this also checks things that arguably aren't about being more general, such as position. Random ideas: are_args_compatible, is_left_corresponding_and_more_general

JukkaL · 2016-12-09T00:36:47Z

mypy/subtypes.py

+    # used for in practice.
+
+    # Every argument in R must have a corresponding argument in L, and every
+    # required argument in L must have a corresponding arugment in R.


Typo: arugment

The typo arugment is still there :)

JukkaL · 2016-12-09T00:38:20Z

mypy/subtypes.py

+            # sure left has its own infinite set of optional named arguments.
+            if not left.is_kw_arg:
+                return False
+            left_names = set([name for name in left.arg_names if name is not None])


Reminder: Use set comprehensions.

JukkaL · 2016-12-09T00:59:48Z

mypy/messages.py

+                                constructor,
+                                arg_name,
+                                strip_quotes(self.format(arg_type)),
+                                arg_kind in (ARG_NAMED, ARG_NAMED_OPT)))


Could we have this default to False and omit the third argument if it's False? Currently the representations of arguments feel overly long to me, and this would help a bit.

JukkaL · 2016-12-09T01:03:35Z

And I'd like to add that some of the edge cases that this PR fixes are super subtle and required a lot of sweating the details.

JukkaL · 2016-12-09T02:09:08Z

This doesn't look right:

def l(x): ...
def r(__x): ...
r = l   # error

If I add a type annotation to l and r there's no error. It looks like positional-only arguments aren't inferred for functions with implicit Any types.

This doesn't generate an error, though it doesn't look okay:

def l(x) -> None: ...
def r(__, *, x) -> None: ...
r = l   # no error, though l isn't compatible

I think that we need to make sure that each left argument only maps to a single required right argument.

JukkaL · 2016-12-20T17:24:28Z

Can you fix the merge conflict? I'll try to review the latest changes later this week.

The new algorithm correctly handles argument names. It pays attention to them when matching up arguments for function assignments, but not for overrides (because too many libraries vary the agument names between superclass and subclass). I'll include a full writeup in prose of the new algorithm in the pull request. Print callable types with named arguments differently from each other when important Fix importFunctionAndAssignFunction test to match var names

sixolet · 2016-12-21T22:50:13Z

Ok. I rebased, and also made a small refactor -- I'm not totally sure how I feel about it, because it makes the code more readable (and shorter!), but also introduces a slight exacerbation of a current circular dependency. I do thing the concept of a corresponding_argument is something other code will want eventually, though.

JukkaL

I ran this against Dropbox internal repos and it found at least one thing that looked like a bug -- nice!

A few things are still causing trouble:

Mypy now complains about typeshed/stdlib/2/posixpath.pyi -- the stub may need to be tweaked a bit.
Maybe we should not complain about incompatible names for positional arguments if one of the callables is dynamically typed (all arguments and return value have Any types). Mypy tries to not give many false positives for legacy unnanotated code, and here we now generate more errors for things which might be innocent, such as having an argument that is implicitly considered positional-only (but without the dunder prefix, since that's a recent thing) so the name isn't important. I saw conditional function definitions that are now are generating errors.

JukkaL · 2016-12-22T13:23:55Z

mypy/subtypes.py

+    # used for in practice.
+
+    # Every argument in R must have a corresponding argument in L, and every
+    # required argument in L must have a corresponding arugment in R.


The typo arugment is still there :)

…e now

JukkaL · 2016-12-22T18:39:37Z

Looks good now -- thanks for powering through!

ambv · 2016-12-22T19:36:58Z

✨ 🍰 ✨

However, FYI, this broke the typeshed build since it uncovered some stub problems. Fixing those now:

$ tests/mypy_test.py -p2
running mypy --python-version 2.7 --strict-optional # with 524 files
third_party/2/werkzeug/wrappers.pyi:194: error: Definition of "freeze" in base class "BaseResponse" is incompatible with definition in base class "ETagResponseMixin"
third_party/2/werkzeug/contrib/testtools.pyi:13: error: Definition of "freeze" in base class "BaseResponse" is incompatible with definition in base class "ETagResponseMixin"
third_party/2/werkzeug/datastructures.pyi:176: error: Definition of "__delitem__" in base class "ImmutableHeadersMixin" is incompatible with definition in base class "Headers"
third_party/2/itsdangerous.pyi:149: error: Definition of "load_payload" in base class "URLSafeSerializerMixin" is incompatible with definition in base class "Serializer"
third_party/2/itsdangerous.pyi:152: error: Definition of "load_payload" in base class "URLSafeSerializerMixin" is incompatible with definition in base class "Serializer"
third_party/2and3/boto/connection.pyi:114: error: Signature of "make_request" incompatible with supertype "AWSAuthConnection"
third_party/2and3/boto/s3/connection.pyi:71: error: Signature of "make_request" incompatible with supertype "AWSAuthConnection"

sixolet · 2016-12-22T20:55:02Z

Hmm! I'm sorry! For my future edification, does the typeshed build include more stringent checking than the checks of typeshed in the mypy build? I'd merged a few changes to typeshed to get that passing, but I'd like to not miss stuff in the future.

…

On Dec 22, 2016, at 1:37 PM, Łukasz Langa ***@***.***> wrote: ✨ 🍰 ✨ However, FYI, this broke the typeshed build since it uncovered some stub problems. Fixing those now: $ tests/mypy_test.py -p2 running mypy --python-version 2.7 --strict-optional # with 524 files third_party/2/werkzeug/wrappers.pyi:194: error: Definition of "freeze" in base class "BaseResponse" is incompatible with definition in base class "ETagResponseMixin" third_party/2/werkzeug/contrib/testtools.pyi:13: error: Definition of "freeze" in base class "BaseResponse" is incompatible with definition in base class "ETagResponseMixin" third_party/2/werkzeug/datastructures.pyi:176: error: Definition of "__delitem__" in base class "ImmutableHeadersMixin" is incompatible with definition in base class "Headers" third_party/2/itsdangerous.pyi:149: error: Definition of "load_payload" in base class "URLSafeSerializerMixin" is incompatible with definition in base class "Serializer" third_party/2/itsdangerous.pyi:152: error: Definition of "load_payload" in base class "URLSafeSerializerMixin" is incompatible with definition in base class "Serializer" third_party/2and3/boto/connection.pyi:114: error: Signature of "make_request" incompatible with supertype "AWSAuthConnection" third_party/2and3/boto/s3/connection.pyi:71: error: Signature of "make_request" incompatible with supertype "AWSAuthConnection" — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

Starting with python/mypy#2521 mypy is performing stricter function signature checks. This makes the stubs diverge from the actual implementation but makes the stubs internally consistent. Since this is an actual typing issue in the base implementation, we need to defer to the original authors to fix it.

ambv · 2016-12-22T23:53:39Z

@sixolet, typeshed runs Mypy over all stubs which the Mypy test runner currently doesn't do. Maybe it should? In the mean time, re-running typeshed tests is your best bet to be sure nothing breaks.

As you can see above, I reported bugs and put workarounds in typeshed for everything except for https://github.com/boto/boto. In this case, their implementation is really seriously breaking the Liskov Substitution Principle. Consider:

class AWSAuthConnection(object):
  def make_request(self, method, path, headers=None, data='', host=None,
    auth_path=None, sender=None, override_num_retries=None,
    params=None, retry_handler=None): ...

class AWSQueryConnection(AWSAuthConnection):
  def make_request(self, action, params=None, path='/', verb='GET'): ...

I'm skeptical if we can expect upstream to fix their design issue in this case, because this is pretty ancient code that's still in major use. This API is just broken and the stub cannot really do anything about it. We need a way to let Mypy know to ignore this particular problem in stubs. In the mean time, I excluded third_party/2and3/boto/.* from typeshed tests.

Starting with python/mypy#2521 mypy is performing stricter function signature checks. This makes the stubs diverge from the actual implementation but makes the stubs internally consistent. Since this is an actual typing issue in the base implementation, we need to defer to the original authors to fix it. Sadly, in this case the breakage is rather fundamental and unlikely to get fixed by upstream. Consider: ``` class AWSAuthConnection(object): def make_request(self, method, path, headers=None, data='', host=None, auth_path=None, sender=None, override_num_retries=None, params=None, retry_handler=None): ... class AWSQueryConnection(AWSAuthConnection): def make_request(self, action, params=None, path='/', verb='GET'): ... ``` Hence, until we have a workaround for the error produced by Mypy, we're excluding those stubs from being tested against.

JukkaL reviewed Dec 9, 2016

View reviewed changes

sixolet force-pushed the function-compatibility branch from bc2f11a to 16cf426 Compare December 10, 2016 01:59

sixolet added 11 commits December 21, 2016 13:57

Rename argument shorter

8b3b788

Explain what i am doing better

5e68ee3

Stop checking argument names for multiple base classes

8bf9c53

Elide __foo argument names from funcs where args are not typed

1e4d467

Cosmetic

c42bd86

Make the text descriptions of args more concise

c8d4458

Account for one left argument with two corresponding right args

d7df8cd

cosmetic

d811a82

cosmetic

95d1c58

Remove In class foo

d752499

sixolet force-pushed the function-compatibility branch from 16cf426 to d752499 Compare December 21, 2016 22:12

Factor out corresponding_argument method

499e1d2

Optional for the name and pos

7617eac

JukkaL reviewed Dec 22, 2016

View reviewed changes

sixolet added 5 commits December 22, 2016 07:38

typo

77c8f73

Ignore implicitly typed function arg names

62fd868

Test for ignoring arg names for implicit defs

5c6d574

Fix tests

4397926

Delete tests for implicit callable lacking names as they make no sens…

2a07da8

…e now

JukkaL merged commit 507c3c2 into python:master Dec 22, 2016

ambv mentioned this pull request Dec 22, 2016

Move boto to 2and3, add stubs for s3 python/typeshed#787

Merged

ambv mentioned this pull request Dec 22, 2016

Serializer.load_payload overrides in subclasses have incompatible signatures pallets/itsdangerous#74

Closed

ambv mentioned this pull request Dec 22, 2016

Headers.__delitem__ signature incompatible with ImmutableHeadersMixin.__delitem__ pallets/werkzeug#1051

Closed

ambv mentioned this pull request Dec 22, 2016

Wrong order of base classes in wrappers.Response pallets/werkzeug#1052

Closed

gvanrossum mentioned this pull request Dec 24, 2016

Sync typeshed #2606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function compatibility rewrite #2521

Function compatibility rewrite #2521

sixolet commented Dec 2, 2016

sixolet commented Dec 2, 2016 •

edited

Loading

sixolet commented Dec 2, 2016

JukkaL commented Dec 2, 2016

sixolet commented Dec 3, 2016

JukkaL left a comment

JukkaL Dec 8, 2016

JukkaL Dec 8, 2016

JukkaL Dec 9, 2016

JukkaL Dec 9, 2016

JukkaL Dec 9, 2016

JukkaL Dec 22, 2016 •

edited

Loading

JukkaL Dec 9, 2016

JukkaL Dec 9, 2016

JukkaL commented Dec 9, 2016

JukkaL commented Dec 9, 2016

JukkaL commented Dec 20, 2016

sixolet commented Dec 21, 2016

JukkaL left a comment

JukkaL Dec 22, 2016 •

edited

Loading

JukkaL commented Dec 22, 2016

ambv commented Dec 22, 2016

sixolet commented Dec 22, 2016 via email

ambv commented Dec 22, 2016

Function compatibility rewrite #2521

Function compatibility rewrite #2521

Conversation

sixolet commented Dec 2, 2016

sixolet commented Dec 2, 2016 • edited Loading

sixolet commented Dec 2, 2016

JukkaL commented Dec 2, 2016

sixolet commented Dec 3, 2016

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL Dec 8, 2016

Choose a reason for hiding this comment

JukkaL Dec 8, 2016

Choose a reason for hiding this comment

JukkaL Dec 9, 2016

Choose a reason for hiding this comment

JukkaL Dec 9, 2016

Choose a reason for hiding this comment

JukkaL Dec 9, 2016

Choose a reason for hiding this comment

JukkaL Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

JukkaL Dec 9, 2016

Choose a reason for hiding this comment

JukkaL Dec 9, 2016

Choose a reason for hiding this comment

JukkaL commented Dec 9, 2016

JukkaL commented Dec 9, 2016

JukkaL commented Dec 20, 2016

sixolet commented Dec 21, 2016

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

JukkaL commented Dec 22, 2016

ambv commented Dec 22, 2016

sixolet commented Dec 22, 2016 via email

ambv commented Dec 22, 2016

sixolet commented Dec 2, 2016 •

edited

Loading

JukkaL Dec 22, 2016 •

edited

Loading

JukkaL Dec 22, 2016 •

edited

Loading