Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Lib/test/test_defaultdict.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,5 +204,20 @@ def default_factory():
self.assertEqual(test_dict[key], 2)
self.assertEqual(count, 2)

def test_repr_recursive_factory(self):
# gh-145492: defaultdict.__repr__ should not cause infinite recursion
# when the factory's __repr__ calls repr() on the defaultdict.
class ProblematicFactory:
def __call__(self):
return {}
def __repr__(self):
repr(dd)
return "ProblematicFactory()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I am not too late, but this test never triggers an infinite recursion in python 3.14 without the fix.
I suggest replacing this line with return f"ProblematicFactory for {repr(dd)}" as reported in the @KowalskiThomas issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Thanks for the catch! I though I tested, but apparently I used the wrong code :/

@mvanhorn, why did you change this part of the reproducer?

@KowalskiThomas, now I regret not asking you to send your branch as a PR. Sorry! :(
If you want to get it over the finish line (alas, without the start), now's your chance. Or I can send a fix-up PR myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch by @YvesDup - I didn't realize I was weakening the reproducer. I've submitted a fix-up PR at #145761 that uses return f"ProblematicFactory for {repr(dd)}" to match the original reproducer. Happy to close it if @KowalskiThomas wants to pick it up instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, go ahead @KowalskiThomas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #145788 which should fix the regression test to properly fail!


dd = defaultdict(ProblematicFactory())
# Should not raise RecursionError
r = repr(dd)
self.assertIn('ProblematicFactory()', r)

if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix infinite recursion in :class:`collections.defaultdict` ``__repr__``
when a ``defaultdict`` contains itself. Based on analysis by KowalskiThomas
in :gh:`145492`.
5 changes: 3 additions & 2 deletions Modules/_collectionsmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -2385,9 +2385,10 @@ defdict_repr(PyObject *op)
}
defrepr = PyUnicode_FromString("...");
}
else
else {
defrepr = PyObject_Repr(dd->default_factory);
Py_ReprLeave(dd->default_factory);
Py_ReprLeave(dd->default_factory);
}
}
if (defrepr == NULL) {
Py_DECREF(baserepr);
Expand Down
Loading