Tests fail with frozendict 2.0.6

[matoro]

Just trying to update my Synapse dependencies and noticed that one test for canonicaljson failed when using the latest release of the new frozendict.

This is testing the following versions:

• python 3.9.6
• canonicaljson 1.4.0
• frozendict 2.0.6
• simplejson 3.17.3

 * python3_9: running distutils-r1_run_phase python_test
...E...
======================================================================
ERROR: test_frozen_dict (test_canonicaljson.TestCanonicalJson)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0/test_canonicaljson.py", line 110, in test_frozen_dict
    encode_canonical_json(frozendict({"a": 1})), b'{"a":1}',
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0-python3_9/lib/canonicaljson.py", line 73, in encode_canonical_json
    s = _canonical_encoder.encode(json_object)
  File "/usr/lib/python3.9/site-packages/simplejson/encoder.py", line 296, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.9/site-packages/simplejson/encoder.py", line 378, in iterencode
    return _iterencode(o, 0)
  File "/var/tmp/portage/dev-python/canonicaljson-1.4.0/work/python-canonicaljson-1.4.0-python3_9/lib/canonicaljson.py", line 29, in _default
    return obj._dict
AttributeError: 'frozendict.frozendict' object has no attribute '_dict'
 
----------------------------------------------------------------------
Ran 7 tests in 0.037s
 
FAILED (errors=1)

[alex19EP]

first bad version is frozendict 2.0.4

[DMRobertson]

I talked about this with @H-Shay. We couldn't seem to reproduce it!

I noticed that frozendict's source code contained a C extension and wondered if that might be required to see the failure.

To test if you're using the C extension, use python -c "import frozendict; print(frozendict.c_ext)". This was false in my virtual environment, so I uninstalled frozendict and reinstalled it with

pip uninstall frozendict
pip install frozendict --no-binary=:all:

Having done so, the test failed.

[matoro]

Confirming that my frozendict is using the C extension.

[H-Shay]

As @DMRobertson noted above, it appears that the frozendict object created by the C extension is incompatible with our code here:

python-canonicaljson/canonicaljson.py

Line 25 in f691369

def _default(obj):

I think the team needs to take a look at this to determine what to do in this situation, in the meantime it appears that if you avoid using the C extension it should work.

[clokep]

As @DMRobertson noted above, it appears that the frozendict object created by the C extension is incompatible with our code here:

The "quick" solution would probably to also determine if the C-extensions are being used and copy the dict in that case. (I'm guessing there's no equivalent version of _dict in the C-code since it is probably implement in C, not compatible with a Python dictionary.)

That's a bit hacky, but I'm not sure there are other solutions. A better fallback might be to try and copy on attribute error:

try:
    return obj._dict
except AttributeError:
    return dict(obj)

[DMRobertson]

I wrote these notes up elsewhere but probably should have stuck them on github. For completeness:

I can see at least three implementations of frozendict.

• V1.1 and earlier. Pure python implementation. frozendict is a brand new class with a _dict attribute.
• V1.2--1.5, V2 without C extension. Pure python. frozendict inherits from dict and so is automatically json-encodable. That means the _default in canonicaljson isn't needed---that's why the problem wasn't reproducible at first, I think?)
• V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

There's also some discussion here by the current maintainer of frozendict. They were frustrated that the stdlib json module doesn't encode a generic mapping. (The rationale from the CPython core: just because something implements the mapping interface doesn't mean its entire state is exposed by that interface; we don't want to have lossy serialisation.)

[squahtx]

V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

A quick skim of the frozendict C extension reveals a surprising amount of hashtable implementation-y code: https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c

which makes me suspect the C extension has its own dict implementation and there's no Python dict that can be pulled out to avoid taking a copy.

[DMRobertson]

V2 with C extension. C implementation. Not a subclass of dict, nor does it have a _dict attribute. So I'm not sure there's a nice option other than converting it to a dict beforehand?

A quick skim of the frozendict C extension reveals a surprising amount of hashtable implementation-y code: https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c

which makes me suspect the C extension has its own dict implementation and there's no Python dict that can be pulled out to avoid taking a copy.

Yeah, I'm guessing it's CPython's builtin dict, but with the mutability interface removed?

[squahtx]

Yeah, I'm guessing it's CPython's builtin dict, but with the mutability interface removed?

Ah, it actually is a copy of the code!
https://github.com/Marco-Sulla/python-frozendict/blob/master/frozendict/src/3_6/frozendictobject.c
https://github.com/python/cpython/blob/main/Objects/dictobject.c