Skip to content

gh-160147: Add PyFrozenSetWriter C API - #160154

Open
vstinner wants to merge 1 commit into
python:mainfrom
vstinner:frozenset_writer
Open

vstinner wants to merge 1 commit into
python:mainfrom
vstinner:frozenset_writer

Conversation

@vstinner

@vstinner vstinner commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Use the new API in:

  • Modules/_abc.c
  • Modules/_asynciomodule.c
  • Modules/_hashopenssl.c
  • Modules/_ssl.c

Use the new API in:

* Modules/_abc.c
* Modules/_asynciomodule.c
* Modules/_hashopenssl.c
* Modules/_ssl.c
@read-the-docs-community

Copy link
Copy Markdown

Documentation build overview

📚 cpython-previews | 🛠️ Build #35312580 | 📁 Comparing dd64229 against main (c72b435)

  🔍 Preview build  

3 files changed
± c-api/set.html
± whatsnew/3.16.html
± whatsnew/changelog.html

@vstinner vstinner changed the title gh-158660: Add PyFrozenSetWriter C API gh-160147: Add PyFrozenSetWriter C API Oct 22, 2026
@bedevere-app bedevere-app Bot added the type-feature A feature request or enhancement label Oct 22, 2026
@vstinner

Copy link
Copy Markdown
Member Author

Updating Modules/marshal.c to use PyFrozenSetWriter is more complicated, I prefer to do it in a follow-up PR.

Comment thread Objects/setobject.c
Comment on lines +3278 to +3288
// Now that the frozenset is fully initialized, it's safe to track it in
// the GC. Track the frozenset if at least one item is tracked by the GC.
Py_ssize_t pos = 0;
setentry *entry;
while (set_next(frozenset, &pos, &entry)) {
PyObject *item = entry->key;
if (_PyObject_IS_GC(item) && _PyObject_GC_IS_TRACKED(item)) {
_PyObject_GC_TRACK(frozenset);
break;
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe we can skip iterating on the items and just always track the frozenset in the GC. Most set functions already do that: always call _PyObject_GC_TRACK() without checking items.

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 am +1 on skipping.

@vstinner

Copy link
Copy Markdown
Member Author

cc @corona10

@vstinner

Copy link
Copy Markdown
Member Author

This C API is just an abstraction on top of existing PySet functions. PyFrozenSetWriter_Create() doesn't allocate a PyFrozenSetWriter structure. In fact, it's just a PySetObject* pointer! It is the same design than the PyLongWriter API.

@corona10 corona10 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.

Overall lgtm.

@rhettinger

Copy link
Copy Markdown
Contributor

The edits to Modules/_abc.c, Modules/_asynciomodule.c, Modules/_hashopenssl.c, and Modules/_ssl.c all add more code than they replace. I don't see how the new API adds value.

@vstinner

Copy link
Copy Markdown
Member Author

@rhettinger:

I don't see how the new API adds value.

There multiple advantages of this new API.

The current PySet_Add() API requires that there is exactly a single reference to the frozenset, otherwise it fails with SystemError. The frozenset can leak to other C functions or even to Python before it's fully initialized. The proposed Writer API doesn't have this issue.

The internal frozenset is not tracked by the GC until Finish() is called (when the frozenset is fully initialized). So it's not possible to observe a frozenset being modified in Python (which must not be possible, frozenset are immutable, right?). See for example #159730 for recent examples of fixed bugs.

Also, I think that's a bad design to have a PySet_Add() function to modify an immutable (frozenset) object. We should avoid such API. So I propose deprecating calling PySet_Add() on a frozenset (replaced with this new Writer API). See issues:

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

Labels

awaiting merge type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants