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

collections.abc.Indexable #70176

Closed
abarnert mannequin opened this issue Jan 1, 2016 · 32 comments
Closed

collections.abc.Indexable #70176

abarnert mannequin opened this issue Jan 1, 2016 · 32 comments
Assignees
Labels
3.9 stdlib type-feature

Comments

@abarnert
Copy link
Mannequin

@abarnert abarnert mannequin commented Jan 1, 2016

BPO 25988
Nosy @rhettinger, @vstinner, @ned-deily, @ambv, @serhiy-storchaka, @MojoVampire, @timgraham, @ilevkivskyi, @Carreau, @carlbordum
PRs
  • #5414
  • #5460
  • #5734
  • #5743
  • #10596
  • #18545
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ilevkivskyi'
    closed_at = <Date 2019-10-07.10:22:42.808>
    created_at = <Date 2016-01-01.22:18:59.545>
    labels = ['type-feature', 'library', '3.9']
    title = 'collections.abc.Indexable'
    updated_at = <Date 2021-01-12.23:39:07.222>
    user = 'https://bugs.python.org/abarnert'

    bugs.python.org fields:

    activity = <Date 2021-01-12.23:39:07.222>
    actor = 'vstinner'
    assignee = 'levkivskyi'
    closed = True
    closed_date = <Date 2019-10-07.10:22:42.808>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2016-01-01.22:18:59.545>
    creator = 'abarnert'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 25988
    keywords = ['patch']
    message_count = 32.0
    messages = ['257311', '257487', '298712', '298728', '298734', '298751', '298777', '298790', '298791', '298810', '298830', '298835', '298838', '298879', '298887', '311147', '311310', '311311', '311319', '311335', '311341', '311346', '311668', '312316', '312317', '312321', '330063', '342921', '342930', '354067', '362221', '384984']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'vstinner', 'ned.deily', 'lukasz.langa', 'serhiy.storchaka', 'abarnert', 'josh.r', 'Tim.Graham', 'levkivskyi', 'mbussonn', 'carlbordum']
    pr_nums = ['5414', '5460', '5734', '5743', '10596', '18545']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25988'
    versions = ['Python 3.9']

    @abarnert
    Copy link
    Mannequin Author

    @abarnert abarnert mannequin commented Jan 1, 2016

    In an -ideas thread, Guido suggested (http://article.gmane.org/gmane.comp.python.ideas/37599):

    If we want some way to turn something that just defines __getitem__ and __len__ into a proper sequence, it should just be made to inherit from Sequence, which supplies the default __iter__ and __reversed__. (Registration is *not* good enough here.) If we really want a way to turn something that just supports __getitem__ into an Iterable maybe we can provide an additional ABC for that purpose; let's call it a HalfSequence until we've come up with a better name. (We can't use Iterable for this because Iterable should not reference __getitem__.)

    Later in the thread, Nick Coghlan suggested (http://article.gmane.org/gmane.comp.python.ideas/37614):

    Perhaps collections.abc.Indexable would work? Invariant:

    for idx, val in enumerate(container):
        assert container[idx] is val
    

    That is, while enumerate() accepts any iterable, Indexable containers
    have the additional property that the contained values can be looked
    up by their enumeration index. Mappings (even ordered ones) don't
    qualify, since they offer a key:value lookup, but enumerating them
    produces an index:key relationship.

    So, in particular:

    • Indexable is a subclass of Iterable.
    • Sequence is a subclass of Indexable, Sized, and Container instead of Iterable, Sized, and Container. (Or, if bpo-25987 also goes through, of Reversible, Indexable, Sized, and Container.)
    • The abstract method __getitem__ and the concrete __iter__ implementation get moved up from Sequence to Indexable.
    • Indexable does _not_ have a subclass hook (to avoid making every Mapping, generic type, etc. accidentally Indexable).

    This means that you can write this (borrowing an example from Steven D'Aprano in http://article.gmane.org/gmane.comp.python.ideas/23369/):

        class Squares(collections.abc.Indexable):
            def __getitem__(self, index):
                return index**2

    Because this no longer depends on the old-style sequence protocol, testing it with ABCs will work as expected.

    For related issues, see bpo-25987, bpo-25864, bpo-25958, and python/typing#170

    @abarnert abarnert mannequin added stdlib type-feature labels Jan 1, 2016
    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 4, 2016

    Interesting. I just hope the name doesn't confuse people into connecting this with the Index type (a type of integer that can't be cast from float).

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jul 20, 2017

    The last few weeks something bothered while working on Protocols PEP: protocols should be ideally compact (and PEP already emphasizes this).
    However, the only potential candidates for __getitem__ are Sequence and Mapping, that are both quite bulky (half dozen members each). I was thinking about different options like adding BaseMap as a base for Mapping and Sequence that will only have __getitem__. I expect this to be a popular protocol, since often people just need something that can be subscripted.

    Fortunately I stumbled into this issue. It looks like the optimal way now is:

    • Have an abstract base class (let's call it BaseMap, although I don't really like this name) in collections.abc that has only __getitem__ method.
    • It will be inherited by both Sequence and Mapping, but for the purpose of static typing, Sequence[T] will be a subtype of BaseMap[int, T], while Mapping[KT, VT] will be a subtype of BaseMap[KT, VT].
    • BaseMap will be contravariant in key, this will solve problems with Mapping (invariant in key), for example a function that expects BaseMap[str, int] will accept Dict[Union[str, unicode], int].
    • BaseMap will be a protocol in typing, so that people can extend it depending on their needs (e.g. add a .get() method).

    Guido, Łukasz if you agree, then I will add this to the Protocols PEP and will make a PR to collections.abc. We need to agree on the name, there are two options now: BaseMap and Indexable. I don't like the second since I would rather have it as a generic alias: Indexable = BaseMap[int, T]. However, BaseMap is also not very good, since I want something more "neutral" between Mapping and Sequence.

    @ilevkivskyi ilevkivskyi self-assigned this Jul 20, 2017
    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 20, 2017

    IIRC, the original motivation for ABCs was to differentiate distinct uses of __getitem__ (we forever struggled with differentiating sequences from mapping). It seems to me that this proposal is a step backwards. Other than a feeling of lightness, I don't think this proposal does anything for us. What is point of knowing an object is Subscriptable without knowing how it is to be used.

    The OP has a sense that Mapping and Sequence are "too heavy" but I think the reality that useful classes almost never use __getitem__ in isolation; rather, it is part of a small constellation of methods that are typically used together. I would prefer that collections.abc continue to reflect that reality.

    Also, I worry that collections.abc is becoming cluttered. The existence of use ABCs like MutableMapping is being drowned-out by one-trick-ponies. We're developing an unfavorable ratio of theoretical building blocks versus the practical tools.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jul 20, 2017

    Raymond,

    The existence of use ABCs like MutableMapping is being drowned-out by one-trick-ponies. We're developing an unfavorable ratio of theoretical building blocks versus the practical tools.

    Why do you think they are "theoretical"? FWIW, a simple search on GitHub for abc.X gives this:
    Sequence 322K
    Mapping 267K
    Iterable 244K
    Container 99K
    MutableMapping 77K

    I don't see any pattern here (although this may be only anecdotal evidence :-)

    Other than a feeling of lightness, I don't think this proposal does anything for us.

    This proposal makes more sense (and motivation) in the context of static typing, since it is a good way to know how __getitem__ is going to be used -- from its static type. Something typed with Subscriptable[int, T] would accept both Sequence[T] and Mapping[int, T]. The latter two are currently differentiated by __reversed__ (Sequence is Reversible as opposed to Mapping).

    Concerning an invariant that order of iteration is consistent with indexing by subsequent integers, I think this can't be checked reliably neither statically, nor by any simple runtime isinstance check. For example a subclass can override Indexable.__iter__ and break the iteration order. We can still add it, and rely on user cooperation.

    Anyway, I am not insisting on adding either Subscriptable nor Indexable, but I think it is important that these things are discussed in view of PEP-544.

    Alternative proposal would be to add Subscriptable and its subclass Indexable only to typing as protocols (with some special-casing in type checkers for the latter to provide a higher level of contract).
    (Concerning the name, I think Enumerable sounds better than Indexable, IIUC the idea is to work correctly with enumerate.)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jul 20, 2017

    At least one use of Indexable (by whatever name, but this rolls off the tongue the easiest) would be the metaclass in typing.py that allows one to write List[int] -- there's no containter here!

    The "drowning out" seems purely subjective -- perhaps if you order the contents of the module alphabetically you won't get much insight, but the docs should use a better organizing principle than that. (After all alphabetization was invented as a crutch for searching. :-)

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 21, 2017

    Guido, do you think there is some way to decouple collections.abc from spilling into the namespace for non-abc collections module?

    At the interactive prompt, running dir() on collections gives an alphabetical hodgepodge of the two modules. To my eyes, it is difficult to discern the concrete from the abstract.

    >>> dir(collections)
    ['AsyncGenerator', 'AsyncIterable', 'AsyncIterator', 'Awaitable', 'ByteString', 'Callable', 'ChainMap', 'Collection', 'Container', 'Coroutine', 'Counter', 'Generator', 'Hashable', 'ItemsView', 'Iterable', 'Iterator', 'KeysView', 'Mapping', 'MappingView', 'MutableMapping', 'MutableSequence', 'MutableSet', 'OrderedDict', 'Reversible', 'Sequence', 'Set', 'Sized', 'UserDict', 'UserList', 'UserString', 'ValuesView', '_Link', '_OrderedDictItemsView', '_OrderedDictKeysView', '_OrderedDictValuesView', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '_chain', '_class_template', '_collections_abc', '_count_elements', '_eq', '_field_template', '_heapq', '_iskeyword', '_itemgetter', '_proxy', '_recursive_repr', '_repeat', '_repr_template', '_starmap', '_sys', 'abc', 'defaultdict', 'deque', 'namedtuple']

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jul 21, 2017

    Names from collections.abc are re-exported to collections for backward compatibility. IIRC Serhiy also wanted to stop re-exporting them. I am not sure whether we need any "deprecation period" for this.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jul 21, 2017

    I'm not sure that *new* names in collections.abc should be re-exported to collections. This isn't required for backward compatibility.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jul 21, 2017

    Yes we will need a deprecation period for this starting with 3.7. It's fine
    not to import names newly added to collections.abc in 3.7 (i.e. collections
    would have to explicitly import everything that it got via * in 3.6).

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 22, 2017

    Yes we will need a deprecation period for this starting with 3.7.

    There are two ways to do this. A wimpy and clean way is to just add a deprecation notice in the docs. The thorough and messy way is to temporarily copy everything from the 3.6 collections.abc into the regular collections module and then insert deprecation warning code directly in each of copied classes.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jul 22, 2017

    The copying approach likely won't work because there will be code that uses isinstance(x, collections.abc.Sequence) and other code that uses isinstance(x, collections.Sequence) and they'll expect both to be true.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jul 22, 2017

    If we want to add programming warnings, we should replace sys.modules['collections'] with an instance of a module type subclass with corresponding properties raising a DeprecationWarning.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jul 22, 2017

    The copying approach likely won't work ...

    Then it seems like just adding a note to the docs is the way to go. That's probably okay though. I don't think we've every gotten much benefit from the programmatic deprecation warnings.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jul 23, 2017

    I think it's not worth the hack that Serhiy suggests. But we should make
    some noise around this change in the 3.7 announcement.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Jan 29, 2018

    New changeset e6d3421 by Raymond Hettinger in branch 'master':
    bpo-25988: Deprecate exposing collections.abc in collections #49664
    e6d3421

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 31, 2018

    Guido:

    But we should make some noise around this change in the 3.7 announcement.

    How about a paragraph in the Deprecation section of the "What's New in Python 3.7"? document

    https://docs.python.org/dev/whatsnew/3.7.html#deprecated

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 31, 2018

    > But we should make some noise around this change in the 3.7 announcement.

    How about a paragraph in the Deprecation section of the "What's New in Python 3.7"? document

    https://docs.python.org/dev/whatsnew/3.7.html#deprecated

    You mean other than what's there? The text that's currently there begins with "In Python 3.8, ..." which almost sounds like it's misplaced or a typo. Maybe you can amend it somehow to explain that in 3.7, using or importing the ABCs from "collections" instead of from "collections.abc" is deprecated, and in 3.8 it will stop working.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 31, 2018

    Can't we use the new shiny PEP-562 here to actually emit a warning when collections.Iterable is used instead of collections.abc.Iterable?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 31, 2018

    Can't we use the new shiny PEP-562 here to actually emit a warning when collections.Iterable is used instead of collections.abc.Iterable?

    PR 5460 implements this.

    It exposes issues in third-party packages. urllib3 and pyparsing directly use or import ABCs from 'collections'. request and pip are indirectly affected. If not add these warnings in 3.7 we can break the world in 3.8.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 31, 2018

    Good idea, Victor, and thanks for the PR, Serhiy!

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 31, 2018

    New changeset c66f9f8 by Serhiy Storchaka in branch 'master':
    bpo-25988: Emit a warning when use or import ABCs from 'collections'. (bpo-5460)
    c66f9f8

    @timgraham
    Copy link
    Mannequin

    @timgraham timgraham mannequin commented Feb 5, 2018

    The last commit that added the deprecation warning needs to be added to the 3.7 branch.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Feb 18, 2018

    New changeset 0442de5 by Ivan Levkivskyi in branch '3.7':
    bpo-25988: Emit a warning when use or import ABCs from 'collections'. (GH-5734)
    0442de5

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Feb 18, 2018

    Thanks, Tim, for noticing that and thanks, Ivan, for taking care of it. I should have cherrypicked this into 3.7.0b1.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Feb 18, 2018

    New changeset 2e84e47 by Ned Deily in branch '3.7':
    bpo-25988: add NEWS entry for 3.7.0b2 (bpo-5743)
    2e84e47

    @carlbordum
    Copy link
    Mannequin

    @carlbordum carlbordum mannequin commented Nov 18, 2018

    Thank you for all the hours all of you spent on making Python awesome.

    I would like to start contributing as well.

    Do you consider it time to remove __getattr__ in [Lib/collections/__init__.py](https://github.com/python/cpython/blob/main/Lib/collections/__init__.py) on master yet (introduced in pr bpo-5460)?

    @MojoVampire
    Copy link
    Mannequin

    @MojoVampire MojoVampire mannequin commented May 20, 2019

    mbussonn: Your new PR looks like it's related to bpo-36953 ("Remove collections ABCs?"), not this issue specifically. Can you withdraw/reissue attached to the correct issue?

    @Carreau
    Copy link
    Mannequin

    @Carreau Carreau mannequin commented May 20, 2019

    mbussonn: Your new PR looks like it's related to bpo-36953 ("Remove collections ABCs?"), not this issue specifically. Can you withdraw/reissue attached to the correct issue?

    Apologies, I've rebased and updated the issue numbers, not sure how I got the wrong one. I've also unlinked from here.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 7, 2019

    New changeset ef092fe by Serhiy Storchaka in branch 'master':
    bpo-25988: Do not expose abstract collection classes in the collections module. (GH-10596)
    ef092fe

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Feb 18, 2020

    New changeset af5ee3f by Victor Stinner in branch 'master':
    bpo-39674: Revert "bpo-25988: Do not expose abstract collection classes in the collections module. (GH-10596)" (GH-18545)
    af5ee3f

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 12, 2021

    New changeset c47c78b by Hugo van Kemenade in branch 'master':
    bpo-37324: Remove ABC aliases from collections (GH-23754)
    c47c78b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants