A real fix for issue #250 (failure with mock) #295

ilevkivskyi · 2016-10-09T21:51:23Z

@gvanrossum
This PR actually fixes #250

The main idea here is optimizing generics for cases where a type information is added to existing code.
Now:

Node[int] and Node have identical __bases__ and identical __mro__[1:] (except for the first item, since it is the class itself).
After addition of typing information (i.e. making some classes generic), __mro__ is changed very little, at most one bare Generic appears in __mro__.
Consequently, only non-parameterized generics appear in __bases__ and __mro__[1:].

Interestingly, this could be achieved in few lines of code and no existing test break.

On the positive side of this approach, there is very little chance that existing code (even with sophisticated "magic") will break after addition of typing information.

On the negative side, it will be more difficult for runtime type-checkers to perform decorator-based type checks (e.g. enforce method overriding only by consistent methods). Essentially, now type erasure happens partially at the class creation time (all bases are reduced to origin).

gvanrossum

Hm, I don't like losing the extra info from the bases and mro. While Mark probably wants me to erase all type information at runtime (leading to a much smaller typing.py) I prefer to save enough information so that runtime introspection can work.

E.g. if I write

T = TypeVar('T')
class C(Dict[str, T]): pass

I want to be able to reconstruct that T corresponds to the value of the dict.

gvanrossum · 2016-10-10T15:49:16Z

python2/typing.py

+        if extra is not None and type(extra) is abc.ABCMeta and extra not in bases:
+            bases = (extra,) + bases
+        bases = tuple(_gorg(b) if isinstance(b, GenericMeta) else b for b in bases)
+        if any(isinstance(b, GenericMeta) and b is not Generic for b in bases):


This could use a comment explaining what's happening -- it looks like the idea is to remove bare Generic from the bases if there are other generic-derived bases, but to leave generic if if not.

markshannon · 2016-10-10T16:17:06Z

I do indeed want a much smaller typing.py
After all, if whoever is writing this mythical runtime type-checker can implement a useful type-checker, then they can manage some variation on ast.parse(inspect.getsource(...))

ilevkivskyi · 2016-10-10T22:27:25Z

@gvanrossum
I added the comment you requested and I propose a compromise solution: in latest commit I added a field __orig_bases__ to non-parameterized generics that preserves original bases before we start all manipulations with __extra__ and __bases__. This way:

The fix still works (and should prevent other bugs in existing code after addition of typing info)
Only some classes in typing.py get slightly bigger (as compared to Mark's ideal).
One can extract all the necessary original typing info at runtime using __orig_bases__ and little effort.

gvanrossum · 2016-10-10T23:14:40Z

Can you explain how this fixes #250 better than the previous fix for that issue?

ilevkivskyi · 2016-10-11T08:17:34Z

@gvanrossum

Can you explain how this fixes #250 better than the previous fix for that issue?

Oh, sorry, I should have started from this. There was no fix. I just mentioned it in #289

A side note, while playing with this I have found an easy way to fix #250. I will submit a PR later when we will decide on the permanent fix for this one.

And there by "a PR later" I meant exactly this PR, but it looks like you closed #250 by #289.
Sorry for misunderstanding.

gvanrossum · 2016-10-11T16:59:43Z

Actually 250 was auto-closed by GitHub because of the phrase "fix #250" in your commit message. (I reopened it.)

gvanrossum

Sorry that these reviews take so long to turn around. Many things I can review between two bytes of breakfast, but typing stuff requires real thought. :-)

gvanrossum · 2016-10-14T22:53:03Z

src/typing.py

        self.__parameters__ = tvars
        self.__args__ = args
        self.__origin__ = origin
        self.__extra__ = extra
        # Speed hack (https://github.com/python/typing/issues/196).
        self.__next_in_mro__ = _next_in_mro(self)
+        if origin is None:


This condition is a little mysterious. It seems to say "if we were called from a class statement" (per line 947). I wonder if it would be more straightforward to set __orig_bases__ exactly in case orig_bases != bases? Or maybe not?

I changed this a bit, to make it more clear and convenient to use. Now __orig_bases__ is always created on subclassing, and then copied on __getitem__. So that one don't need to use _gorg to find original bases, they are accessible immediately. I added a comment in code as well.

gvanrossum · 2016-10-14T23:03:53Z

src/test_typing.py

+        T = TypeVar('T')
+        U = TypeVar('U')
+        V = TypeVar('V')
+        # these should just work


That's slang for "these shouldn't crash", but it would be pedagogical to show what the various new dunder attributes of C and D actually are.

OK, fixed this.

gvanrossum · 2016-10-14T23:07:07Z

src/test_typing.py

+    def test_orig_bases(self):
+        T = TypeVar('T')
+        class C(typing.Dict[str, T]): ...
+        self.assertEqual(C.__orig_bases__, (typing.Dict[str, T],))


I know there are no docs for __parameters__ etc. either, but I would still like to see some kind of example code that takes a class like this and recovers what it means given the various new dunder methods (__origin__, __orig_bases__, __parameters__). E.g. if I had a dict {x: y} how would I check that it's a subclass of Dict[str, T]? (The answer should reveal that the type of x must be str and T must be the type of y.)

It is quite difficult to write a reasonable runtime type check functions. I just added three naive functions just to illustrate how to use the dunder attributes for typical type checking tasks.

In process of doing this I realized that it is difficult to perform runtime type checks since type erasure happens not only at subclassing, but also at generic class instantiation. In the new commit I added __orig_class__ to instances (if subclass allows this, i.e., it does not use __slots__) that saves a reference to original class before type erasure.
So that now Node[int]().__class__ is Node, while Node[int]().__orig_class__ is Node[int].

…for generic instances

ilevkivskyi · 2016-10-16T16:15:54Z

@gvanrossum
I agree with your comments, please take a look at the new version (details of changes are in replies to your comments in code).

gvanrossum

Just a few nits -- I'm pretty positive about this now!

gvanrossum · 2016-10-20T22:51:59Z

python2/test_typing.py

+        def naive_dict_check(obj, tp):
+            # Check if a dictionary conforms to Dict type
+            if len(tp.__parameters__) > 0:
+                return NotImplemented


NotImplemented is not a great value to return here -- it's really only supposed to be used by binary operators (e.g. __add__) to indicate that the reverse variant should be tried (i.e. __radd__). Maybe raising NotImplementedError would be better?

While this is just a test, it will be used as an example and copied, and raising makes it clearer that that's an unimplemented feature of the naive check.

gvanrossum · 2016-10-20T22:58:17Z

src/typing.py

@@ -1128,6 +1130,8 @@ def __new__(cls, *args, **kwds):
        else:
            origin = _gorg(cls)
            obj = cls.__next_in_mro__.__new__(origin)
+            if '__dict__' in cls.__dict__:
+                obj.__orig_class__ = cls


I think I'd just catch AttributeError here -- the "dict in dict" idiom feels obscure.

ilevkivskyi · 2016-10-20T23:16:47Z

@gvanrossum Thank you for review! Fixed both points.

gvanrossum · 2016-10-21T02:58:17Z

OK godspeed!

gvanrossum · 2016-10-21T02:58:52Z

Python 3.6 beta 3 goes out on Oct 31. We have some time to think about this still. Did you have other things you really wanted to get in?

ilevkivskyi · 2016-10-21T23:00:51Z

@gvanrossum

Python 3.6 beta 3 goes out on Oct 31. We have some time to think about this still. Did you have other things you really wanted to get in?

I have an idea of how to fix all three issues #298 #299 #301 in a single PR. It should be not big. However, it will change how the types store parameters. Now, e.g. Union uses __union_params__, Callable uses __args__ and __result__, etc. I am going to change this so that all use the same scheme as Generic: __parameters__, __args__, and __origin__. This will allow to "unify" several pieces of code and fix the three mentioned issues. I think I will finish this by Monday or Tuesday.

gvanrossum · 2016-10-25T18:22:29Z

SGTM. I originally gave them all different names to ensure I would not confuse myself, but I think in retrospect it's better to unify.

ilevkivskyi added 3 commits October 9, 2016 23:20

A real fix for python#250

b07b8ea

Use _gorg for the case of multiply parameterized generics

eb7801b

Formatting

be5d7ed

gvanrossum reviewed Oct 10, 2016

View reviewed changes

Response to comments + some tests

63482fa

ilevkivskyi mentioned this pull request Oct 11, 2016

Alternative (simpler) repr for generics #296

Merged

gvanrossum mentioned this pull request Oct 12, 2016

isinstance + class indirectly deriving from SupportsInt results in "TypeError: Protocols cannot be used with isinstance()" #297

Closed

gvanrossum reviewed Oct 14, 2016

View reviewed changes

Add tests illustrating new dunder attributes; preserve initial class …

307bf64

…for generic instances

gvanrossum requested changes Oct 20, 2016

View reviewed changes

Response to comments

f4932bb

gvanrossum merged commit a38bdac into python:master Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A real fix for issue #250 (failure with mock) #295

A real fix for issue #250 (failure with mock) #295

ilevkivskyi commented Oct 9, 2016

gvanrossum left a comment

gvanrossum Oct 10, 2016

markshannon commented Oct 10, 2016

ilevkivskyi commented Oct 10, 2016 •

edited

Loading

gvanrossum commented Oct 10, 2016 via email

ilevkivskyi commented Oct 11, 2016

gvanrossum commented Oct 11, 2016 •

edited

Loading

gvanrossum left a comment

gvanrossum Oct 14, 2016

ilevkivskyi Oct 16, 2016

gvanrossum Oct 14, 2016

ilevkivskyi Oct 16, 2016

gvanrossum Oct 14, 2016

ilevkivskyi Oct 16, 2016

ilevkivskyi commented Oct 16, 2016

gvanrossum left a comment

gvanrossum Oct 20, 2016

gvanrossum Oct 20, 2016

ilevkivskyi commented Oct 20, 2016

gvanrossum commented Oct 21, 2016

gvanrossum commented Oct 21, 2016

ilevkivskyi commented Oct 21, 2016

gvanrossum commented Oct 25, 2016 via email

A real fix for issue #250 (failure with mock) #295

A real fix for issue #250 (failure with mock) #295

Conversation

ilevkivskyi commented Oct 9, 2016

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Oct 10, 2016

ilevkivskyi commented Oct 10, 2016 • edited Loading

gvanrossum commented Oct 10, 2016 via email

ilevkivskyi commented Oct 11, 2016

gvanrossum commented Oct 11, 2016 • edited Loading

gvanrossum 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

ilevkivskyi commented Oct 16, 2016

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Oct 20, 2016

gvanrossum commented Oct 21, 2016

gvanrossum commented Oct 21, 2016

ilevkivskyi commented Oct 21, 2016

gvanrossum commented Oct 25, 2016 via email

ilevkivskyi commented Oct 10, 2016 •

edited

Loading

gvanrossum commented Oct 11, 2016 •

edited

Loading