Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes older immutability/freezing “technical debt” by consolidating the “freezable” mechanism around set_freezable() storage (attribute / ob_flags), dropping the old register-based approach, and renaming the public query API from isfrozen to is_frozen (with a compatibility alias in Lib/immutable.py).
Changes:
- Remove
_PyImmutability_RegisterFreezable/_immutable.register_freezableand the associated weakref/set bookkeeping. - Rename
_immutable.isfrozen→_immutable.is_frozen, and update the Python-facing API + tests accordingly. - Initialize built-in freezable/shallow-immutable types via
_PyImmutability_SetFreezable/_PyImmutability_RegisterShallowImmutable.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Python/pystate.c | Removes interpreter cleanup of the deleted freezable-types state. |
| Python/immutability.c | Refactors immutability state init and freezable bookkeeping; updates traversal behavior and SetFreezable semantics. |
| Modules/clinic/_immutablemodule.c.h | Regenerates clinic output: drops register_freezable, renames isfrozen → is_frozen. |
| Modules/arraymodule.c | Switches module type registration from _RegisterFreezable to _SetFreezable. |
| Modules/_test_reachable.c | Switches test types to _SetFreezable. |
| Modules/_struct.c | Switches module type registration to _SetFreezable. |
| Modules/_immutablemodule.c | Drops register_freezable and NotFreezable export; renames isfrozen → is_frozen. |
| Modules/_elementtree.c | Switches module type registration to _SetFreezable. |
| Modules/_decimal/_decimal.c | Switches decimal types to _SetFreezable. |
| Modules/_datetimemodule.c | Switches datetime C-API types to _SetFreezable. |
| Modules/_ctypes/_ctypes.c | Switches ctypes type registration macro to _SetFreezable. |
| Modules/_collectionsmodule.c | Switches collections types to _SetFreezable. |
| Lib/test/test_freeze/test_weakref.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_set_freezable.py | Updates imports/usages and removes reliance on _immutable.register_freezable. |
| Lib/test/test_freeze/test_rollback.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_reachable.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_prefreeze.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_multi_freeze.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_module_proxy.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_implicit.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_decorators.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_ctypes.py | Updates imports/usages to is_frozen. |
| Lib/test/test_freeze/test_core.py | Removes NotFreezable-based tests; updates to is_frozen. |
| Lib/test/test_freeze/test_common.py | Adjusts common test helpers to drop NotFreezable and use is_frozen. |
| Lib/immutable.py | Renames exported query API to is_frozen, keeps runtime alias isfrozen. |
| Include/internal/pycore_immutability.h | Removes freezable_types and moves enum out; leaves immutability state shape updated. |
| Include/cpython/immutability.h | Moves _Py_freezable_status enum here; updates _PyImmutability_SetFreezable signature. |
Comments suppressed due to low confidence (1)
Lib/immutable.py:60
- all no longer includes the backward-compatible alias
isfrozeneven though the module still defines it. If the intent is to keepisfrozenworking “for now”, consider including it in all (and potentially documenting/deprecating it) sofrom immutable import *remains compatible during the transition.
__all__ = [
"freeze",
"is_frozen",
"set_freezable",
"NotFreezableError",
"ImmutableModule",
"FREEZABLE_YES",
"FREEZABLE_NO",
"FREEZABLE_EXPLICIT",
"FREEZABLE_PROXY",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+1509
to
1521
| assert(PyErr_Occurred() == NULL); | ||
| int rc = PyObject_SetAttr(obj, &_Py_ID(__freezable__), value); | ||
| Py_DECREF(value); | ||
| if (rc == 0) { | ||
| return 0; | ||
| } else { | ||
| // Attempting to set the attribute can cause different errors | ||
| // depending on the type implementation. We just clear it here | ||
| // and store the freezability in the object fields instead. | ||
| // This should be safe, since `PyErr` should never be set when | ||
| // this function is called. | ||
| PyErr_Clear(); | ||
| } |
Comment on lines
101
to
103
| state->warned_types = _Py_hashtable_new( | ||
| _Py_hashtable_hash_ptr, | ||
| _Py_hashtable_compare_direct); |
Comment on lines
+155
to
+165
| &PyClassMethod_Type, // TODO(Immutable): mjp I added this, is it correct? Discuss with maj | ||
| &PyClassMethodDescr_Type, | ||
| &PyStaticMethod_Type, | ||
| &PyMethod_Type, | ||
| &PyCapsule_Type, | ||
| &PyCode_Type, | ||
| &PyCell_Type, | ||
| &PyFrame_Type, | ||
| &_PyWeakref_RefType, | ||
| &PyModule_Type, // TODO(Immutable): mjp I added this, is it correct? Discuss with maj | ||
| &_PyImmModule_Type, |
Comment on lines
+34
to
+35
| if self.obj == None: | ||
| return |
Comment on lines
13
to
17
| struct _Py_immutability_state { | ||
| int late_init_done; | ||
| PyObject *freezable_types; | ||
| _Py_hashtable_t *shallow_immutable_types; | ||
| PyObject *destroy_cb; | ||
| _Py_hashtable_t *warned_types; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removing some old technical debt.