gh-129250: allow pickle instances of generic classes #129446
Conversation
Objects/typevarobject.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| PyObject *typing = PyImport_ImportModule("typing"); |
There was a problem hiding this comment.
We shouldn't need to do this, we know when type params come directly from a PEP 695 generic.
An alternative implementation approach could be to save some information on TypeVar creation (when INTRINSIC_TYPEVAR is called). At that point, we don't have the function object yet, but we know what its module, qualname, and index are going to be, so we could store that information on the TypeVar, which should be enough to unpickle it.
There was a problem hiding this comment.
TL;DR: Two possible approaches, both require a new INTRINSC. The first requires a minimal instruction emit added to compile.c whereas the second requires a bit more (peek future scope __qualname__). The second is the one currently up here.
Disassembly of <code object <generic parameters of cls> ...
...
1:0-2:8 LOAD_DEREF 2 (.type_params)
1:0-2:8 CALL_INTRINSIC_1 10 (INTRINSIC_SUBSCRIPT_GENERIC)
1:0-2:8 STORE_FAST_LOAD_FAST 17 (.generic_base, .generic_base)
1:0-2:8 CALL 3
+ 1:0-2:8 CALL_INTRINSIC_1 12 (INTRINSIC_SET_TYPE_PARAMS_OWNER)
1:0-2:8 RETURN_VALUE
Or:
Disassembly of <code object <generic parameters of func> at 0x7f812b61d360, file "../y.py", line 13>:
-- COPY_FREE_VARS 1
13:0-13:0 RESUME 0
13:13-13:14 LOAD_CONST 0 ('T')
13:13-13:14 CALL_INTRINSIC_1 7 (INTRINSIC_TYPEVAR)
13:13-13:14 COPY 1
13:13-13:14 STORE_FAST 1 (T)
+ 13:13-13:14 LOAD_GLOBAL 0 (__name__)
+ 13:13-13:14 LOAD_CONST 1 ('cls.func')
+ 13:13-13:14 LOAD_SMALL_INT 0
+ 13:13-13:14 BUILD_TUPLE 3
+ 13:13-13:14 CALL_INTRINSIC_2 6 (INTRINSIC_SET_TYPEPARAM_OWNER)
13:13-13:14 BUILD_TUPLE 1
13:4-13:23 LOAD_CONST 2 (<code object func at 0x7f812b6cc9a0, file "../y.py", line 13>)
13:4-13:23 MAKE_FUNCTION
13:4-13:23 SWAP 2
13:4-13:23 CALL_INTRINSIC_2 4 (INTRINSIC_SET_FUNCTION_TYPE_PARAMS)
13:4-13:23 RETURN_VALUE
There was a problem hiding this comment.
Thank you! It makes me a little sad to add this much complexity, but this does fix a real bug and the runtime overhead should be quite small. Let's add some more tests and run it through the buildbots to make sure there are no refleaks, and then we can land it.
|
I do want to state that the code owner of EDIT: To minimize changes the first method is preferable, not he one that is currently up. |
Misc/NEWS.d/next/Core_and_Builtins/2025-01-29-15-32-02.gh-issue-129250.ExhmQQ.rst
Outdated
Show resolved
Hide resolved
|
If worried about the extra compiler complexity, consider the first approach which is much simpler at the cost of no pickle of the TypeAlias |
This is a maybe PR, not sure if is not overkill for original issue #129250 which may be too niche?
Fixes OP issue by allowing pickling of anonymous type parameters - those which do not exist at module scope but are defined through generic class and function syntax
class cls[T]: ...ordef func[T](): .... This is done by adding a weakrefownervariable which is set on creation for anonymous type params and provides a context for pickling so that on unpickle the type param can be recreated identically. TypeVar, ParamSpec and TypeVarTuple are modifed to work in this manner. Owner is only ever set at creation for type params which have__module__==typingand the name either does not exist intypingor if it does is not exactly this typeparam.If this is accepted as the way to go, then here are some questions:
TypeAlias does not currently support weak references for some reason so can't use this method to pickle the anon typeparams in
type t[A, *B, **C] = .... This is probably not a problem as a TypeAlias can't instantiate objects with its anon typeparams like a func or class can? Can add support anyways now if weakrefs on TypeAlias will be added at some point in the future?What to do on explicit assign to
__type_params__, add owner to assigned typeparams where applicable? Remove from old?Only path supported to anonymous type param is sequence index in owner
__type_params__, are other paths possible/desired?If
builtin___build_class__()is not a good place to set new class typeparams owner, there are other options.