Author josh.r
Recipients eric.snow, josh.r, kaniini, methane, python-dev, rhettinger, serhiy.storchaka, xiang.zhang, zach.ware
Date 2016-10-26.00:37:24
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1477442246.69.0.74608934898.issue27275@psf.upfronthosting.co.za>
In-reply-to
Content
The Python implementation of OrderedDict breaks for issue28014, at least on 3.4.3 (it doesn't raise KeyError, but if you check the repr, it's only showing one of the two entries, because calling __getitem__ is rearranging the OrderedDict).

>>> s = SimpleLRUCache(2)
>>> s['t1'] = 1
>>> s
SimpleLRUCache([('t1', 1)])
>>> s['t2'] = 2
>>> s
SimpleLRUCache([('t1', 1)])
>>> s
SimpleLRUCache([('t2', 2)])

Again, the OrderedDict code (in the Python case, __repr__, in the C case, popitem) assumes __getitem__ is idempotent, and again, the violation of that constraint makes things break. They break differently in the Python implementation and the C implementation, but they still break, because people are trying to force OrderedDict to do unnatural things without implementing their own logic to ensure their violations of the dict pseudo-contract actually works.

popitem happens to be a common cause of problems because it's logically a get and delete combined. People aren't using it for the get feature, it's just a convenient way to remove items from the end; if they bypassed getting and just deleted it would work, but it's a more awkward construction, so they don't. If they implemented their own popitem that avoided their own non-idempotent __getitem__, that would also work.

I'd be perfectly happy with making popitem implemented in terms of pop on subclasses when pop is overridden (if pop isn't overridden though, that's effectively what popitem already does).

I just don't think we should be making the decision that popitem *requires* inheritance for all dict subclasses that have (normal) idempotent __contains__ and __getitem__ because classes that violate the usual expectations of __contains__ and __getitem__ have (non-segfaulting) problems.

Note: In the expiring case, the fix is still "wrong" if someone used popitem for the intended purpose (to get and delete). The item popped might have expired an hour ago, but because the fixed code bypasses __getitem__, it will happily return the expired a long expired item (having bypassed expiration checking). It also breaks encapsulation, returning the expiry time that is supposed to be stripped on pop. By fixing one logic flaw on behalf of a fundamentally broken subclass, we introduced another.
History
Date User Action Args
2016-10-26 00:37:27josh.rsetrecipients: + josh.r, rhettinger, methane, python-dev, eric.snow, zach.ware, serhiy.storchaka, xiang.zhang, kaniini
2016-10-26 00:37:26josh.rsetmessageid: <1477442246.69.0.74608934898.issue27275@psf.upfronthosting.co.za>
2016-10-26 00:37:26josh.rlinkissue27275 messages
2016-10-26 00:37:24josh.rcreate