python / cpython

The Python programming language
https://www.python.org
Other
63.18k stars 30.25k forks source link

Weakproxy is an instance of collections.Iterator #68255

Open 80be0bab-bcfb-49f0-8bad-3ceaf350fc94 opened 9 years ago

80be0bab-bcfb-49f0-8bad-3ceaf350fc94 commented 9 years ago
BPO 24067
Nosy @gvanrossum, @freddrake, @pitrou, @benjaminp, @gareth-rees

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: ```python assignee = None closed_at = None created_at = labels = ['type-bug', 'library'] title = 'Weakproxy is an instance of collections.Iterator' updated_at = user = 'https://bugs.python.org/ereuveni' ``` bugs.python.org fields: ```python activity = actor = 'fdrake' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'ereuveni' dependencies = [] files = [] hgrepos = [] issue_num = 24067 keywords = [] message_count = 13.0 messages = ['242159', '242172', '242174', '242175', '242177', '242178', '242179', '242180', '242181', '242182', '242184', '242186', '242187'] nosy_count = 6.0 nosy_names = ['gvanrossum', 'fdrake', 'pitrou', 'benjamin.peterson', 'gdr@garethrees.org', 'ereuveni'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue24067' versions = ['Python 2.7', 'Python 3.4', 'Python 3.5'] ```

80be0bab-bcfb-49f0-8bad-3ceaf350fc94 commented 9 years ago

Calling weakref.proxy() on an object returns something that is an instance of collections.Iterator, regardless of whether the object is an instance of collections.Iterator or even if the object itself is iterable.

To reproduce, run the following code (verified in python2.7 and 3.4 on my Mac):

import collections
import weakref

class NotIterator:
    pass

not_iterator = NotIterator()
isinstance(not_iterator, collections.Iterator) # returns False

proxy_object = weakref.proxy(not_iterator)
isinstance(proxy_object, collections.Iterator) # returns True, should be False
faa20c90-fcf0-43f4-810b-00286077d549 commented 9 years ago

Not just Iterator, but Container, Hashable, Iterable, and Sized too!

    >>> import weakref
    >>> class C: pass
    >>> o = C()
    >>> w = weakref.proxy(o)
    >>> from collections.abc import *
    >>> isinstance(w, Container)
    True
    >>> isinstance(w, Hashable)
    True
    >>> isinstance(w, Iterable)
    True
    >>> isinstance(w, Sized)
    True
pitrou commented 9 years ago

Those methods are defined precisely so that they can be delegated. Since virtually anything can be proxied, the weakref.proxy *class* cannot tell upfront whether the proxied object will be an Iterator or not. I don't really see how to get around that.

faa20c90-fcf0-43f4-810b-00286077d549 commented 9 years ago

Hashable is particularly misleading, because "weakref.Proxy objects are not hashable regardless of the referent".

pitrou commented 9 years ago

weakref.Proxy objects are not hashable regardless of the referent

Interesting. @fdrake, do you remember why that is?

faa20c90-fcf0-43f4-810b-00286077d549 commented 9 years ago

The documentation says that weakref.Proxy objects are not hashable because "this avoids a number of problems related to their fundamentally mutable nature, and prevent their use as dictionary keys".

Hashable objects must be immutable, otherwise the hash might change, invalidating the invariants that make dictionaries work, but Proxy objects are fundamentally mutable: when there are no more strong references to the proxied object, the object gets destroyed and the Proxy object now refers to None. If the Proxy object were hashable then its hash would change at this point.

pitrou commented 9 years ago

Still, weakref.ref objects are hashable, so I don't understand why weakref.proxy would be different.

Hashability of weakref.ref has been added in 29aa832b8787, to support weak dictionaries. See bpo-403985. It simply wasn't discussed whether to also add hashability to proxy objects.

pitrou commented 9 years ago

Ok, PEP-205 explains it a bit more:

"the resulting proxy cannot be used as a dictionary key since it cannot be compared once the referent has expired, and comparability is necessary for dictionary keys. Operations on proxy objects after the referent dies cause weakref.ReferenceError to be raised in most cases."

Perhaps this can be relaxed, and comparison simply made to return false. The following behaviour is a bit troubling :-)

>>> p
<weakproxy at 0x7f4054fe8cd8 to NoneType at 0x88a780>
>>> p == p
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ReferenceError: weakly-referenced object no longer exists
freddrake commented 9 years ago

I don't see any reason for proxy objects to be less hashable than ref objects.

As for the p == p case, where the referent has expired, returning True if p is p seems acceptable (along with False inequalities, and True for other comparisons allowing equality), but anything beyond that seems unwise. Not sure whether that would really be enough to help real use cases.

faa20c90-fcf0-43f4-810b-00286077d549 commented 9 years ago

I don't see any reason for proxy objects to be less hashable than ref objects.

The difference is that unlike a ref object, a proxy object is supposed to forward its method calls to the proxied object. So consider what happens if you forward the __hash__ method to the proxied object: the hash will change when the object dies.

A proxy object could, of course, not forward the __hash__ method, instead computing its own hash. But I think this would do more harm than good: surely most attempts to store weakref.Proxy objects in sets or dictionaries are going to be mistakes -- the user should have used a WeakKeyDictionary or a WeakSet instead.

freddrake commented 9 years ago

Clearly I've been away from this code for a long time.

The hash support for ref objects is definitely a very special case, only intended to support WeakKeyDictionary. We that class implemented in C, we'd probably want the hash support for refs not to be exposed.

You've convinced me hashability for proxies isn't desirable. Let's stick with the status quo on this.

pitrou commented 9 years ago

So consider what happens if you forward the __hash__ method to the proxied object: the hash will change when the object dies.

ref objects behave differently: they inherit their referent's hash value when alive, and remember it. proxy objects could be made to behave the same way.

The hash support for ref objects is definitely a very special case, only intended to support WeakKeyDictionary

I've relied several times on the hashability of ref objects, in third-party code. (OTOH, I never use weakref proxies)

freddrake commented 9 years ago

ref objects behave differently: they inherit their referent's hash value when alive, and remember it. proxy objects could be made to behave the same way.

They could, yes, but that would break the proxy behavior, and the hash \<--> equality behavior for mutable objects.

In particular, mutable objects can become equal; if the hashes were computed for the proxies before that happened, the hashes would be inappropriate later. That's pretty important.