Raise validation error when unhashable items added to a set

[mikeedjones]

Change Summary

Adds the validate_hashable function to check that items being added to a set by validate_iter_to_set have an implemented __hash__ function. Adds the associated SetItemNotHashable error and error message.

I'm not sure this is the best place to check whether or not items being added to a set are hashable, and I'm not sure binding the object is strictly necessary to check its hashability. Maybe this validation should happen in SetValidator instead and only capture the case where the user is specifically trying to add unhashable objects to a set?

Related issue number

pydantic/pydantic#11360 in the pydantic repo - I can open a mirror issue in pydantic-core?

Checklist

• Unit tests for the changes exist
• Documentation reflects the changes where applicable
• Pydantic tests pass with this pydantic-core (except for expected changes)
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @davidhewitt

[codspeed-hq]

CodSpeed Performance Report

Merging #1619 will not alter performance

_{Comparing mikeedjones:mj/unhashable-items-in-set (3d924d5) with main (164b9ff)}

Summary

✅ 157 untouched benchmarks

[codecov]

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.60%. Comparing base (ab503cb) to head (3d924d5).
Report is 282 commits behind head on main.

Files with missing lines	Patch %	Lines
src/input/return_enums.rs	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1619      +/-   ##
==========================================
- Coverage   90.21%   89.60%   -0.61%     
==========================================
  Files         106      115       +9     
  Lines       16339    17929    +1590     
  Branches       36       25      -11     
==========================================
+ Hits        14740    16066    +1326     
- Misses       1592     1844     +252     
- Partials        7       19      +12     

Files with missing lines	Coverage Δ
python/pydantic_core/core_schema.py	95.08% <ø> (+0.31%)	⬆️
src/errors/types.rs	99.72% <100.00%> (+0.27%)	⬆️
src/input/return_enums.rs	87.32% <88.23%> (+1.61%)	⬆️

... and 63 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164b9ff...3d924d5. Read the comment docs.

[mikeedjones]

please review :)

[davidhewitt]

Thanks, overall this looks in the right direction, I'd like to suggest an efficiency gain before merging.

[davidhewitt]

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.

[mikeedjones]

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?

[davidhewitt]

Let's go with that 👍

[davidhewitt]

@pydantic/oss - any opinion on the new error type introduced here? Seems ok to me, though we'll need to handle downstream in pydantic.

[Viicos]

Thanks @mikeedjones for looking into this. Can this be made more generic in some way? Sets are not the only type requiring hashable members. Dict keys comes to mind, although I don't think there's currently a way to reproduce the same error, as we only allow mappings to validate against dicts (meaning if you already have a valid mapping, keys are guaranteed to be hashable).

[mikeedjones]

I agree that would be better @Viicos - but as you point out in most other instances the hashability of the relevant item is checked much earlier in the validation.

A generic version might be a validator for python's Hashable? (unless there is one which i've missed sorry) - But adding that validator by default to validation of a dict's keys or a set's members would duplicate the checks done in pyo3 and other libraries, as @davidhewitt pointed out.

[Viicos]

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

[Viicos]

Yeah I think it's fine to continue with this approach scoped to sets, and we can extend/change this in the future.

[Viicos]

@pydantic/oss - any opinion on the new error type introduced here? Seems ok to me, though we'll need to handle downstream in pydantic.

LGTM!

[davidhewitt]

Thanks @mikeedjones for adding the downstream PR too 👍