Skip to content

gh-145856: Fix plistlib.dumps() skipkeys behavior with mixed key types#150109

Open
ankhikarmakar wants to merge 3 commits into
python:mainfrom
ankhikarmakar:fix-145856-plistlib-skipkeys
Open

gh-145856: Fix plistlib.dumps() skipkeys behavior with mixed key types#150109
ankhikarmakar wants to merge 3 commits into
python:mainfrom
ankhikarmakar:fix-145856-plistlib-skipkeys

Conversation

@ankhikarmakar

Copy link
Copy Markdown
Contributor

When both skipkeys=True and sort_keys=True were passed to plistlib.dump() / plistlib.dumps(), the sort ran before non-string keys were filtered out, so a dictionary with mixed key types raised TypeError instead of dropping the non-string keys per the documented contract.

This PR filters non-string keys before sorting in all three write_dict sites — the XML writer (PlistWriter.write_dict) and both passes of the binary writer (_BinaryPlistWriter._flatten and _BinaryPlistWriter._write).

Test plan:

  • Extended test_skipkeys to cover sort_keys=True in addition to sort_keys=False.
  • Added test_skipkeys_with_sort_keys_mixed_types (the reproducer from the issue) — fails without the fix, passes with it, across all formats.
  • Existing test_plistlib (72 tests) still passes.

@python-cla-bot

python-cla-bot Bot commented May 19, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@ankhikarmakar ankhikarmakar force-pushed the fix-145856-plistlib-skipkeys branch from a516b1a to 9be8728 Compare May 19, 2026 20:28

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks reasonable & complete, and it even simplifies the implementation a bit.
Thank you!

@encukou encukou added the sprint label May 19, 2026
@github-project-automation github-project-automation Bot moved this to Todo in Sprint May 19, 2026
@github-project-automation github-project-automation Bot moved this from Todo to In Progress in Sprint May 19, 2026
@ankhikarmakar ankhikarmakar force-pushed the fix-145856-plistlib-skipkeys branch from 987b4d7 to 0fc8545 Compare May 19, 2026 21:04
@encukou

encukou commented May 20, 2026

Copy link
Copy Markdown
Member

Now it fails. Could you fix the test_keys_no_string test?

Also, please do not force-push to CPython PRs, it makes the PR hard to review.

…y types

When both skipkeys=True and sort_keys=True were passed to
plistlib.dump()/dumps(), the sort ran before the non-string keys were
filtered out, raising TypeError on dictionaries with mixed key types.
Filter non-string keys before sorting in all three write_dict sites
(XML, binary pass 1, binary pass 2).
@ankhikarmakar ankhikarmakar force-pushed the fix-145856-plistlib-skipkeys branch from ed2b54f to 9d970c8 Compare May 20, 2026 05:34
@ankhikarmakar

Copy link
Copy Markdown
Contributor Author

Reverted to the old commit. Please review @encukou .

@serhiy-storchaka

Copy link
Copy Markdown
Member

There was also equivalent PR #150109, opened a week earlier, which was closed to give the author of the issue chance to submit his own PR, which he didn't.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest to add @VanshAgarwal24036 as a co-author. He created an equivalent PR earlier.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at this again I see several issues.

  • We create a copy of items even if sort_keys is false. If the dict was huge, this can consume significant amount of memory. Perhaps we should make a copy only if it is needed for sorting.
  • We can get different error than "keys must be strings" if sort_keys is true, skipkeys is false, and the dict contains mixed keys. We should check types before sorting if skipkeys is false.

@bedevere-app

bedevere-app Bot commented Jun 6, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants