Fix subclass discovery for slots=True and nested concrete hierarchies#51
Conversation
…te hierarchies This covers both bugs: skipping ghost classes left by `@dataclass(slots=True)` and recursing into concrete classes to find their concrete children.
There was a problem hiding this comment.
I don't think this is strictly a correct solution, but maybe it is better than the alternative. The __fields_serializer__ is put on by the @serializable decorator, but there is no reason that a class decorated with @abstract_serializable cannot be subclassed by a regular Serializable instead of with @serializable.
There was a problem hiding this comment.
Hmm, maybe testing if the class has to_data and from_data methods. That is essentially testing for isinstance(cls, Serializer).
There was a problem hiding this comment.
No, that's not right either because then SerializableMixin won't work. Stupid slots=True classes... Dataclass should have been a metaclass.
There was a problem hiding this comment.
I did some more research on this topic, and I think I now have a more robust approach towards this issue with slot=True classes. I refactored a bit related tests too.
drhagen
left a comment
drhagen
left a comment
There was a problem hiding this comment.
I think you genuinely solved this tough problem. Great contribution!
There was a problem hiding this comment.
uv only has the 3.14.0 version and I don't want to use it. I don't typically keep this up to date. In fact, it is usually better to have it be old. Updating to 3.13.8 would be fine.
There was a problem hiding this comment.
3.13.12 seems to be available too.
There was a problem hiding this comment.
I had an outdated version of uv locally. You are correct that 3.13.12 is available, and upgrading to that is fine.
I was wrong about 3.14.0 only being available. In fact, 3.14.3 is available, but I still don't want to upgrade to it. I like to be a little behind for local development to avoid accidentally using new Python features.
It is also fine to leave this out of this PR and leave it for a general dependencies upgrade.
There was a problem hiding this comment.
I updated uv everywhere, hopefully (CI and pyproject.toml). CI is passing now.
There was a problem hiding this comment.
Ok, this is actually really clever. We are probably relying on undocumented behavior, but that's what CI is for.
| # @dataclass(slots=True) creates a replacement class and leaves the original in __subclasses__() | ||
| # (https://github.com/python/cpython/issues/135228). Hold strong references so original classes | ||
| # survive GC and exercise the skipping logic. | ||
| garbage_collection_protection = [ | ||
| Base, | ||
| AbstractMiddle, | ||
| SlotsConcrete, | ||
| SlotsConcreteChild, | ||
| NoSlotsConcrete, | ||
| ManualConcrete, | ||
| MixinConcrete, | ||
| SlotsManualConcrete, | ||
| ] |
There was a problem hiding this comment.
It does not feel like this could do anything. The module itself is preventing those objects from getting garbage collected. I removed this locally and the tests still passed. Were you seeing sporatic failures?
There was a problem hiding this comment.
This was apparently needed for the initial version of these tests but not anymore.
| } | ||
|
|
||
|
|
||
| # ── Identity: every entry is the replacement, not the original ──────────────── |
There was a problem hiding this comment.
We don't do these kinds of comments in the Serialite test suite. Feel free to make the test name descriptive.
| # ── Identity: every entry is the replacement, not the original ──────────────── |
2 participants
When
@dataclass(slots=True)is used, Python creates a new class and leaves the original as a ghost in__subclasses__(). The old discovery logic treated any non-abstract subclass as concrete, so these ghosts were incorrectly included. I added a check for__fields_serializer__to confirm a subclass was actually decorated with@serializable.The old logic also never recursed into concrete classes, so a
@serializablesubclass of another@serializableclass was never discovered. I recurse into concrete classes to find their children too, now.I also updated the
Resulttype annotation and extended the type-checking test.