Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-132762: Fix underallocation bug in dict.fromkeys() and expand test coverage#133627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
previously covered: - fast path for dictionary inputs - fast path when object's constructor returns non-empty dict (too small for good coverage)now additionally covered: - fast path for set inputs - slow path for non-set, non-dictionary inputs - fast path when object's constructor returns *large* non-empty dict - slow path when object is a proper subclass of dict
python-cla-botbot commentedMay 8, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, lgtm!
421ba58 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@angela-tarantula for the PR, and@colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…h-133627)The function `dict_set_fromkeys()` adds elements of a set to an existingdictionary. The size of the expanded dictionary was estimated with`PySet_GET_SIZE(iterable)`, which did not take into account the size of theexisting dictionary.(cherry picked from commit421ba58)Co-authored-by: Angela Liss <59097311+angela-tarantula@users.noreply.github.com>
Sorry,@angela-tarantula and@colesbury, I could not cleanly backport this to |
GH-133685 is a backport of this pull request to the3.14 branch. |
…ythongh-133627)The function `dict_set_fromkeys()` adds elements of a set to an existingdictionary. The size of the expanded dictionary was estimated with`PySet_GET_SIZE(iterable)`, which did not take into account the size of theexisting dictionary.(cherry picked from commit421ba58)Co-authored-by: Angela Liss <59097311+angela-tarantula@users.noreply.github.com>
GH-133686 is a backport of this pull request to the3.13 branch. |
) (gh-133685)The function `dict_set_fromkeys()` adds elements of a set to an existingdictionary. The size of the expanded dictionary was estimated with`PySet_GET_SIZE(iterable)`, which did not take into account the size of theexisting dictionary.(cherry picked from commit421ba58)Co-authored-by: Angela Liss <59097311+angela-tarantula@users.noreply.github.com>
) (gh-133686)The function `dict_set_fromkeys()` adds elements of a set to an existingdictionary. The size of the expanded dictionary was estimated with`PySet_GET_SIZE(iterable)`, which did not take into account the size of theexisting dictionary.(cherry picked from commit421ba58)Co-authored-by: Angela Liss <59097311+angela-tarantula@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Closes#132762
Summary
dict_set_fromkeys()was only sizing its new table by the size of theiterableinput, ignoring any existing entries in the dictionary. This triggered an infinite loop indictresize()whenever the new dictionary size was too small to reinsert those entries. This patch adds the samePy_MAX(…, DK_LOG_SIZE(mp->ma_keys))guard thatdict_dict_fromkeys()uses, so we never accidentally shrink the table below its current capacity. The relevant test casebaddict3has been updated to cover this edge case.For more background, see myproposal.
3 New Regression Tests
Ever since thefast path wasupdated, the slow path completely lost test coverage. To rectify this, I added 3 new tests:
iterableinput is neither a set nor a dictionaryfromkeys()is called on a proper subclass of dict,baddict4dict_dict_fromkeys()anddict_set_fromkeys()are separate)Thanks for the review!@DinoV@colesbury