We hit a fascinating regression in Werkzeug today and you’ll never believe the fix!
This really seems like a gotcha (if not an outright bug?) in the language. It seems like an object’s behavior should remain exactly the same when a subclass implements a method that merely delegates to super()’s implementation. Fast-path optimizations should not cause entirely different results.
Would anyone with expertise in dict internals be able to chime in on this?
Thanks!
Josh
Would you like to summarise the issue and the fix, rather than expect us
all to read through the issue’s history? I’ve read the four comments and
the diffs and I don’t understand what is being said or the context.
The PR seems to be saying that it is necessary to add an __iter__
method which merely calls super, in order to fix a bug in the subclass;
but the conversation seem to be saying that adding that call to super
causes a difference in behaviour. It refers to dedicated unit tests,
but I cannot see them in the PR.
So I’m confused.
Can you provide a minimal reproducible example?
Maybe, relating to this issue.
https://bugs.python.org/issue34320
Especially, see this discussion.
From old days, dict subclasses used the “fast-path” always. To fix the issue, dict subclasses use the “fast-path” when they don’t override __iter__.
Maybe, we need to test __getitem__ too, or use fast-path only for exact dict, not subclasses.
But I don’t know why this is regression. Dict subclasses which dont override __iter__ use fast-path before/after the fix. So there is no behavior change for them.
Thanks for taking a look at this!
Please see https://repl.it/@jab/dict-subclass-copy-surprise for a more minimal reproduction of this issue (pasted below as well). If you run as-is, you’ll see the unexpected behavior trigger an AssertionError. If you change SHOW_BUG to False, you won’t. Is it intended that toggling the value of SHOW_BUG in this code causes different results?
Thanks,
Josh
class Parent:
def __init__(self, value):
self._value = value
def method(self):
return self._value
class Child1(Parent):
pass
c1 = Child1(42)
result = c1.method()
assert result == 42, result
class Child2(Parent):
def method(self):
return super().method()
c2 = Child2(42)
result = c2.method()
assert result == 42, result
# You might think that for all "Parent" classes,
# for all "method"s, Child1.method should return
# the same result as Child2.method.
# But when "Parent" is "dict" and method is "__iter__",
# that is not the case:
SHOW_BUG = True
class ChildDict1(dict):
"""Simplification of werkzeug.datastructures.MultiDict."""
def __init__(self):
pass
if not SHOW_BUG:
def __iter__(self):
return super().__iter__()
def add(self, key, value):
self.setdefault(key, []).append(value)
def __setitem__(self, key, value):
"""Like add, but removes any existing key first."""
super().__setitem__(key, [value])
def getall(self, key) -> list:
return super().__getitem__(key)
def __getitem__(self, key):
"""Return the first value for this key."""
return self.getall(key)[0]
def items(self, multi=False):
for (key, values) in super().items():
if multi:
yield from ((key, value) for value in values)
else:
yield key, values[0]
def values(self):
return (values[0] for values in super().values())
# Remaining overridden implementations of methods
# inherited from dict are elided for brevity.
cd1 = ChildDict1()
assert dict(cd1) == {}
cd1[1] = "one"
cd1.add(1, "uno")
assert cd1.getall(1) == ["one", "uno"]
assert list(cd1.items()) == [(1, "one")]
assert list(cd1.values()) == [ "one"]
assert dict(cd1) == {1: "one"}, cd1 # this line triggers the bug
I don’t know why you couldn’t see the unit tests in the PR, they’ve been there from the beginning.
I’m pasting the current contents of https://patch-diff.githubusercontent.com/raw/pallets/werkzeug/pull/2043.diff below.
diff --git a/src/werkzeug/datastructures.py b/src/werkzeug/datastructures.py
index 8ff359744..0625bf528 100644
--- a/src/werkzeug/datastructures.py
+++ b/src/werkzeug/datastructures.py
@@ -355,6 +355,15 @@ def __setstate__(self, value):
dict.clear(self)
dict.update(self, value)
+ def __iter__(self):
+ """This looks like a no-op, but actually preserves required interop.
+
+ See https://github.com/pallets/werkzeug/pull/2043.
+ """
+ return dict.__iter__(self)
+ # return super().__iter__() also works here, which makes this look
+ # even more like it should be a no-op, yet actually changes behavior.
+
def __getitem__(self, key):
"""Return the first data value for this key;
raises KeyError if not found.
diff --git a/tests/test_datastructures.py b/tests/test_datastructures.py
index 7249faa8f..a69143611 100644
--- a/tests/test_datastructures.py
+++ b/tests/test_datastructures.py
@@ -70,6 +70,12 @@ def create_instance(module=None):
ud[b"newkey"] = b"bla"
assert ud != d
+ def test_multidict_dict_interop(self):
+ md = self.storage_class([("a", 1), ("a", 2)])
+ assert {**md}["a"] != [1, 2]
+ assert {**md}["a"] == 1
+ assert {**md} == dict(md) == {"a": 1}
+
def test_basic_interface(self):
md = self.storage_class()
assert isinstance(md, dict)
Hope this helps, and thanks again for taking a look!
Since you said it is a regression, I expect old Python versions works as expected but only recent Python versions have surprising behavior?
Which version worked as expected? Python 2?
The “regression” refers to Werkzeug, not Python, and occurs in the Werkzeug v2 release candidate. The issue did not occur in the stable releases of Werkzeug because the data structures that subclassed dict had their own implementation of __iter__.