classification
Title: WeakSet cmp methods
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.0, Python 3.1
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: schuppenies Nosy List: benjamin.peterson, pitrou, rhettinger, schuppenies
Priority: normal Keywords: needs review, patch

Created on 2009-05-08 01:07 by schuppenies, last changed 2009-05-18 21:48 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
_weakrefset.patch schuppenies, 2009-05-11 16:30
_weakrefset-notimplemented.patch schuppenies, 2009-05-15 15:37
Messages (17)
msg87420 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-08 01:07
Running this code:

>>> import weakref
>>> class C: pass
...
>>> ws = weakref.WeakSet([C])
>>> if ws == 1:
...      print(1)
...

gives me the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    File "/home/bob/python/svn/py3k/Lib/_weakrefset.py", line 121, in __eq__
        return self.data == set(ref(item) for item in other)
        TypeError: 'int' object is not iterable

Looking at _weakrefset.py line 121 gives

  def __eq__(self, other):
      return self.data == set(ref(item) for item in other)

which treats any 'other' object as a set like object. Therefore
comparing WeakSet to a non-set-like object always fails.

Do I understand it correctly and if so, is this the intended behavior?
msg87546 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-10 20:58
I don't think it is the intended behaviour. Comparison for equality
should return either True or False (or perhaps NotImplemented?), not
raise a TypeError.
msg87560 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-11 02:08
Here is a patch which will return False instead of TypeError. This is
the same behavior a normal set has. Two things though.

1. I don't know wether the 'import _abcoll' statement somehow influences
the bootstrap in one way or the other, because 'from _abcoll import
Iterable' does.

2. The current WeakSet implementation returns True if a WeakSet is
compared to any Iterable which contains the same set of objects:

>>> import weakref
>>>
>>> class Foo: pass
...
>>> l = [Foo(), Foo(), Foo()]
>>> ws = weakref.WeakSet(l)
>>> ws == l
True

Not sure wether this is intended, since this is not the same behavior of
a normal set. If it is intended, I think it should be documented.
msg87575 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-11 14:18
> 2. The current WeakSet implementation returns True if a WeakSet is
> compared to any Iterable which contains the same set of objects:

Sounds bad. It should probably be fixed.
msg87577 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-11 16:30
Sounds right to me. Here is another patch plus tests.

Going through the other tests, I adapted two more tests to actually test
WeakSet. Also, I found the following one and think it is a copy&paste
from test_set which is not useful for test_weakset. Should it be removed
(as currently done in the patch)?

def test_set_literal(self):
    s = set([1,2,3])
    t = {1,2,3}
    self.assertEqual(s, t)
msg87578 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-11 16:33
> Going through the other tests, I adapted two more tests to actually test
> WeakSet. Also, I found the following one and think it is a copy&paste
> from test_set which is not useful for test_weakset. Should it be removed
> (as currently done in the patch)?

Absolutely!
msg87771 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-14 21:57
Is the current patch ready for consumption?
msg87777 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-14 22:30
The test passes on my machine, but a quick review would definitely be
nice :)
msg87780 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-05-14 23:23
Shouldn't the return value be NotImplemented instead of False?
msg87782 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-14 23:48
If that is the right behavior then yes. Is this documented somewhere?
msg87784 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-05-15 01:27
See http://docs.python.org/reference/datamodel.html#object.__eq__
msg87794 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-15 03:06
I am now a bit confused about the semantics of __eq__ for WeakSets. Is a
WeakSet only equal to another WeakSet with the same elements or to any
iterable with the same elements, e.g. list? Because this is how I read
the current implementation. If it is the latter, when should a
NotImplementedError be raised? Whenever the other object is not an Iterable?
msg87814 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 12:51
Why are you confused? You already asked that question earlier in the thread.
msg87818 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-15 15:37
Maybe because I take the doc too specfic. It says "A rich comparison
method may return the singleton NotImplemented if it does not implement
the operation for a given pair of arguments."

I see the type check of the 'other' object as an operation towards the
equal comparison, since it validates wether 'self' and 'other' can be
equal at all. If they are of a different type, then they cannot be
equal, thus the anwser to "Are 'self' and 'other' equal?" should be
False. This again, would mean an equal operation is implemented and
returning NotImplemented is not the right anwser.

But going through similar code snippets shows otherwise, so my
understanding must be lacking something. Therefore here is patch which
returns NotImplemented.
msg87819 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 15:45
> I see the type check of the 'other' object as an operation towards the
> equal comparison, since it validates wether 'self' and 'other' can be
> equal at all. If they are of a different type, then they cannot be
> equal, thus the anwser to "Are 'self' and 'other' equal?" should be
> False. This again, would mean an equal operation is implemented and
> returning NotImplemented is not the right anwser.

The explanation for NotImplemented is that an user-defined class may
decide it can compare equal to a WeakSet (without inheriting from
WeakSet). Returning NotImplemented from WeakSet.__eq__ gives a chance to
the user-defined class' __eq__ method to be called.
msg87821 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-15 16:42
You can commit the latest patch, provided all tests pass.
msg88011 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2009-05-18 00:18
Fixed in r72751.
History
Date User Action Args
2009-05-18 21:48:35pitrousetresolution: accepted -> fixed
2009-05-18 00:18:29schuppeniessetstatus: open -> closed

messages: + msg88011
2009-05-15 16:42:04pitrousetassignee: schuppenies
resolution: accepted
messages: + msg87821
stage: patch review -> commit review
2009-05-15 15:45:44pitrousetmessages: + msg87819
2009-05-15 15:37:18schuppeniessetfiles: + _weakrefset-notimplemented.patch

messages: + msg87818
2009-05-15 12:51:41pitrousetmessages: + msg87814
2009-05-15 03:06:51schuppeniessetmessages: + msg87794
2009-05-15 01:27:08benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg87784
2009-05-14 23:48:22schuppeniessetmessages: + msg87782
2009-05-14 23:23:21rhettingersetnosy: + rhettinger
messages: + msg87780
2009-05-14 22:30:40schuppeniessetmessages: + msg87777
2009-05-14 21:57:39pitrousetmessages: + msg87771
2009-05-13 02:04:59schuppeniessetstage: needs patch -> patch review
2009-05-11 16:33:32pitrousetmessages: + msg87578
2009-05-11 16:30:56schuppeniessetfiles: - _weakrefset.patch
2009-05-11 16:30:44schuppeniessetfiles: + _weakrefset.patch

messages: + msg87577
2009-05-11 14:18:07pitrousetmessages: + msg87575
2009-05-11 02:08:32schuppeniessetkeywords: + needs review, patch
files: + _weakrefset.patch
messages: + msg87560
2009-05-10 20:58:03pitrousetpriority: normal

nosy: + pitrou
messages: + msg87546

stage: needs patch
2009-05-08 01:07:21schuppeniescreate