Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix generics inheritance and drop __subclasscheck__ #207

Closed
wants to merge 3 commits into from

Conversation

bintoro
Copy link
Contributor

@bintoro bintoro commented Apr 20, 2016

I don't expect this to be reviewed anytime soon, but the effort is now fairly complete, so might as well put it out there for comments.

Resolves #136
Resolves #202
Resolves #203

At the heart of #136 is a conflation between type compatibility and subtype relationships. Even though Sequence[Manager] is compatible with Sequence[Employee], it's not a subclass of it. Rather, both derive from Sequence[+T_co].

To address this, the PR introduces a new helper is_compatible(), which implements the "is consistent with or a subtype of" relation. In short, it does what issubclass() has been doing until now.

The practical end result is that

  • issubclass() is no longer applicable to the special types Any, Union, etc.

  • is_compatible() always works — if neither argument is a special type, is_compatible(T1, T2) reduces to issubclass(T1, T2)

  • issubclass() acquires its normal semantics w.r.t. generics:

    >>> issubclass(Sequence[Manager], Sequence[Employee])
    False
    >>> issubclass(Sequence[Manager], Sequence)
    True

    whereas:

    >>> is_compatible(Sequence[Manager], Sequence[Employee])
    True

Commit summary

There are three commits:

  • The first simply stops the __extra__ linkage from leaking down to generics' subclasses. This fixes the bug where issubclass(dict, CustomMapping) == True. [Diff]
  • The second adds the __extra__ ABCs as actual base classes of their typing twins. It also refactors GenericMeta.__subclasscheck__ into a compatibility check and a __subclasshook__ dedicated to handling the __extra__ connection. [Diff] This achieves a few things:
    • User-defined generics become recognized as subclasses of the associated collections ABCs.
    • Abstract and mixin methods from collections.abc become available.
    • issubclass(list, Iterable[X]) no longer returns true for arbitrary X (Kill __subclasscheck__ #136).
  • The final commit is largely a mechanical change: the special types are made to raise a TypeError in response to issubclass(). Every type metaclass gets a method named __is_compatible__ to host the prior __subclasscheck__ logic. [Diff]

Notes

  • Full parity is preserved in using unparameterized types in place of the corresponding collections.abc classes in isinstance() and issubclass() calls:

    issubclass(type, typing.Callable) == issubclass(type, collections.Callable) == True
    issubclass(list, typing.Iterable) == issubclass(list, collections.Iterable) == True
    ...
  • The intent has been not to mess with the types' behavior. That said, the introduction of is_compatible() yields a couple of automatic improvements regarding Any:

    • The relation evaluates to true when either argument is Any; previously issubclass(Any, C) would return false for some arbitrary class C but true for a typing type.
    • is_compatible(Iterable[Any], Iterable[int]) correctly returns true.
  • The merging of Specific asserts #205 caused a huge conflict in the tests. I will look into it eventually if there's interest in merging this.

  • The proposal is compatible with the plan to convert some of the collection types to protocols, if that's still on the table.

@bintoro bintoro force-pushed the extra-as-base branch 2 times, most recently from c12d92f to bbeadb1 Compare April 21, 2016 11:56
@ilevkivskyi
Copy link
Member

I like this PR, it looks like it could fix many issues. But I have some questions:

  1. You have two different functions is_consistent and is_compatible. As I understand we need only one function that should answer whether assigning a value with type t1 to a variable with declared/inferred type t2 considered an error or not. Do I understand correctly that is_compatible supposed to do exactly this, and why do you need a second function?
  2. Maybe you should update your tests taking into account more of this: Specific asserts #205 (maybe at least use self.assertIsInstance on lines 1247-1250, etc.)
  3. This class definition class List(list, MutableSequence[T], extra=list) and the same for set, frozenset, dict, defaultdict violate DRY (you need to repeat the base class in extra every time), and, to be honest, look a bit ugly. Is it possible to somehow simplify this?

@bintoro
Copy link
Contributor Author

bintoro commented May 4, 2016

@ilevkivskyi The short answer to 1 and 3 is that, when it comes to certain details, the PR represents a WIP due to the exact requirements not being known.

  1. is_consistent is just a helper performing an Any-aware identity comparison. It's quite possible that it will simply be folded into is_compatible. It should be marked as private, but for all I know, neither function might end up as part of the module's public API. (I'm not aware of a decision laying out what, if any, type checking features typing.py should provide.)
  2. I wrote 99% of the PR before Specific asserts #205 was submitted, so the tests are based on an earlier revision. I can certainly update them to follow the unittest style, but that alone won't fix the conflict.
  3. The duplicates in the definitions of List & co. were introduced to make the first commit a self-contained patch that can work without the rest. They are there to satisfy the test that checks for issubclass(list, List) == True.

In the context of the full PR, the equality becomes very questionable. And even if we do want it to hold, the duplication won't be necessary. (The conversion of extras into bases will no longer skip builtins here.)

EDIT: Looking at this again, I recalled the reason for not eliminating the duplicates yet. It's Generator and the fact that on Python 3.4 and earlier its extra companion is GeneratorType, which cannot be inherited from. For the time being, I settled on a blanket ban on automatically inserting builtins as bases; hence the duplication in cases where we do want to derive from a builtin.

@gvanrossum
Copy link
Member

Superseded by #283. Many of the ideas here went into that PR, and it closes most of the same issues. Thanks!

@gvanrossum gvanrossum closed this Sep 28, 2016
gvanrossum pushed a commit that referenced this pull request Oct 9, 2016
 Here I am adding tests as discussed with @bintoro (related to #207).

These tests actually revealed three small bugs:

- Old style classes in Python don't have `__mro__`.
- We should use `__extra__` instead of extra in Python2 (since no kwargs in classes).
- We should allow overriding `__subclasshook__` by subclasses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants