Skip to content

Conversation

@xFrednet
Copy link
Collaborator

@xFrednet xFrednet commented Jun 4, 2025

The check_freezable check used to check if the type of the given object is freezable. The is_freezable_builtin function allowed all PyType_Type instances to be frozen, which in turn allowed types with c extensions to be frozen. Freezing instances of such c types would fail. However, I believe this can lead to bugs, where instances of a sub c type would be allowed to be frozen.

I propose that check_freezable only does extensive checks on types, and assumes an object is freezable if the type is immutable.

Notice that this PR changes the semantics. The previous version would accept a type with C extensions to be frozen, but no instance of it. While this prevents the type itself from being frozen, which implicitly disallows any instances from being frozen. As an upside, we only run the potentially expensive checks on types and not on every object instance.

@matajoh Any thoughts

@xFrednet xFrednet force-pushed the immutable-throws branch from 6d3fa7b to 77a1f6a Compare June 5, 2025 07:39
@xFrednet
Copy link
Collaborator Author

xFrednet commented Jun 5, 2025

This PR was inspired by this example:

import sys
from immutable import freeze

undefined_name                       # Cause an exception
e = sys.last_value                   # Get the exception
be = e.__class__.__base__.__base__   # Get `BaseException`

freeze(e)

I wonder if this succeeded on my test branch due to some dirty state, because now it seems to behave the same on both implementations

@xFrednet xFrednet marked this pull request as draft June 5, 2025 08:30
@xFrednet
Copy link
Collaborator Author

xFrednet commented Jun 5, 2025

I noticed that this fix is also not complete -.-. The problem is, that this prevents the creation of safe wrapper types in the form of subtypes

@mjp41 mjp41 force-pushed the immutable-main branch 7 times, most recently from b7bd81c to 32152ca Compare June 11, 2025 12:26
@xFrednet xFrednet closed this Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants