Skip to content

Commit

Permalink
fix: #517 special handling of typing module
Browse files Browse the repository at this point in the history
  • Loading branch information
mmckerns committed Jul 23, 2022
1 parent 5a66152 commit 0ca195f
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 0 deletions.
25 changes: 25 additions & 0 deletions dill/_dill.py
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ def get_file_type(*args, **kwargs):

import inspect
import dataclasses
import typing

from pickle import GLOBAL

Expand Down Expand Up @@ -761,6 +762,13 @@ def _create_ftype(ftypeobj, func, args, kwds):
args = ()
return ftypeobj(func, *args, **kwds)

def _create_typing_tuple(argz, *args): #NOTE: workaround python bug

This comment has been minimized.

Copy link
@tvalentyn

tvalentyn Jul 25, 2022

would be nice to mention python/cpython#94245

if not argz:
return typing.Tuple[()].copy_with(())
if argz == ((),):
return typing.Tuple[()]
return typing.Tuple[argz]

def _create_lock(locked, *args): #XXX: ignores 'blocking'
from threading import Lock
lock = Lock()
Expand Down Expand Up @@ -1276,6 +1284,23 @@ def save_classobj(pickler, obj): #FIXME: enable pickler._byref
logger.trace(pickler, "# C2")
return

@register(typing._GenericAlias)

This comment has been minimized.

Copy link
@tvalentyn

tvalentyn Jul 25, 2022

should we register this on all Python version or only on the buggy ones?

This comment has been minimized.

Copy link
@mmckerns

mmckerns Jul 26, 2022

Author Member

I had the same thought. Currently, the latest 3.10 release has the bug, but the next one won't. The bug won't be fixed in 3.7-3.9. You will note that dill only intercepts pickle in L1294, and the other two cases use the __reduce__ methods that come with the classes themselves. The fix in _create_typing_tuple will be needed past the python bug fix being applied, due to any stored pickles containing the second erroneous case Tuple[]. So, we will need to keep it around... however, if we want to change it once the patches are in releases for 3.10 and 3.11, then we can modify the cases in this function (and create a dill._shim if needed.

def save_generic_alias(pickler, obj):
args = obj.__args__
if type(obj.__reduce__()) is str:
logger.trace(pickler, "Ga0: %s", obj)
StockPickler.save_global(pickler, obj, name=obj.__reduce__())
logger.trace(pickler, "# Ga0")
elif obj.__origin__ is tuple and (not args or args == ((),)):

This comment has been minimized.

Copy link
@tvalentyn

tvalentyn Jul 25, 2022

Due to and (not args or args == ((),)), line 770 won't get executed. Is that intentional?

This comment has been minimized.

Copy link
@mmckerns

mmckerns Jul 26, 2022

Author Member

Yes. L770 won't get called by _save_generic_alias, but it is more correct with the last case for "create typing tuple". (not empty typing tuple).

logger.trace(pickler, "Ga1: %s", obj)
pickler.save_reduce(_create_typing_tuple, (args,), obj=obj)
logger.trace(pickler, "# Ga1")
else:
logger.trace(pickler, "Ga2: %s", obj)
StockPickler.save_reduce(pickler, *obj.__reduce__(), obj=obj)
logger.trace(pickler, "# Ga2")
return

@register(LockType)
def save_lock(pickler, obj):
logger.trace(pickler, "Lo: %s", obj)
Expand Down
17 changes: 17 additions & 0 deletions dill/tests/test_selected.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,26 @@ def test_frame_related():
assert ok
if verbose: print ("")

def test_typing():
import typing #FIXME: test special cases
x = typing.Dict[int, str]
assert x == dill.copy(x)
x = typing.List[int]
assert x == dill.copy(x)
x = typing.Tuple[int, str]
assert x == dill.copy(x)
x = typing.Tuple[int]
assert x == dill.copy(x)
x = typing.Tuple[()]
assert x == dill.copy(x)
x = typing.Tuple[()].copy_with(())
assert x == dill.copy(x)
return


if __name__ == '__main__':
test_frame_related()
test_dict_contents()
test_class()
test_class_descriptors()
test_typing()

0 comments on commit 0ca195f

Please sign in to comment.