classification
Title: UserList.__getitem__ doesn't account for slices
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, mblahay, r.david.murray, rhettinger, serhiy.storchaka, staticshock, vaultah
Priority: low Keywords: easy, patch

Created on 2016-07-28 06:15 by staticshock, last changed 2019-05-26 17:36 by SilentGhost. This issue is now closed.

Files
File name Uploaded Description Edit
patch staticshock, 2016-07-28 06:15 A patch to return a new UserList when UserList is sliced review
Pull Requests
URL Status Linked Edit
PR 13150 closed serhiy.storchaka, 2019-05-07 06:45
PR 4981 vaultah, 2019-05-07 13:54
PR 13169 merged python-dev, 2019-05-07 19:11
PR 13181 closed miss-islington, 2019-05-07 21:41
PR 13203 merged mblahay, 2019-05-08 17:51
Repositories containing patches
https://bitbucket.org/staticshock/cpython
Messages (20)
msg271503 - (view) Author: Anton Backer (staticshock) * Date: 2016-07-28 06:15
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.
msg271532 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-07-28 11:58
LGTM, but this is change we should probably only make in a feature release, with a note in the What's New porting section.
msg271615 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-07-29 07:06
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]
msg341618 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-06 19:30
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).
msg341644 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-06 20:52
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])
msg341645 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-06 20:54
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
msg341647 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-06 20:57
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.
msg341684 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-07 06:15
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.
msg341725 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-07 13:44
Thank you for bringing up that PR. My team will review and try to find out why it was closed without a merge.
msg341726 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-07 13:45
Please note that I am working with Erick Cervantes at PyCon2019 on this issue.
msg341747 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-07 15:14
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!
msg341833 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-05-07 21:41
New changeset b1c3167c232c36ed3543ca351ff10c613639b5f5 by Mark Shannon (Michael Blahay) in branch 'master':
bpo-27639: Correct return type for UserList slicing operation (#13169)
https://github.com/python/cpython/commit/b1c3167c232c36ed3543ca351ff10c613639b5f5
msg341854 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-08 06:34
The real name of vaultah is Dmitry Kazakov. His PR LGTM. Did you notice that I recreated the original PR?
msg341919 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-08 18:09
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.
msg341980 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-09 14:15
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.
msg342131 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-10 20:50
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
msg342596 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-15 19:34
PR 13203 is still waiting for merge
msg343451 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-25 03:03
PR 13203 is still waiting for merge
msg343551 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-05-26 14:28
New changeset f3d909428c7c61ea32e8fbb9c8b48344e7904a53 by Mark Shannon (Michael Blahay) in branch '3.7':
BPO-27639: Correct return type for UserList slicing operation (#13203)
https://github.com/python/cpython/commit/f3d909428c7c61ea32e8fbb9c8b48344e7904a53
msg343570 - (view) Author: Michael Blahay (mblahay) * Date: 2019-05-26 17:32
Thank you Mark. Everything for this one is complete. The issue may be closed.
History
Date User Action Args
2019-05-26 17:36:51SilentGhostsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-05-26 17:32:39mblahaysetmessages: + msg343570
2019-05-26 14:28:14Mark.Shannonsetmessages: + msg343551
2019-05-25 03:03:39mblahaysetmessages: + msg343451
2019-05-15 19:34:49mblahaysetmessages: + msg342596
2019-05-10 23:27:54rhettingersetassignee: rhettinger ->
2019-05-10 20:50:26mblahaysetmessages: + msg342131
2019-05-09 14:15:48mblahaysetmessages: + msg341980
2019-05-08 18:09:54mblahaysetmessages: + msg341919
2019-05-08 17:51:06mblahaysetpull_requests: + pull_request13114
2019-05-08 06:34:12serhiy.storchakasetmessages: + msg341854
2019-05-07 21:41:18miss-islingtonsetpull_requests: + pull_request13097
2019-05-07 21:41:08Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg341833
2019-05-07 19:11:49python-devsetpull_requests: + pull_request13085
2019-05-07 15:14:49mblahaysetmessages: + msg341747
2019-05-07 13:54:37vaultahsetpull_requests: + pull_request13069
2019-05-07 13:45:08mblahaysetmessages: + msg341726
2019-05-07 13:44:15mblahaysetmessages: + msg341725
2019-05-07 06:45:25serhiy.storchakasetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request13065
2019-05-07 06:15:41serhiy.storchakasetmessages: + msg341684
2019-05-06 20:57:44mblahaysetmessages: + msg341647
2019-05-06 20:54:31mblahaysetmessages: + msg341645
2019-05-06 20:52:21mblahaysetmessages: + msg341644
2019-05-06 19:30:41mblahaysetnosy: + mblahay
messages: + msg341618
2019-05-06 16:00:47serhiy.storchakasetkeywords: + easy, - patch
stage: patch review -> needs patch
versions: + Python 3.7, Python 3.8, - Python 3.6
2019-05-06 15:55:03vaultahsetpull_requests: - pull_request4870
2017-12-22 22:02:46vaultahsetnosy: + vaultah
2017-12-22 21:43:07vaultahsetstage: needs patch -> patch review
pull_requests: + pull_request4870
2016-07-29 07:06:35rhettingersetmessages: + msg271615
2016-07-29 07:01:29rhettingersetmessages: - msg271612
2016-07-29 06:33:47rhettingersetmessages: + msg271612
2016-07-28 16:15:47staticshocksetfiles: - 581663cb2d4d.diff
2016-07-28 16:12:39staticshocksetfiles: + 581663cb2d4d.diff
keywords: + patch
2016-07-28 11:58:06r.david.murraysetversions: + Python 3.6, - Python 3.5
nosy: + r.david.murray

messages: + msg271532

stage: needs patch
2016-07-28 06:22:51staticshocksethgrepos: + hgrepo351
2016-07-28 06:18:32serhiy.storchakasetpriority: normal -> low
assignee: rhettinger

nosy: + serhiy.storchaka, rhettinger
2016-07-28 06:15:07staticshockcreate