Jellying __builtin__.set and frozenset

[james@…]

I've created a patch adding support for jellying sets and frozensets. No attempt has been made to support the implementations from the sets module.

I have not attempted to implement any circular reference handling, as I do not understand it. I would be more than happy to do so given a few pointers.

[therve]

This is probably a duplicate of #642. Also, this ticket should take place after #2761 is merged, so that we could add support to sets.Set more or less transparently. It's a blocker because we support 2.3.

[exarkun]

I'm not sure if #2761 will help this much. PB will have to recognize both kinds of sets and represent them uniformly on the network. It'll help a bit for deserialization though, I guess.

[james@…]

I'd considered serializing sets.Set and builtin.set to the same representation, and unserializing to builtin.set with a fallback to sets.Set. I did not implement it because of a concern for the type changing across the network collection. I think they should be serialized as separate types.

In that same spirit, #642 refers to the sets module and can be implemented separately.

[warner]

FWIW, in Foolscap I chose to serialize native 'set' and sets.Set identically, and to deserialize it to native 'set' only. (Foolscap now requires python2.4 or later). This causes a problem for application code that was written to be compatible with python2.3 (and uses sets.Set): the usual place where it crops up is when the app does set math (intersection or union) between a set coming in over the wire with a local set, since 'set' and sets.Set do not interoperate. However, obviously this code doesn't use foolscap directly, so there's always some new code being written which can coerce the inbound arguments to sets.Set before handing them to the old code. A hassle, but so far it hasn't seemed to be a big problem.

I decided to stick to one representation of 'set' in the Foolscap wire protocol because it seemed silly to expose an implementation detail in the protocol, especially because I hope for there to be non-Python implementations some day.

I'm not sure what to recommend for oldpb, though, since Twisted is still providing python2.3 compatibility.

Oh, and of course frozenset is a distinct type.

[therve]

(In [22397]) Basic implementation.

Refs #2958

[therve]

Please review!

[exarkun]

There's a significant problem with circular references involving sets:

exarkun@boson:~$ python
Python 2.5.1 (r251:54863, Oct  5 2007, 13:36:32) 
[GCC 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from twisted.spread.jelly import jelly, unjelly
>>> class x:
...     def __init__(self, set):
...             self.set = set
... 
>>> s = set()
>>> o = x(s)
>>> s.add(o)
>>> unjelly(jelly(o))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 1119, in unjelly
    return _Unjellier(taster, persistentLoad, invoker).unjellyFull(sexp)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 633, in unjellyFull
    o = self.unjelly(obj)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 670, in unjelly
    ret = thunk(obj[1:])
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 768, in _unjelly_reference
    o = self.unjelly(exp)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 685, in unjelly
    state = self.unjelly(obj[1])
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 670, in unjelly
    ret = thunk(obj[1:])
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 832, in _unjelly_dictionary
    self.unjellyInto(kvd, 1, v)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 748, in unjellyInto
    o = self.unjelly(jel)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 670, in unjelly
    ret = thunk(obj[1:])
  File "/home/exarkun/Projects/Twisted/trunk/twisted/spread/jelly.py", line 808, in _unjelly_set
    return set(l)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/persisted/crefutil.py", line 41, in __hash__
    assert 0, "I am not to be used as a dictionary key."
AssertionError: I am not to be used as a dictionary key.
>>> 

Basically, this stems from the use of unjellyInto in _unjelly_set. If there's a circular reference, unjellyInto will put a NotKnown into the container. This raises two problems. First, NotKnown isn't hashable (intentionally, afaict). Second, even if it could be put into the set, nothing would ever fix it up to have the right value later on.

Some other stuff:

• type in _unjelly_frozenset docstring - frozebset.
• The four new branches in the jelly method to handle sets could just be two, I think, without making it much more difficult to understand.
• indentation for the examples in the module docstring doesn't quite look right
• test_oldImmutableSets is doing an identity comparison of set against jelly.sets.ImmutableSet.
• Maybe it should be jelly._sets instead of jelly.set so people don't get the wrong idea? Or is this intended to be a public feature?
• We should document the type-switching that's possible with sets.

[exarkun]

_unjelly_set_or_frozenset is misnamed, since it isn't called via any dynamic dispatch mechanism. The underscores should go. jelly_iterable is the same, I think.

The module docstring's indentation still seems wrong.

Merge when fixed.

[therve]

(In [22570]) Merge set-jelly-2958

Author: therve Reviewer: exarkun Fixes #2958 Fixes #642

Add support for set, frozenset, sets.Set and sets.ImmutableSet to jelly.