Fix 3903: untyped defs on derived classes #7548

gantsevdenis · 2019-09-21T15:34:34Z

fix #3903
Looked simple enough, but not sure about the part:

if not defn.info:
           return None

If removed, produces AssertionErrors

ilevkivskyi

Thanks for working on this! I have few general comments:

I think this should be independent of --check-untyped-defs. One might want to have the latter not enables and only check the methods that have a typed definition in a superclass.
I think the benefit should be not only checking the bodies using "inherited" types, but also external access should be typed, i.e. Sub.meth(...) calls should be checked using the signature of Super.meth. For this reason I think the copying should happen earlier, during semantic analysis.
What should we do if the signature is impossible to copy? In particular what if signatures are not identical, but still Liskov-compatible?
What if the method in a subclass annotates only some arguments? Should this trigger an error, should we copy the rest, should we just assume the missing ones are Any?
You need to make your implementation works correctly in incremental and daemon modes (the latter one might require some tricky changes to type dependency tracking).
You need to be sure this works well with generic classes, where you wound need to not just copy the argument/return types, but map them correctly taking into account type variable binding along MRO.

ilevkivskyi · 2019-09-28T20:29:26Z

I think most of the questions are answered in #3903 (comment) (there are also couple additional points).

gantsevdenis · 2019-10-08T07:13:28Z

Thank you for guiding!
I have followings question points:

I added support for generics, but not sure if something exists already which could be used, instead of my own substitute.. function? I couldn't find out how to use some of existing TransformVisitors
I had to do a cast, because I couldn't solve typing problem..
I am not sure to understand the difference between incremental mode, and daemon mode, but I ve tried using feature with daemon, and could not encounter any dependency tracking issues (I used a very simple setup though, only 2 modules, both monitored by daemon, one import each other, one of them defines aclass Base, and the other imports it: the typing issues in the second module were caught ) Ok I see a problem indeed in incremental mode. Will work on it

ilevkivskyi

Although you said:

Looked simple enough

This is relatively tricky feature. I left few large-scale comments. This looks still not ready fo a detailed review.

ilevkivskyi · 2019-10-23T18:34:28Z

mypy/main.py

@@ -442,7 +442,9 @@ def add_invertible_flag(flag: str,
    add_invertible_flag('--disallow-untyped-decorators', default=False, strict_flag=True,
                        help="Disallow decorating typed functions with untyped decorators",
                        group=untyped_group)
-
+    add_invertible_flag('--inherit-signatures', default=False, strict_flag=True,


I don't think this needs to be a strict flag, because --disallow-untyped-defs is already part of --strict IIRC.

ilevkivskyi · 2019-10-23T19:10:11Z

mypy/semanal.py

@@ -160,6 +160,45 @@
 Tag = int


+def substitute_unbounds(t: UnboundType, names_to_types: Mapping[str, Instance]) -> ProperType:


Sorry for misleading suggestion, doing the copy during semantic analyzis is still bad. I was thinking about superclass being in a different module that is type-checked before the module with subclass, but it obviously doesn't work always.

Also this function is just one big hack. I was thinking about something along the lines of mypy.checker.TypeChecker.bind_and_map_method().

For the latter to work well I see a possible strategy:

Add two flags (or maybe better one three state flag) to functions indicating whether this function can be "improved", i.e. it has some untyped arguments, while a supertype has those typed, and whether the "copying" has already been performed.

The first flag should be set during semantic analysis, this should be thought carefully to work well in import cycles.

During type checking, whenever we encounter a reference to a method Sub.meth for which we know it can be "improved", but wasn't yet, we trigger a deferral.

Finally, we perform the copying while analyzing the method itself (in a place where you did initially, but with a more sophisticated logic like currently)

Be sure that aststrip.py resets the flags, and deps.py generates dependencies so that subclass method callsites are all rechecked when the signature in supertype changes (this may be already the case, at least partially).

@gantsevdenis I can't find your comment that I have got by e-mail, but here is the response. Self-types is something like this:

T = TypeVar('T') class Base: def copy(self: T) -> T: ... class Sub(Base): ... x = Sub().copy() # inferred type of 'x' is 'Sub'

ilevkivskyi · 2019-10-23T19:10:47Z

mypy/semanal.py

+    Then class B will be transformed to:
+         class B(A[int,str]):
+            def foo(self) -> str:pass
+            def bar(self,*args)->Callable[[str],int]:pass


Please use PEP 8 everywhere including in docstring and in tests! (You can however skip empty lines in tests whenever it makes sense).

ilevkivskyi · 2019-10-23T19:13:29Z

mypy/semanal.py

+        assert isinstance(parent, FuncDef), "got %s" % type(parent)
+        pnb_args, cnb_args = len(parent.arg_kinds), len(func.arg_kinds)
+        if cnb_args < pnb_args:
+            self.fail("Signature of \"%s\" incompatible with supertype \"%s\"" %


All the message generating logic should live messages.py. Also I think we should emit a more specific error message, explaining that we can't copy the signature, and mentioning what went wrong (not enough arguments, etc).

Also in Python you can use 'single quotes to avoid "escaping" double quotes' (also below).

ilevkivskyi · 2019-10-23T19:20:40Z

test-data/unit/check-classes.test

+    def return_VT(self, *args,**kwargs): return 5
+    def return_VT_wrong(self,*args,**kwargs): return ""  # E: Incompatible return value type (got "str", expected "int")
+    def return_Tuple(self,*args,**kwargs): return ("", 5)  # E: Incompatible return value type (got "Tuple[str, int]", expected "Tuple[int, str]")
+[builtins fixtures/dict.pyi]


Several important scenarios are still missing, for example:

Methods with self-types

Generic inheritance where subclass has new type variables with non-trivial mapping to old ones

Chained inheritance where C inherits B that inherits A

Multiple inheritance

Incremental mode tests, see check-incremental.test

Daemon mode tests, see fine-grained.test

Import cycles

Tests that inherited signature is used on external and internal access (including situations with deferrals)

ilevkivskyi · 2019-10-23T19:23:16Z

Also please note there are some merge conflicts.

rmorshea · 2019-11-11T21:56:33Z

docs/source/command_line.rst

@@ -276,6 +276,10 @@ definitions or calls.
    It will assume all arguments have type ``Any`` and always infer ``Any``
    as the return type.

+``--inherit-signatures``
+    This allows to use base class function signature in derived classes, without explicitly


Slight typo - "This allows the use of..." instead of "This allows to use..."

rmorshea · 2019-11-11T21:58:09Z

This is awesome work and would go a long way in resolving all my use cases for python/typing#270. Thanks for this!

Edit: this doesn't actually cover python/typing#270 but much of the logic here would probably be shared with any implementation of the features suggested therein.

robertwb · 2019-11-13T00:03:51Z

I would love to see this as well, would eliminate a lot of DRY violations that are currently required for typing.

msullivan · 2020-10-18T21:23:58Z

I'm going to close this because it has been open with changes requested for quite some time. Anyone should feel free to pick it back up, though!

Add new option, added test

cd1d3f4

gantsevdenis changed the title ~~Add new option, added test~~ Fix 3903: untyped defs on derived classes Sep 21, 2019

fix none case

0e33def

ilevkivskyi reviewed Sep 28, 2019

View reviewed changes

gantsevdenis added 5 commits October 8, 2019 08:58

add tests

5a95d7c

support generics, moved to semanal.py

7e349a3

update doc

600a105

undo changes

61c1d6d

moved to semanal.py

17a3485

gantsevdenis added 3 commits October 8, 2019 10:12

py35 style

67856cd

py35 style

ec7657c

py35 style types

1c908a7

ilevkivskyi requested changes Oct 23, 2019

View reviewed changes

chadrik mentioned this pull request Nov 6, 2019

[BEAM-7746] Add python type hints (part 1) apache/beam#9915

Merged

3 tasks

ilevkivskyi mentioned this pull request Nov 10, 2019

Proposal: signature copying for kwargs. python/typing#270

Closed

rmorshea reviewed Nov 11, 2019

View reviewed changes

Kami mentioned this pull request Mar 1, 2020

Add Python type annotation for base Compute API apache/libcloud#1306

Merged

ilevkivskyi mentioned this pull request Mar 6, 2020

Inheriting type hints from abstract parents #8492

Closed

msullivan closed this Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 3903: untyped defs on derived classes #7548

Fix 3903: untyped defs on derived classes #7548

gantsevdenis commented Sep 21, 2019 •

edited

Loading

ilevkivskyi left a comment

ilevkivskyi commented Sep 28, 2019

gantsevdenis commented Oct 8, 2019 •

edited

Loading

ilevkivskyi left a comment

ilevkivskyi Oct 23, 2019

ilevkivskyi Oct 23, 2019

ilevkivskyi Nov 13, 2019

ilevkivskyi Oct 23, 2019

ilevkivskyi Oct 23, 2019

ilevkivskyi Oct 23, 2019

ilevkivskyi commented Oct 23, 2019

rmorshea Nov 11, 2019

rmorshea commented Nov 11, 2019 •

edited

Loading

robertwb commented Nov 13, 2019

msullivan commented Oct 18, 2020

		@@ -160,6 +160,45 @@
		Tag = int


		def substitute_unbounds(t: UnboundType, names_to_types: Mapping[str, Instance]) -> ProperType:

Fix 3903: untyped defs on derived classes #7548

Fix 3903: untyped defs on derived classes #7548

Conversation

gantsevdenis commented Sep 21, 2019 • edited Loading

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Sep 28, 2019

gantsevdenis commented Oct 8, 2019 • edited Loading

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi Oct 23, 2019

Choose a reason for hiding this comment

ilevkivskyi Oct 23, 2019

Choose a reason for hiding this comment

ilevkivskyi Nov 13, 2019

Choose a reason for hiding this comment

ilevkivskyi Oct 23, 2019

Choose a reason for hiding this comment

ilevkivskyi Oct 23, 2019

Choose a reason for hiding this comment

ilevkivskyi Oct 23, 2019

Choose a reason for hiding this comment

ilevkivskyi commented Oct 23, 2019

rmorshea Nov 11, 2019

Choose a reason for hiding this comment

rmorshea commented Nov 11, 2019 • edited Loading

robertwb commented Nov 13, 2019

msullivan commented Oct 18, 2020

gantsevdenis commented Sep 21, 2019 •

edited

Loading

gantsevdenis commented Oct 8, 2019 •

edited

Loading

rmorshea commented Nov 11, 2019 •

edited

Loading