- Notifications
You must be signed in to change notification settings - Fork1.3k
Update to pass test for unhashable collections#4640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vm/src/builtins/list.rs Outdated
| context | ||
| .types | ||
| .list_type | ||
| .slots | ||
| .hash | ||
| .store(Some(unhashable_wrapper)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe there is a more elegant way to do this...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And I missed adding unhashable_wrapper thing for deque, so test_deque fails. Deque implementation resides in collections.rs, but there is no type initialization function similar to thisinit function incollections.rs. Where is a good place to put this code?
I will look in the details. |
vm/src/builtins/bytearray.rs Outdated
| context | ||
| .types | ||
| .bytearray_type | ||
| .slots | ||
| .hash | ||
| .store(Some(unhashable_wrapper)); |
youknowoneMar 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have a suggestion about the location of this initialization.
We put these information toPyClassDef for builtin types.
addingHASHABLE const with default value false will be good.
Then we use the information during making class. that'sPyClassImpl::make_slots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, I'll try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe default value is false, not true. hashable is common in builtin type world, but not outside.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a few more style suggestions.
vm/src/class.rs Outdated
| .map(|h| h as usize == unhashable_wrapper as usize) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| .map(|h| hasusize == unhashable_wrapperasusize) | |
| .unwrap_or(false) | |
| .map_or(0, |h| hasusize) == unhashable_wrapperasusize |
the cost ofmap_or(0, |h| h as usize) is zero here due to its layout.
vm/src/class.rs Outdated
| @@ -112,6 +112,16 @@ pub trait PyClassImpl: PyClassDef { | |||
| .into(); | |||
| class.set_attr(identifier!(ctx, __new__), bound); | |||
| } | |||
| if class | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to check if this is related to#3460 later
vm/src/types/slot.rs Outdated
| .map(|a| a.is(&ctx.none())) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| .map(|a| a.is(&ctx.none())) | |
| .unwrap_or(false); | |
| .map_or(false, |a| a.is(&ctx.none)); |
ctx.none() increase reference count ofNone. We don't need to do.
vm/src/types/slot.rs Outdated
| if is_unhashable { | ||
| toggle_slot!(hash, unhashable_wrapper); | ||
| } else { | ||
| toggle_slot!(hash, hash_wrapper); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if is_unhashable{ | |
| toggle_slot!(hash, unhashable_wrapper); | |
| }else{ | |
| toggle_slot!(hash, hash_wrapper); | |
| } | |
| let wrapper =if is_unhashable{ | |
| unhashable_wrapper | |
| }else{ | |
| hash_wrapper | |
| }; | |
| toggle_slot!(wrapper) |
vm/src/types/slot.rs Outdated
| @@ -243,6 +243,11 @@ fn hash_wrapper(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> { | |||
| } | |||
| } | |||
| /// Marks a type as unhashable. Similar to PyObject_HashNotImplemented in CPython | |||
| pub fn unhashable_wrapper(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| pubfnunhashable_wrapper(zelf:&PyObject,vm:&VirtualMachine) ->PyResult<PyHash>{ | |
| pubfnhash_unhashable(zelf:&PyObject,vm:&VirtualMachine) ->PyResult<PyHash>{ |
Other functions are called wrapper because they are wrapping a python call.
This function is not a wrapper of python call, so giving a different name would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, probablyhash_not_implemented by following cpython naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great! it now passes deque too.
vm/src/class.rs Outdated
| @@ -60,6 +60,7 @@ pub trait PyClassDef { | |||
| const TP_NAME: &'static str; | |||
| const DOC: Option<&'static str> = None; | |||
| const BASICSIZE: usize; | |||
| const UNHASHABLE: bool; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| constUNHASHABLE:bool; | |
| constUNHASHABLE:bool =false; |
default value can be placed here
vm/src/class.rs Outdated
| @@ -71,6 +72,7 @@ where | |||
| const TP_NAME: &'static str = T::TP_NAME; | |||
| const DOC: Option<&'static str> = T::DOC; | |||
| const BASICSIZE: usize = T::BASICSIZE; | |||
| const UNHASHABLE: bool = false; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PyRef<T> must followsT
| constUNHASHABLE:bool =false; | |
| constUNHASHABLE:bool =T::UNHASHABLE; |
19fe761 toccded70Compareccded70 toe5b338eCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great, thank you for digging in this tricky issue.
Uh oh!
There was an error while loading.Please reload this page.
In CPython:
PyObject_HashNotImplementedis assigned to hash slot), then__hash__attribute of the type dict is set to None so that the type is considered as unhashable on Python side too."__hash__": Noneis given as the type dict (liketype('C', (object,), {'__hash__': None})), then the hash slot is filled with PyObject_HashNotImplemented.In RustPython, Unhashable trait exists to mark a type as unhashble. But there is no way to check if a type implements Unhashable trait at runtime, so it is hard to follow the CPython's mechanism.
So I've added
unhashable_wrapperfunction similar to PyObject_HashNotImplemented, and used that as a marker for unhashable on the native side.Thanks for the advice from@youknowone !