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

UserList.__getitem__ doesn't account for slices #71826

Closed
staticshock mannequin opened this issue Jul 28, 2016 · 20 comments
Closed

UserList.__getitem__ doesn't account for slices #71826

staticshock mannequin opened this issue Jul 28, 2016 · 20 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@staticshock
Copy link
Mannequin

staticshock mannequin commented Jul 28, 2016

BPO 27639
Nosy @rhettinger, @bitdancer, @markshannon, @serhiy-storchaka, @vaultah, @staticshock, @mblahay
PRs
  • bpo-27639: Slices of UserLists are instances of UserList now. #13150
  • bpo-27639: slices of UserLists should be instances of UserList #4981
  • bpo-27639: Correct return type for UserList slicing operation #13169
  • [3.7] bpo-27639: Correct return type for UserList slicing operation (GH-13169) #13181
  • [3.7] bpo-27639: Correct return type for UserList slicing operation #13203
  • Files
  • patch: A patch to return a new UserList when UserList is sliced
  • 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 = None
    closed_at = <Date 2019-05-26.17:36:51.318>
    created_at = <Date 2016-07-28.06:15:07.780>
    labels = ['3.7', '3.8', 'type-bug', 'library', 'easy']
    title = "UserList.__getitem__ doesn't account for slices"
    updated_at = <Date 2019-05-26.17:36:51.317>
    user = 'https://github.com/staticshock'

    bugs.python.org fields:

    activity = <Date 2019-05-26.17:36:51.317>
    actor = 'SilentGhost'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-05-26.17:36:51.318>
    closer = 'SilentGhost'
    components = ['Library (Lib)']
    creation = <Date 2016-07-28.06:15:07.780>
    creator = 'staticshock'
    dependencies = []
    files = ['43914']
    hgrepos = ['351']
    issue_num = 27639
    keywords = ['patch', 'easy']
    message_count = 20.0
    messages = ['271503', '271532', '271615', '341618', '341644', '341645', '341647', '341684', '341725', '341726', '341747', '341833', '341854', '341919', '341980', '342131', '342596', '343451', '343551', '343570']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'r.david.murray', 'Mark.Shannon', 'serhiy.storchaka', 'vaultah', 'staticshock', 'mblahay']
    pr_nums = ['13150', '4981', '13169', '13181', '13203']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27639'
    versions = ['Python 3.7', 'Python 3.8']

    @staticshock
    Copy link
    Mannequin Author

    staticshock mannequin commented Jul 28, 2016

    The docs for UserList say:

    List operations which return a new sequence attempt to create an instance of the actual implementation class.

    However, UserList([])[:] returns a list, not a UserList.

    @staticshock staticshock mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 28, 2016
    @bitdancer
    Copy link
    Member

    LGTM, but this is change we should probably only make in a feature release, with a note in the What's New porting section.

    @rhettinger
    Copy link
    Contributor

    Take a look at the following code from UserList.py in Python2.7 to get an idea of how this was implemented previously:

        def __getslice__(self, i, j):
            i = max(i, 0); j = max(j, 0)
            return self.__class__(self.data[i:j])
        def __setslice__(self, i, j, other):
            i = max(i, 0); j = max(j, 0)
            if isinstance(other, UserList):
                self.data[i:j] = other.data
            elif isinstance(other, type(self.data)):
                self.data[i:j] = other
            else:
                self.data[i:j] = list(other)
        def __delslice__(self, i, j):
            i = max(i, 0); j = max(j, 0)
            del self.data[i:j]

    @serhiy-storchaka serhiy-storchaka added easy 3.7 (EOL) end of life 3.8 only security fixes labels May 6, 2019
    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    Here is a test that more explicitly shows the problem.

    >>> from collections import UserList
    >>> UserList([0,1,2,3,4,5])[0:2].__class__
    <class 'list'>
    >>> 

    It can clearly be seen here that the return type of the slicing operator is list when it should, in this case, be UserList (or whatever class one is using that is derived from UserList).

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    The root cause of this issue seems to be the failure to implement type usage in __getitem__ when the deprecated __getslice__ was removed. This is why slicing worked correctly in 2.7, but not the 3.x versions.

    In 3.8, the __getitem__ method is used to create the slice, but here we can see that all it does is pass the task to data, which is of type list and then fails to convert the result to the correct type.

      def __getitem__(self, i): return self.data[i]

    Using other methods as examples, the fix should look like this:

      def __getitem__(self, i): return self.__class__(self.data[i])

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    It is also worth noting that the definition of UserList moved from Lib/UserList.py in 2.7 to Lib/collections/init.py in 3.x

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    Results from a quick unit test on the proposed changes were positive:

    >>> from collections import UserList
    >>> UserList([0,1,2,3,4,5])[0:2].__class__
    <class 'collections.UserList'>

    If you compare this result with the one a couple comments above, you can see that the result is no longer a list, but rather of type UserList.

    @serhiy-storchaka
    Copy link
    Member

    PR 4981 LGTM (except that it would be worth to add the author's name in the NEWS entry). It is sad that it was not reviewed for 2 years and is closed now.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 7, 2019

    Thank you for bringing up that PR. My team will review and try to find out why it was closed without a merge.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 7, 2019

    Please note that I am working with Erick Cervantes at PyCon2019 on this issue.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 7, 2019

    Let it be known that the author of PR 4981 is known as vaultah. He/she personally closed the pull request this morning stating a lack of need to be recognized for the work. Per his/her instructions I am reviewing the changes and incorporating in our solution as needed. Thank you vaultah!

    @markshannon
    Copy link
    Member

    New changeset b1c3167 by Mark Shannon (Michael Blahay) in branch 'master':
    bpo-27639: Correct return type for UserList slicing operation (bpo-13169)
    b1c3167

    @serhiy-storchaka
    Copy link
    Member

    The real name of vaultah is Dmitry Kazakov. His PR LGTM. Did you notice that I recreated the original PR?

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 8, 2019

    PR 13150, the recreation of PR 4981, was noticed just after I created PR 13169. It was decided to continue forward with PR 13169 since PR 13150 contained changes that were out of scope for BPO-27639 for which it was suspected might have been the cause of the non-merging of PR 4981. Thank for identifying Dmitry; I did my best to give credit where credit was due.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 9, 2019

    For those that are wondering what is going on with PR 13181 and PR 13203, those are both for back porting the change to 3.7. The auto generated PR 13181 failed for an unknown reason and then the bot deleted the branch so the PR could not be taken any further. PR 13203 is a manual attempt at the backport which is moving along much for successfully.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 10, 2019

    PR 13203 has finally made it through all checks successfully and is now awaiting merging. Please someone pick it up and merge it. Thank you

    @rhettinger rhettinger removed their assignment May 10, 2019
    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 15, 2019

    PR 13203 is still waiting for merge

    1 similar comment
    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 25, 2019

    PR 13203 is still waiting for merge

    @markshannon
    Copy link
    Member

    New changeset f3d9094 by Mark Shannon (Michael Blahay) in branch '3.7':
    BPO-27639: Correct return type for UserList slicing operation (bpo-13203)
    f3d9094

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 26, 2019

    Thank you Mark. Everything for this one is complete. The issue may be closed.

    @SilentGhost SilentGhost mannequin closed this as completed May 26, 2019
    @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.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants