Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 11 commits into from
Mar 7, 2023
Merged

Update to pass test for unhashable collections #4640

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eb0f2f3
Update to pass test for unhashable
blue-pandaa Mar 7, 2023
2e12da2
Fix casting error of function pointer
blue-pandaa Mar 5, 2023
92526b5
Generalize unhashable logic using PyClassDef
blue-pandaa Mar 7, 2023
725060c
Use map_or instead of map + unwrap_or
blue-pandaa Mar 7, 2023
5588a63
Compare with ctx.none instead of ctx.none()
blue-pandaa Mar 7, 2023
297179e
Simplify duplicated call of toggle_slot!
blue-pandaa Mar 7, 2023
95fd1e8
Follow CPython's naming for unhashable
blue-pandaa Mar 7, 2023
6d4e7c9
Make PyRef<T>'s UNHASHABLE follow T's
blue-pandaa Mar 7, 2023
e5b338e
Remove unused import
blue-pandaa Mar 7, 2023
07274f3
Remove expectedFailure annotation of passed test
blue-pandaa Mar 7, 2023
ea7c4f0
Update vm/src/builtins/slice.rs
youknowone Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Update to pass test for unhashable
  • Loading branch information
@blue-pandaa
blue-pandaa committed Mar 7, 2023
commit eb0f2f3ed8a07d83ea4ba5927254f33e8e9a75ed
2 changes: 0 additions & 2 deletions Lib/test/test_collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,8 +892,6 @@ def __await__(self):
self.assertFalse(isinstance(CoroLike(), Coroutine))
self.assertFalse(issubclass(CoroLike, Coroutine))

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_Hashable(self):
# Check some non-hashables
non_samples = [bytearray(), list(), set(), dict()]
Expand Down
16 changes: 10 additions & 6 deletions vm/src/builtins/bytearray.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ use crate::{
},
sliceable::{SequenceIndex, SliceableSequenceMutOp, SliceableSequenceOp},
types::{
AsBuffer, AsMapping, AsNumber, AsSequence, Callable, Comparable, Constructor, Hashable,
Initializer, IterNext, IterNextIterable, Iterable, PyComparisonOp, Unconstructible,
Unhashable,
unhashable_wrapper, AsBuffer, AsMapping, AsNumber, AsSequence, Callable, Comparable,
Constructor, Initializer, IterNext, IterNextIterable, Iterable, PyComparisonOp,
Unconstructible,
},
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject,
VirtualMachine,
Expand Down Expand Up @@ -91,6 +91,13 @@ impl PyPayload for PyByteArray {

/// Fill bytearray class methods dictionary.
pub(crate) fn init(context: &Context) {
context
.types
.bytearray_type
.slots
.hash
.store(Some(unhashable_wrapper));

@youknowone youknowone Mar 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

@seungha-kim seungha-kim Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll try.

@youknowone youknowone Mar 6, 2023

Copy link
Copy Markdown
Member

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.


PyByteArray::extend_class(context, context.types.bytearray_type);
PyByteArrayIterator::extend_class(context, context.types.bytearray_iterator_type);
}
Expand All @@ -100,7 +107,6 @@ pub(crate) fn init(context: &Context) {
with(
Constructor,
Initializer,
Hashable,
Comparable,
AsBuffer,
AsMapping,
Expand Down Expand Up @@ -873,8 +879,6 @@ impl AsNumber for PyByteArray {
}
}

impl Unhashable for PyByteArray {}

impl Iterable for PyByteArray {
fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
Ok(PyByteArrayIterator {
Expand Down
15 changes: 10 additions & 5 deletions vm/src/builtins/dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ use crate::{
protocol::{PyIterIter, PyIterReturn, PyMappingMethods, PyNumberMethods, PySequenceMethods},
recursion::ReprGuard,
types::{
AsMapping, AsNumber, AsSequence, Callable, Comparable, Constructor, Hashable, Initializer,
IterNext, IterNextIterable, Iterable, PyComparisonOp, Unconstructible, Unhashable,
unhashable_wrapper, AsMapping, AsNumber, AsSequence, Callable, Comparable, Constructor,
Hashable, Initializer, IterNext, IterNextIterable, Iterable, PyComparisonOp,
Unconstructible,
},
vm::VirtualMachine,
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyRefExact, PyResult,
Expand Down Expand Up @@ -206,7 +207,6 @@ impl PyDict {
Constructor,
Initializer,
AsMapping,
Hashable,
Comparable,
Iterable,
AsSequence,
Expand Down Expand Up @@ -512,8 +512,6 @@ impl Comparable for PyDict {
}
}

impl Unhashable for PyDict {}

impl Iterable for PyDict {
fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
Ok(PyDictKeyIterator::new(zelf).into_pyobject(vm))
Expand Down Expand Up @@ -1209,6 +1207,13 @@ impl AsSequence for PyDictValues {
}

pub(crate) fn init(context: &Context) {
context
.types
.dict_type
.slots
.hash
.store(Some(unhashable_wrapper));

PyDict::extend_class(context, context.types.dict_type);
PyDictKeys::extend_class(context, context.types.dict_keys_type);
PyDictKeyIterator::extend_class(context, context.types.dict_keyiterator_type);
Expand Down
23 changes: 10 additions & 13 deletions vm/src/builtins/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use crate::{
sequence::{MutObjectSequenceOp, OptionalRangeArgs, SequenceExt, SequenceMutExt},
sliceable::{SequenceIndex, SliceableSequenceMutOp, SliceableSequenceOp},
types::{
AsMapping, AsSequence, Comparable, Constructor, Hashable, Initializer, IterNext,
IterNextIterable, Iterable, PyComparisonOp, Unconstructible, Unhashable,
unhashable_wrapper, AsMapping, AsSequence, Comparable, Constructor, Initializer, IterNext,
IterNextIterable, Iterable, PyComparisonOp, Unconstructible,
},
utils::collection_repr,
vm::VirtualMachine,
Expand Down Expand Up @@ -86,15 +86,7 @@ pub(crate) struct SortOptions {
pub type PyListRef = PyRef<PyList>;

#[pyclass(
with(
Constructor,
Initializer,
AsMapping,
Iterable,
Hashable,
Comparable,
AsSequence
),
with(Constructor, Initializer, AsMapping, Iterable, Comparable, AsSequence),
flags(BASETYPE)
)]
impl PyList {
Expand Down Expand Up @@ -492,8 +484,6 @@ impl Comparable for PyList {
}
}

impl Unhashable for PyList {}

fn do_sort(
vm: &VirtualMachine,
values: &mut Vec<PyObjectRef>,
Expand Down Expand Up @@ -612,6 +602,13 @@ impl IterNext for PyListReverseIterator {
}

pub fn init(context: &Context) {
context
.types
.list_type
.slots
.hash
.store(Some(unhashable_wrapper));

@seungha-kim seungha-kim Mar 5, 2023

Copy link
Copy Markdown
Contributor Author

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

@seungha-kim seungha-kim Mar 5, 2023

Copy link
Copy Markdown
Contributor Author

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


let list_type = &context.types.list_type;
PyList::extend_class(context, list_type);

Expand Down
23 changes: 10 additions & 13 deletions vm/src/builtins/set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use crate::{
recursion::ReprGuard,
types::AsNumber,
types::{
AsSequence, Comparable, Constructor, Hashable, Initializer, IterNext, IterNextIterable,
Iterable, PyComparisonOp, Unconstructible, Unhashable,
unhashable_wrapper, AsSequence, Comparable, Constructor, Hashable, Initializer, IterNext,
IterNextIterable, Iterable, PyComparisonOp, Unconstructible,
},
utils::collection_repr,
vm::VirtualMachine,
Expand Down Expand Up @@ -489,15 +489,7 @@ fn reduce_set(
}

#[pyclass(
with(
Constructor,
Initializer,
AsSequence,
Hashable,
Comparable,
Iterable,
AsNumber
),
with(Constructor, Initializer, AsSequence, Comparable, Iterable, AsNumber),
flags(BASETYPE)
)]
impl PySet {
Expand Down Expand Up @@ -805,8 +797,6 @@ impl Comparable for PySet {
}
}

impl Unhashable for PySet {}

impl Iterable for PySet {
fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
Ok(zelf.inner.iter().into_pyobject(vm))
Expand Down Expand Up @@ -1251,6 +1241,13 @@ impl IterNext for PySetIterator {
}

pub fn init(context: &Context) {
context
.types
.set_type
.slots
.hash
.store(Some(unhashable_wrapper));

PySet::extend_class(context, context.types.set_type);
PyFrozenSet::extend_class(context, context.types.frozenset_type);
PySetIterator::extend_class(context, context.types.set_iterator_type);
Expand Down
14 changes: 10 additions & 4 deletions vm/src/builtins/slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// sliceobject.{h,c} in CPython
use super::{PyInt, PyIntRef, PyTupleRef, PyType, PyTypeRef};
use crate::types::unhashable_wrapper;
use crate::{
class::PyClassImpl,
convert::ToPyObject,
function::{FuncArgs, OptionalArg, PyComparisonValue},
sliceable::SaturatedSlice,
types::{Comparable, Constructor, Hashable, PyComparisonOp, Unhashable},
types::{Comparable, Constructor, PyComparisonOp},
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
};
use num_bigint::{BigInt, ToBigInt};
Expand All @@ -25,7 +26,8 @@ impl PyPayload for PySlice {
}
}

#[pyclass(with(Hashable, Comparable))]
// #[pyclass(with(Hashable, Comparable))]
Comment thread
youknowone marked this conversation as resolved.
Outdated
#[pyclass(with(Comparable))]
impl PySlice {
#[pygetset]
fn start(&self, vm: &VirtualMachine) -> PyObjectRef {
Expand Down Expand Up @@ -258,8 +260,6 @@ impl Comparable for PySlice {
}
}

impl Unhashable for PySlice {}

#[pyclass(module = false, name = "EllipsisType")]
#[derive(Debug)]
pub struct PyEllipsis;
Expand Down Expand Up @@ -292,6 +292,12 @@ impl PyEllipsis {
}

pub fn init(ctx: &Context) {
ctx.types
.slice_type
.slots
.hash
.store(Some(unhashable_wrapper));

PySlice::extend_class(ctx, ctx.types.slice_type);
PyEllipsis::extend_class(ctx, ctx.types.ellipsis_type);
}
5 changes: 2 additions & 3 deletions vm/src/builtins/type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,11 +1019,10 @@ impl SetAttr for PyType {
}
let assign = value.is_assign();

let mut attributes = zelf.attributes.write();
if let PySetterValue::Assign(value) = value {
attributes.insert(attr_name, value);
zelf.attributes.write().insert(attr_name, value);
} else {
let prev_value = attributes.remove(attr_name);
let prev_value = zelf.attributes.write().remove(attr_name);
if prev_value.is_none() {
return Err(vm.new_exception(
vm.ctx.exceptions.attribute_error.to_owned(),
Expand Down
12 changes: 11 additions & 1 deletion vm/src/class.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
builtins::{PyBaseObject, PyBoundMethod, PyType, PyTypeRef},
identifier,
object::{Py, PyObjectPayload, PyObjectRef, PyRef},
types::{PyTypeFlags, PyTypeSlots},
types::{unhashable_wrapper, PyTypeFlags, PyTypeSlots},
vm::Context,
};
use rustpython_common::{lock::PyRwLock, static_cell};
Expand Down Expand Up @@ -112,6 +112,16 @@ pub trait PyClassImpl: PyClassDef {
.into();
class.set_attr(identifier!(ctx, __new__), bound);
}

if class

@youknowone youknowone Mar 6, 2023

Copy link
Copy Markdown
Member

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

.slots
.hash
.load()
.map(|h| h as u64 == unhashable_wrapper as u64)
.unwrap_or(false)
{
class.set_attr(ctx.names.__hash__, ctx.none.clone().into());
Comment thread
youknowone marked this conversation as resolved.
}
}

fn make_class(ctx: &Context) -> PyTypeRef
Expand Down
8 changes: 3 additions & 5 deletions vm/src/stdlib/collections.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ mod _collections {
sequence::{MutObjectSequenceOp, OptionalRangeArgs},
sliceable::SequenceIndexOp,
types::{
AsSequence, Comparable, Constructor, Hashable, Initializer, IterNext, IterNextIterable,
Iterable, PyComparisonOp, Unhashable,
AsSequence, Comparable, Constructor, Initializer, IterNext, IterNextIterable, Iterable,
PyComparisonOp,
},
utils::collection_repr,
AsObject, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
Expand Down Expand Up @@ -57,7 +57,7 @@ mod _collections {

#[pyclass(
flags(BASETYPE),
with(Constructor, Initializer, AsSequence, Comparable, Hashable, Iterable)
with(Constructor, Initializer, AsSequence, Comparable, Iterable)
)]
impl PyDeque {
#[pymethod]
Expand Down Expand Up @@ -574,8 +574,6 @@ mod _collections {
}
}

impl Unhashable for PyDeque {}

impl Iterable for PyDeque {
fn iter(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
Ok(PyDequeIterator::new(zelf).into_pyobject(vm))
Expand Down
33 changes: 16 additions & 17 deletions vm/src/types/slot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {

@youknowone youknowone Mar 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@youknowone youknowone Mar 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, probably hash_not_implemented by following cpython naming

Err(vm.new_type_error(format!("unhashable type: {}", zelf.class().name())))
}

fn call_wrapper(zelf: &PyObject, args: FuncArgs, vm: &VirtualMachine) -> PyResult {
vm.call_special_method(zelf.to_owned(), identifier!(vm, __call__), args)
}
Expand Down Expand Up @@ -414,7 +419,17 @@ impl PyType {
update_pointer_slot!(as_mapping, mapping_methods);
}
_ if name == identifier!(ctx, __hash__) => {
toggle_slot!(hash, hash_wrapper);
let is_unhashable = self
.attributes
.read()
.get(identifier!(ctx, __hash__))
.map(|a| a.is(&ctx.none()))
.unwrap_or(false);

@youknowone youknowone Mar 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@youknowone youknowone Mar 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
_ if name == identifier!(ctx, __call__) => {
toggle_slot!(call, call_wrapper);
Expand Down Expand Up @@ -663,22 +678,6 @@ pub trait Hashable: PyPayload {
fn hash(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyHash>;
}

pub trait Unhashable: PyPayload {}

impl<T> Hashable for T
where
T: Unhashable,
{
fn slot_hash(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyHash> {
Err(vm.new_type_error(format!("unhashable type: '{}'", zelf.class().name())))
}

#[cold]
fn hash(_zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<PyHash> {
unreachable!("slot_hash is implemented for unhashable types");
}
}

#[pyclass]
pub trait Comparable: PyPayload {
#[inline]
Expand Down