Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
1 change: 1 addition & 0 deletions python/pydantic_core/core_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -4037,6 +4037,7 @@ def definition_reference_schema(
'list_type',
'tuple_type',
'set_type',
'set_item_not_hashable',
'bool_type',
'bool_parsing',
'int_type',
Expand Down
2 changes: 2 additions & 0 deletions src/errors/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ error_types! {
// ---------------------
// set errors
SetType {},
SetItemNotHashable {},
// ---------------------
// bool errors
BoolType {},
Expand Down Expand Up @@ -513,6 +514,7 @@ impl ErrorType {
Self::ListType {..} => "Input should be a valid list",
Self::TupleType {..} => "Input should be a valid tuple",
Self::SetType {..} => "Input should be a valid set",
Self::SetItemNotHashable {..} => "Set items should be hashable",
Self::BoolType {..} => "Input should be a valid boolean",
Self::BoolParsing {..} => "Input should be a valid boolean, unable to interpret input",
Self::IntType {..} => "Input should be a valid integer",
Expand Down
18 changes: 17 additions & 1 deletion src/input/return_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,22 @@ impl BuildSet for Bound<'_, PyFrozenSet> {
}
}

fn validate_hashable<'py>(
py: Python<'py>,
item: &(impl Input<'py> + ?Sized),
state: &mut ValidationState<'_, 'py>,
validator: &CombinedValidator,
) -> ValResult<PyObject> {
let result: PyObject = validator.validate(py, item, state)?;

let bound_result: &Bound<'_, PyAny> = result.bind(py);

match bound_result.hash() {
Ok(_) => Ok(result),
Err(_) => Err(ValError::new(ErrorTypeDefaults::SetItemNotHashable, item)),
}
}

#[allow(clippy::too_many_arguments)]
pub(crate) fn validate_iter_to_set<'py>(
py: Python<'py>,
Expand All @@ -216,7 +232,7 @@ pub(crate) fn validate_iter_to_set<'py>(
false => PartialMode::Off,
};
let item = item_result.map_err(|e| any_next_error!(py, e, input, index))?;
match validator.validate(py, item.borrow_input(), state) {
match validate_hashable(py, item.borrow_input(), state, validator) {
Ok(item) => {
set.build_add(item)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Making the change like this will have the cost of running hash twice for every element added to the set. I suggest instead the code is adjusted to only try calling item.hash() if set.build_add(item) fails, that way we're only adding overhead on the unhappy path.

Copy link
Contributor Author

@mikeedjones mikeedjones Feb 5, 2025

Choose a reason for hiding this comment

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

Sinceset.build_set takes ownership of item, instead of cloning item to calculate the hash if set.build_set fails, I thought it would be better to instead inspect the PyErr and return the SetItemNotHashable if it's a TypeError. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with that 👍

if let Some(max_length) = max_length {
Expand Down
1 change: 1 addition & 0 deletions tests/test_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ def f(input_value, info):
('iteration_error', 'Error iterating over object, error: foobar', {'error': 'foobar'}),
('list_type', 'Input should be a valid list', None),
('tuple_type', 'Input should be a valid tuple', None),
('set_item_not_hashable', 'Set items should be hashable', None),
('set_type', 'Input should be a valid set', None),
('bool_type', 'Input should be a valid boolean', None),
('bool_parsing', 'Input should be a valid boolean, unable to interpret input', None),
Expand Down
17 changes: 17 additions & 0 deletions tests/validators/test_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,23 @@ def test_set_multiple_errors():
]


def test_list_with_unhashable_items():
v = SchemaValidator({'type': 'set'})

class Unhashable:
def __hash__(self):
raise TypeError('unhashable type')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the canonical way to create an unhashable type is to set __hash__ = None


unhashable = Unhashable()

with pytest.raises(ValidationError) as exc_info:
v.validate_python([{'a': 'b'}, unhashable])
assert exc_info.value.errors(include_url=False) == [
{'type': 'set_item_not_hashable', 'loc': (0,), 'msg': 'Set items should be hashable', 'input': {'a': 'b'}},
{'type': 'set_item_not_hashable', 'loc': (1,), 'msg': 'Set items should be hashable', 'input': unhashable},
]


def generate_repeats():
for i in 1, 2, 3:
yield i
Expand Down
Loading