Update to pass test for unhashable collections #4640

seungha-kim · 2023-03-05T14:25:53Z

In CPython:

If a type is marked as unhashable on the native side (i.e. PyObject_HashNotImplemented is 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.
If a type is created at runtime and "__hash__": None is given as the type dict (like type('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_wrapper function similar to PyObject_HashNotImplemented, and used that as a marker for unhashable on the native side.

Thanks for the advice from @youknowone !

seungha-kim · 2023-03-05T14:27:19Z

vm/src/builtins/list.rs

+    context
+        .types
+        .list_type
+        .slots
+        .hash
+        .store(Some(unhashable_wrapper));


Maybe there is a more elegant way to do this...?

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 this init function in collections.rs. Where is a good place to put this code?

youknowone · 2023-03-05T15:18:39Z

I will look in the details.
The single failing test is this one:

======================================================================
FAIL: test_hash (test.test_deque.TestBasic.test_hash)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/RustPython/RustPython/pylib/Lib/test/test_deque.py", line 537, in test_hash
    self.assertRaises(TypeError, hash, deque('abc'))
AssertionError: TypeError not raised by hash

youknowone · 2023-03-05T16:04:12Z

vm/src/builtins/bytearray.rs

+    context
+        .types
+        .bytearray_type
+        .slots
+        .hash
+        .store(Some(unhashable_wrapper));


I have a suggestion about the location of this initialization.

We put these information to PyClassDef for builtin types.
adding HASHABLE const with default value false will be good.

Then we use the information during making class. that's PyClassImpl::make_slots.

Thanks, I'll try.

maybe default value is false, not true. hashable is common in builtin type world, but not outside.

vm/src/class.rs

youknowone

I added a few more style suggestions.

youknowone · 2023-03-06T08:53:11Z

vm/src/class.rs

+            .map(|h| h as usize == unhashable_wrapper as usize)
+            .unwrap_or(false)


Suggested change

.map(|h| h as usize == unhashable_wrapper as usize)

.unwrap_or(false)

.map_or(0, |h| h as usize) == unhashable_wrapper as usize

the cost of map_or(0, |h| h as usize) is zero here due to its layout.

youknowone · 2023-03-06T08:53:11Z

vm/src/class.rs

@@ -112,6 +112,16 @@ pub trait PyClassImpl: PyClassDef {
                    .into();
            class.set_attr(identifier!(ctx, __new__), bound);
        }
+
+        if class


I'd like to check if this is related to #3460 later

youknowone · 2023-03-06T08:53:11Z

vm/src/types/slot.rs

+                    .map(|a| a.is(&ctx.none()))
+                    .unwrap_or(false);


Suggested change

.map(|a| a.is(&ctx.none()))

.unwrap_or(false);

.map_or(false, |a| a.is(&ctx.none));

ctx.none() increase reference count of None. We don't need to do.

youknowone · 2023-03-06T08:53:11Z

vm/src/types/slot.rs

+                if is_unhashable {
+                    toggle_slot!(hash, unhashable_wrapper);
+                } else {
+                    toggle_slot!(hash, hash_wrapper);
+                }


Suggested change

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)

youknowone · 2023-03-06T10:31:03Z

vm/src/types/slot.rs

@@ -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> {


Suggested change

pub fn unhashable_wrapper(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> {

pub fn hash_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.

Oh, probably hash_not_implemented by following cpython naming

youknowone

Great! it now passes deque too.

youknowone · 2023-03-07T03:03:31Z

vm/src/class.rs

@@ -60,6 +60,7 @@ pub trait PyClassDef {
    const TP_NAME: &'static str;
    const DOC: Option<&'static str> = None;
    const BASICSIZE: usize;
+    const UNHASHABLE: bool;


Suggested change

const UNHASHABLE: bool;

const UNHASHABLE: bool = false;

default value can be placed here

youknowone · 2023-03-07T03:03:31Z

vm/src/class.rs

@@ -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;


PyRef<T> must follows T

Suggested change

const UNHASHABLE: bool = false;

const UNHASHABLE: bool = T::UNHASHABLE;

youknowone

Looks great, thank you for digging in this tricky issue.

vm/src/builtins/slice.rs

seungha-kim commented Mar 5, 2023

View changes

youknowone reviewed Mar 5, 2023

View changes

vm/src/class.rs Show resolved Hide resolved

youknowone requested changes Mar 6, 2023

View changes

youknowone reviewed Mar 6, 2023

View changes

youknowone reviewed Mar 7, 2023

View changes

youknowone added the z-ls-2023 Tag to track Line OSS Sprint 2023 label Mar 7, 2023

youknowone mentioned this pull request Mar 7, 2023

Implement protocols (Abstract obejcts layer) #3244

Open

120 tasks

seungha-kim added 8 commits March 7, 2023 22:44

Update to pass test for unhashable

eb0f2f3

Fix casting error of function pointer

2e12da2

Generalize unhashable logic using PyClassDef

92526b5

Use map_or instead of map + unwrap_or

725060c

Compare with ctx.none instead of ctx.none()

5588a63

Simplify duplicated call of toggle_slot!

297179e

Follow CPython's naming for unhashable

95fd1e8

Make PyRef<T>'s UNHASHABLE follow T's

6d4e7c9

seungha-kim force-pushed the feature/unhashable branch from 19fe761 to ccded70 Compare March 7, 2023 13:45

Remove unused import

e5b338e

seungha-kim force-pushed the feature/unhashable branch from ccded70 to e5b338e Compare March 7, 2023 13:50

Remove expectedFailure annotation of passed test

07274f3

youknowone approved these changes Mar 7, 2023

View changes

vm/src/builtins/slice.rs Outdated Show resolved Hide resolved

Update vm/src/builtins/slice.rs

ea7c4f0

youknowone merged commit 223fe14 into RustPython:main Mar 7, 2023

seungha-kim deleted the feature/unhashable branch March 7, 2023 14:53

Feb	MAR	Apr
	07
2022	2023	2024

Update to pass test for unhashable collections #4640

Update to pass test for unhashable collections #4640

seungha-kim commented Mar 5, 2023

seungha-kim Mar 5, 2023

seungha-kim Mar 5, 2023

youknowone commented Mar 5, 2023

youknowone Mar 5, 2023 •

edited

seungha-kim Mar 6, 2023

youknowone Mar 6, 2023

youknowone left a comment

youknowone Mar 6, 2023

youknowone Mar 6, 2023

youknowone Mar 6, 2023

youknowone Mar 6, 2023

youknowone Mar 6, 2023

youknowone Mar 7, 2023

youknowone left a comment

youknowone Mar 7, 2023

youknowone Mar 7, 2023

youknowone left a comment

		.map(\|h\| h as usize == unhashable_wrapper as usize)
		.unwrap_or(false)

	.map(\|h\| h as usize == unhashable_wrapper as usize)
	.unwrap_or(false)
	.map_or(0, \|h\| h as usize) == unhashable_wrapper as usize

	.map(\|a\| a.is(&ctx.none()))
	.unwrap_or(false);
	.map_or(false, \|a\| a.is(&ctx.none));

	pub fn unhashable_wrapper(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> {
	pub fn hash_unhashable(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> {

	const UNHASHABLE: bool = false;
	const UNHASHABLE: bool = T::UNHASHABLE;

Update to pass test for unhashable collections #4640

Update to pass test for unhashable collections #4640

Conversation

seungha-kim commented Mar 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone commented Mar 5, 2023

youknowone Mar 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone left a comment

Choose a reason for hiding this comment

youknowone Mar 5, 2023 •

edited