Fix CatchAll fields breaking on reuse across structural contexts - #253
Open
sammck wants to merge 1 commit into
Open
Fix CatchAll fields breaking on reuse across structural contexts#253sammck wants to merge 1 commit into
sammck wants to merge 1 commit into
Conversation
load_func_for_dataclass()/dump_func_for_dataclass() popped the CATCH_ALL marker off of the per-class alias dict returned by resolve_dataclass_field_to_alias_for_load()/_for_dump(). That dict is a direct reference into a WeakKeyDictionary cached once per class, not a copy, so the first codegen pass for a class permanently stripped the marker from the shared cache. Any later codegen pass for the same class in a different structural context (e.g. used as a nested field after being used as a top-level from_dict()/to_dict() target, or nested in a second, different enclosing class) then saw has_catch_all as False, let the field's CatchAll (a typing.NewType, not a class) fall through to the normal dispatch path, and hit issubclass(origin, t) with a non-class origin, raising TypeError. Use .get() instead of .pop() so the cached dict is only ever read, never mutated, fixing reuse in any number of structural contexts including the self-referential case (which hits two conflicting contexts in a single top-level call).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A dataclass with a
CatchAllfield can currently only be successfullyloaded/dumped in one distinct structural context per process. "Context"
means: used as the direct top-level target of
.from_dict()/.to_dict(),vs. used as a field of enclosing class X, vs. used as a field of enclosing
class Y. Whichever context is hit first works; any second, differently-shaped
use of that same class then fails permanently for the rest of the process
with:
raised from
_loaders.py::load_dispatcher_for_annotation(and the analogousspot in
_dumpers.py) atif issubclass(origin, t):.This isn't recursion-specific — plain, non-self-referential classes hit it
too (e.g. calling
.from_dict()directly on some classLeafwith aCatchAllfield, then later parsing a different class that hasLeafas anested field, breaks the second call). Self-referential classes
(
children: list[Self]) are a special case that fails on their very firstuse, since resolving them as a top-level target already requires generating
codegen for the same class twice in one pass (once as the entry point, once
as the nested self-reference).
Repro
Root cause
load_func_for_dataclass()(anddump_func_for_dataclass()) read theper-class alias mapping via
resolve_dataclass_field_to_alias_for_load()/_for_dump(), which returns a direct reference to a dict cached once perclass in a module-level
WeakKeyDictionary— not a copy. The code then did:.pop()destructively mutates that shared cached dict. The first timecodegen runs for a class,
has_catch_allis correctlyTrueand theCatchAllfield is stripped/special-cased as intended — but the marker isnow permanently gone from the shared cache. If codegen runs again for the
same class in a different structural context (which doesn't hit a
compiled-function cache, since nested-field codegen always calls
load_func_for_dataclass/dump_func_for_dataclassdirectly rather thanreusing
cls.__dataclass_wizard_from_dict__),has_catch_allis nowincorrectly
False. The field's declared type,CatchAll = NewType('CatchAll', Mapping),is a
typing.NewType— callable, but not a class — so it falls through tothe normal dispatch path, which calls
issubclass(origin, t)on it andraises.
Fix
Use
.get(CATCH_ALL)instead of.pop(CATCH_ALL, None)in both_loaders.pyand_dumpers.py.CATCH_ALLis a sentinel string(
'<-|CatchAll|->') that never collides with a real field name, so nothingdownstream relies on the entry being removed from the dict — only on
reading whether it's present. This makes the read side-effect-free, so the
shared per-class cache stays correct no matter how many different
structural contexts the class is later used in.
Test plan
test_catch_all_reused_across_structural_contextsintests/unit/test_loaders.py, covering aCatchAll-bearing class usedas a top-level target, then nested in one enclosing class, then nested
in a second, different enclosing class — all in the same process.
Confirmed it reproduces the original
TypeErroragainst theunpatched source and passes with the fix.
0 failed.