Repository navigation
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #642 Include inherited slots when hashing slotted objects
Summary: This PR changes DeepHash._get_slots_dict to walk type(obj).__mro__ and read each class's own __slots__, treating a string declaration as one name. Before, it iterated obj.__slots__, which only shows the most-derived declaration and splits a string into characters. On dev, instances of fully-slotted subclasses (e.g. class Derived(Base): __slots__ = ()) and of classes with __slots__ = 'value' are hashed without their slot values. Instances with different values therefore get the same hash, and ignore_order=True misses the change. Reading the code, this PR fixes that.
Overlap with other PRs
- #609 (merged to
dev) fixedDeepDiff._dict_from_slotsindeepdiff/diff.py: parent-declared mangled slots were lost on subclass instances (#506). This PR changes a different function,DeepHash._get_slots_dictindeepdiff/deephash.py. It complements #609, does not duplicate it, and does not conflict with it. After both,DeepHashandDeepDiffinclude the same non-mangled slots. Mangled names still differ (see below). - This builds on #577 (merged), which added
_get_slots_dictand its__getattr__fallback. - No other open PR touches this code. The PR merges cleanly with #639 and #641, which also change
deephash.py(checked withgit merge-tree).
Security
No security concerns found in static review. The change only reads class __dict__ entries and instance attributes. It adds no new I/O, imports or dynamic evaluation, and the diff has no hidden Unicode.
Correctness
-
deepdiff/deephash.py:303-306:cls.__dict__.get('__slots__', ())returns only the slots each class declares itself, which is the right approach. If a subclass declares a slot name again, the entry is overwritten with the same value._prep_dictsorts its entries, so MRO order does not affect the hash. -
Private (mangled) slots are still looked up by the name in the declaration (
deepdiff/deephash.py:309), e.g.__idrather than_Base__id, so the lookup never finds them. By defaultignore_private_variablesdrops these keys anyway. Two cases are still affected:- With
ignore_private_variables=False, DeepHash leaves these slots out, whileDeepDiff._dict_from_slotsincludes them (since #609). - If the class defines
__getattr__, the fallback at line 315 calls__getattr__('__id'). If that raises, the whole object becomesunprocessed. This used to happen only for slots declared on the leaf class; it now also happens for inherited ones.
The loop already knows the declaring
cls, so it could unmangle the name here at little cost. Python removes leading underscores from the class name when it mangles: insideclass _Foo,__xbecomes_Foo__x. - With
-
Behavior change: if an inherited slot is not initialized and
__getattr__raises for it, the object is nowunprocessed(asserted attests/test_hash.py:356). This matches the existing leaf-slot behavior from #577. Note that withignore_order, DeepDiff logs such items and leaves them out of the comparison. -
Question:
_prep_objuseshasattr(obj, "__slots__")(line 452) to decide whether to try this strategy. That check is also true when__slots__comes from__getattr__delegation, as with transparent proxies. Before, the delegated slot names were used and their values read through__getattr__. Walkingtype(obj).__mro__won't find them, so such objects would now hash without the delegated values. Do you want to keep that case working, e.g. by falling back toobj.__slots__when the walk finds no declared slots? -
Out of scope, for reference: if any base class lacks
__slots__, instances have a__dict__and theo.__dict__strategy is used, so slot values are still not hashed.DeepDiffcovers that case throughdetailed__dict__. -
Hash stability: hashes of slotted-subclass instances and of string-
__slots__instances will change. That is the intended fix, and the CHANGELOG entry covers it.
Tests
- Reading the code, the new cases at
tests/test_hash.py:304,:338(string form),:356and:380should fail ondev. There,obj.__slots__shows only the leaf declaration or the characters of a string, so neitherhashes[old] != hashes[new]nor theignore_type_in_groupsequality holds. The dict-form__slots__case and theequalcomparisons are controls. - No test covers a private (mangled) slot declared on a parent class. One would be worth adding if unmangling goes in.
Suggestions
- Consider resolving mangled slot names with the declaring class, so
DeepHashbehaves likeDeepDiff._dict_from_slots. - Optional: add a proxy-style regression test if the question above matters.
AI-assisted static review; the code was not executed.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #642 +/- ##
=======================================
Coverage 95.07% 95.08%
=======================================
Files 18 18
Lines 4918 4922 +4
=======================================
+ Hits 4676 4680 +4
Misses 242 242 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DeepHash iterates
obj.__slots__, which can omit parent-defined slots and split a string declaration into characters. Different slot values can then produce equal hashes, causingignore_order=Trueto miss changes.__getattr__behaviorRelated: #506 and #577
Validation: