Update to pass test for unhashable collections by seungha-kim · Pull Request #4640 · RustPython/RustPython

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

+    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:11Z

+    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.

youknowone

I added a few more style suggestions.

youknowone · 2023-03-06T08:44:17Z

+            .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:46:25Z

            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:50:33Z

+                    .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:52:29Z

+                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

 }

+/// 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:01:31Z

    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:01:52Z

    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.

seungha-kim commented Mar 5, 2023

View reviewed changes

youknowone reviewed Mar 5, 2023

View reviewed changes

Comment thread vm/src/class.rs

youknowone requested changes Mar 6, 2023

View reviewed changes

youknowone reviewed Mar 6, 2023

View reviewed changes

youknowone reviewed Mar 7, 2023

View reviewed 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

blue-pandaa 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 reviewed changes

Comment thread vm/src/builtins/slice.rs Outdated

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

minhrongcon2000 pushed a commit to minhrongcon2000/RustPython that referenced this pull request Mar 10, 2023

Update to pass test for unhashable collections (RustPython#4640)

a5bacba

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

Conversation

seungha-kim commented Mar 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Mar 5, 2023

Uh oh!

youknowone Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

youknowone Mar 5, 2023 •

edited

Loading