classification
Title: Most Set methods of KeysView and ItemsView do not work right
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: daniel.urban, eli.bendersky, eric.araujo, rhettinger, stutzbach
Priority: normal Keywords: easy, patch

Created on 2010-07-09 19:31 by stutzbach, last changed 2010-08-22 08:02 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
keysview_test.py stutzbach, 2010-07-09 19:31 Python 3 script to illustrate the problem
issue9214.1.patch eli.bendersky, 2010-07-26 04:27
issue9214.2.patch eli.bendersky, 2010-07-30 07:58
Messages (10)
msg109788 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-07-09 19:31
Attached is a simple Python 3 example script that defines a minimalist MutableMapping that simply wraps a dict, followed by:

x = MySimpleMapping(red=5)
y = x.keys()
z = x.keys() | {'orange'}                                          x['blue'] = 7
print(list(z))
print(list(z))

Output:
['blue', 'red', 'orange']
[]

Expected Output:
['orange', 'red']
['orange', 'red']

The problem is that __or__ ends up returning a new KeysView wrapping a generator instead of returning a set.

To solve the problem, KeysView and ItemsView need to override _from_iterable to return a set instead of a new view.
msg109789 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-07-09 19:32
Oops.  Somehow deleted a linefeed in there.  The example should read:

x = MySimpleMapping(red=5)
y = x.keys()
z = x.keys() | {'orange'}                                                       x['blue'] = 7
print(list(z))
print(list(z))
msg109790 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-07-09 19:33
I guess roundup is deleting my linefeed?  I'm sure I didn't do it that time.  I'm not sure what's going on there, but the "x['blue'] = 7" should be on a line by itself.
msg109805 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-09 23:02
> To solve the problem, KeysView and ItemsView need 
> to override _from_iterable to return a set instead 
> of a new view.

Thanks for the good analysis and suggested fix.  I believe both are correct and they match the behavior of real dictionaries. 

  def _from_iterable(self, it):
     return set(it)

Proposed tests to match real dicts:

  x = MySimpleMapping()   # from stuzback's example
  x['red'] = 5
  y = x.keys()
  assert isinstance(y, collections.Set)
  assert not isinstance(y, collections.MutableSet)
  z = x.keys() | {'orange'}
  assert type(z) is set
msg111593 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2010-07-26 04:27
Hello,

I'm attaching a patch for this issue. 

1. _from_iterable in KeysView and ItemsView overridden as per Daniel's suggestion (Lib/_abcoll.py)
2. Added a test case to Lib/test/test_collections.py that uses this test case (creates the subclass and attempts modifying the view), with Raymond's suggestions incorporated, as well as a simple correctness test.

The patch was made against the latest dev checkout. If it's found sufficient, I can backport it to 2.6/2.7

---

P.S. The patch was generated from a Hg mirror, so to patch it -p1 is to be used. If this is a problem, let me know and I'll generate a patch from an SVN working directory. I got the impression from the #python-dev IRC channel that submitting patches from Hg is fine, but I may be wrong here.
msg111990 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2010-07-29 19:35
The patch applies cleanly to the py3k branch (r83238). The unittests pass. 
The code looks good to me (it does exactly what was described as the solution). There are some trailing whitespace on some lines, that will need to be deleted.
msg112046 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2010-07-30 07:58
Attaching an updated patch, with the trailing whitespace removed. Hope it's more acceptable now.

P.S. Please let me know how to detect such issues in future patches.
msg112117 - (view) Author: Daniel Urban (daniel.urban) * (Python triager) Date: 2010-07-31 07:28
> P.S. Please let me know how to detect such issues in future patches.

In some editors there is an option "Remove trailing spaces" or something like that. Also, some editors (for example Kate) show trailing spaces.
msg114459 - (view) Author: Daniel Stutzbach (stutzbach) (Python committer) Date: 2010-08-20 21:40
The patch looks good to me, too.  The new tests fail without the fix, and pass with the fix.
msg114641 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-08-22 08:02
Fixed in r84252, r84252, and r84254.
History
Date User Action Args
2010-08-22 08:02:24rhettingersetstatus: open -> closed
priority: high -> normal
resolution: accepted -> fixed
messages: + msg114641
2010-08-20 21:40:56stutzbachsetassignee: stutzbach -> rhettinger
messages: + msg114459
stage: test needed -> patch review
2010-07-31 07:28:33daniel.urbansetmessages: + msg112117
2010-07-30 07:58:28eli.benderskysetfiles: + issue9214.2.patch

messages: + msg112046
2010-07-29 19:35:34daniel.urbansetnosy: + daniel.urban
messages: + msg111990
2010-07-26 04:27:47eli.benderskysetfiles: + issue9214.1.patch
keywords: + patch
messages: + msg111593
2010-07-16 15:39:18eli.benderskysetnosy: + eli.bendersky
2010-07-10 21:20:00eric.araujosetnosy: + eric.araujo
2010-07-10 21:19:52eric.araujosetresolution: accepted
2010-07-09 23:02:25rhettingersetpriority: normal -> high

messages: + msg109805
2010-07-09 20:31:54rhettingersetnosy: + rhettinger
2010-07-09 19:33:44stutzbachsetmessages: + msg109790
2010-07-09 19:32:32stutzbachsetmessages: + msg109789
2010-07-09 19:31:43stutzbachcreate