Opened 13 years ago

Closed 12 years ago

#2297 defect closed fixed (fixed)

Make rebuild.latestClass more compliant with new style classes

Reported by: therve Owned by: therve
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, Glyph Branch:


test_rebuild has a todo for new styles classes for a long time.

I attach a patch correcting the todo; nothing too smart here, I just skip latestClass for __builtin__ classes, which looked reasonable to me.

Attachments (1)

rebuild.diff (2.7 KB) - added by therve 13 years ago.

Download all attachments as: .zip

Change History (12)

Changed 13 years ago by therve

Attachment: rebuild.diff added

comment:1 Changed 13 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Glyph to Jean-Paul Calderone

Unfortunately I think the tests were a bit incomplete. I expanded on them somewhat, and testSlots still fails. testTypeSubclass still passes, however.

Code is in rebuild-newstyle-2297 for now. Getting one test working is good and worth making the change for, I think. Maybe we can get the other to work somehow too before merging this, though.

comment:2 Changed 12 years ago by therve

I've looked further at this, and I don't manage do find a solution. I found a post of you on python-dev talking about the problem:

I'm not Python core expert, but I'd say there's definitely a problem in same_slots_added. First the equiv_structs tests is obviously false because of what's is done in compatible_for_assignment. And I didn't understand the size tests, even after reading Martin response to your mail.

I've reduced the problem to this example:

class A(object):
    __slots__ = ['a']

a = A()
B = type(A.__name__, A.__bases__, dict(A.__dict__))
a.__class__ = B

Maybe python-dev can enlighten us another time ?

comment:3 Changed 12 years ago by therve

I found a solution. But it needs a modification in Python, sorry :). Here's the patch:

Index: Objects/typeobject.c
--- Objects/typeobject.c        (revision 53162)
+++ Objects/typeobject.c        (working copy)
@@ -2405,7 +2405,8 @@
 same_slots_added(PyTypeObject *a, PyTypeObject *b)
        PyTypeObject *base = a->tp_base;
-       Py_ssize_t size;
+       Py_ssize_t size, i, size_slot_a, size_slot_b;
+       PyStringObject *elt_a, *elt_b;

        if (base != b->tp_base)
                return 0;
@@ -2416,6 +2417,22 @@
                size += sizeof(PyObject *);
        if (a->tp_weaklistoffset == size && b->tp_weaklistoffset == size)
                size += sizeof(PyObject *);
+       /* Check slots compliance */
+       size_slot_a = PyTuple_GET_SIZE(((PyHeapTypeObject *)a)->ht_slots);
+       size_slot_b = PyTuple_GET_SIZE(((PyHeapTypeObject *)b)->ht_slots);
+       if (size_slot_a != size_slot_b) {
+               return 0;
+       }
+       for (i = 0; i < size_slot_a; i++) {
+               elt_a = (PyStringObject *) PyTuple_GET_ITEM(((PyHeapTypeObject *)a)->ht_slots, i);
+               elt_b = (PyStringObject *) PyTuple_GET_ITEM(((PyHeapTypeObject *)b)->ht_slots, i);
+               if (!(elt_a->ob_size == elt_b->ob_size
+                         && (elt_a->ob_sval[0] == elt_b->ob_sval[0]
+                                 && memcmp(elt_a->ob_sval, elt_b->ob_sval, elt_a->ob_size) == 0))) {
+                       return 0;
+               }
+               size += sizeof(PyObject *);
+       }
        return size == a->tp_basicsize && size == b->tp_basicsize;

With this (and another trick in Python, by deleting the descriptors in __dict__), I can make the test pass.

I think I'll submit the patch to Python, but we'll have to find a temporary solution.

comment:4 Changed 12 years ago by therve

Keywords: review added
Owner: Jean-Paul Calderone deleted

My patch went into Python, it should be available in 2.6 (see I've skipped the test for current version.

comment:5 Changed 12 years ago by Glyph

Keywords: review removed
Owner: set to therve

There should be a sane exception (and tests for it) on Pythons less than 2.6.

The test skip message should also indicate that it is Python 2.6, not Twisted 2.6 (or, say, Linux 2.6 _) where this feature is not yet supported.

Thanks for getting this patch into Python.

comment:6 Changed 12 years ago by therve

Cc: Glyph added
Keywords: review added
Owner: therve deleted

I've added a new test, but I'm not really sure on how to set this up. Also, docstrings are awful :).

comment:7 Changed 12 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:8 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Glyph to therve
Status: assignednew

Near the end of the test module, these lines look erroneous:

    if sys.hexversion >= 0x02060000:
        test_slots.skip = "__class__ assignment for class with slots should work starting Python 2.6"

I guess it's just referring to the wrong test method.

Also, it might be easier to compare sys.version_info to (2, 6), but it's up to you.

In updateInstance, checking the class object for __slots__ would be better than checking the instance object, since an instance might have an {{{slots}} attribute which won't actually affect anything. If you feel like fixing that, there should probably be a test case for it, though (and if you don't want to fix it that's fine too).

comment:9 Changed 12 years ago by therve

Keywords: review added
Owner: changed from therve to Jean-Paul Calderone

Thanks, I fixed the points.

comment:10 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve

Cool, looks good.

comment:11 Changed 12 years ago by therve

Resolution: fixed
Status: newclosed

(In [20116]) Merge rebuild-newstyle-2297

Author: therve Reviewer: exarkun, glyph Fixes #2297

Clean some rough edges of twisted.python.rebuild for new style classes: prevent the reload of builtin, and gives a sane error for classes with slots. It also takes into account a recent patch for Python 2.6 that allows rebuild of classes with slots.

Note: See TracTickets for help on using tickets.