Fix check_freezable only checking object types#58
Closed
xFrednet wants to merge 3 commits intoimmutable-mainfrom
Closed
Fix check_freezable only checking object types#58xFrednet wants to merge 3 commits intoimmutable-mainfrom
check_freezable only checking object types#58xFrednet wants to merge 3 commits intoimmutable-mainfrom
Conversation
6d3fa7b to
77a1f6a
Compare
Collaborator
Author
|
This PR was inspired by this example: 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 |
Collaborator
Author
|
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 |
b7bd81c to
32152ca
Compare
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.
The
check_freezablecheck used to check if the type of the given object is freezable. Theis_freezable_builtinfunction allowed allPyType_Typeinstances 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_freezableonly 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