[Python-Dev] advice needed: best approach to enabling "metamodules"?

[Nathaniel Smith]

On Tue, Dec 2, 2014 at 9:19 AM, Antoine Pitrou <solipsis at pitrou.net> wrote:
> On Mon, 1 Dec 2014 21:38:45 +0000
> Nathaniel Smith <njs at pobox.com> wrote:
>>
>> object_set_class is responsible for checking whether it's okay to take
>> an object of class "oldto" and convert it to an object of class
>> "newto". Basically it's goal is just to avoid crashing the interpreter
>> (as would quickly happen if you e.g. allowed "[].__class__ = dict").
>> Currently the rules (spread across object_set_class and
>> compatible_for_assignment) are:
>>
>> (1) both oldto and newto have to be heap types
>> (2) they have to have the same tp_dealloc
>> (3) they have to have the same tp_free
>> (4) if you walk up the ->tp_base chain for both types until you find
>> the most-ancestral type that has a compatible struct layout (as
>> checked by equiv_structs), then either
>>    (4a) these ancestral types have to be the same, OR
>>    (4b) these ancestral types have to have the same tp_base, AND they
>> have to have added the same slots on top of that tp_base (e.g. if you
>> have class A(object): pass and class B(object): pass then they'll both
>> have added a __dict__ slot at the same point in the instance struct,
>> so that's fine; this is checked in same_slots_added).
>>
>> The only place the code assumes that it is dealing with heap types is
>> in (4b)
>
> I'm not sure. Many operations are standardized on heap types that can
> have arbitrary definitions on static types (I'm talking about the tp_
> methods). You'd have to review them to double check.

Reading through the list of tp_ methods I can't see any other that
look problematic. The finalizers are kinda intimate, but I think
people would expect that if you swap an instance's type to something
that has a different __del__ method then it's the new __del__ method
that'll be called. If we wanted to be really careful we should perhaps
do something cleverer with tp_is_gc, but so long as type objects are
the only objects that have a non-trivial tp_is_gc, and the tp_is_gc
call depends only on their tp_flags (which are unmodified by __class__
assignment), then we should still be safe (and anyway this is
orthogonal to the current issues).

> For example, a heap type's tp_new increments the type's refcount, so
> you have to adjust the instance refcount if you cast it from a non-heap
> type to a heap type, and vice-versa (see slot_tp_new()).

Right, fortunately this is easy :-).

> (this raises the interesting question "what happens if you assign to
> __class__ from a __del__ method?")

subtype_dealloc actually attempts to take this possibility into
account -- see the comment "Extract the type again; tp_del may have
changed it". I'm not at all sure that it's handling is *correct* --
there's a bunch of code that references 'type' between the call to
tp_del and this comment, and there's code after the comment that
references 'base' without recalculating it. But it is there :-)

>> -- same_slots_added unconditionally casts the ancestral types
>> to (PyHeapTypeObject*). AFAICT that's why step (1) is there, to
>> protect this code. But I don't think the check actually works -- step
>> (1) checks that the types we're trying to assign are heap types, but
>> this is no guarantee that the *ancestral* types will be heap types.
>> [Also, the code for __bases__ assignment appears to also call into
>> this code with no heap type checks at all.] E.g., I think if you do
>>
>> class MyList(list):
>>     __slots__ = ()
>>
>> class MyDict(dict):
>>     __slots__ = ()
>>
>> MyList().__class__ = MyDict()
>>
>> then you'll end up in same_slots_added casting PyDict_Type and
>> PyList_Type to PyHeapTypeObjects and then following invalid pointers
>> into la-la land. (The __slots__ = () is to maintain layout
>> compatibility with the base types; if you find builtin types that
>> already have __dict__ and weaklist and HAVE_GC then this example
>> should still work even with perfectly empty subclasses.)
>>
>> Okay, so suppose we move the heap type check (step 1) down into
>> same_slots_added (step 4b), since AFAICT this is actually more correct
>> anyway. This is almost enough to enable __class__ assignment on
>> modules, because the cases we care about will go through the (4a)
>> branch rather than (4b), so the heap type thing is irrelevant.
>>
>> The remaining problem is the requirement that both types have the same
>> tp_dealloc (step 2). ModuleType itself has tp_dealloc ==
>> module_dealloc, while all(?) heap types have tp_dealloc ==
>> subtype_dealloc. Here again, though, I'm not sure what purpose this
>> check serves. subtype_dealloc basically cleans up extra slots, and
>> then calls the base class tp_dealloc. So AFAICT it's totally fine if
>> oldto->tp_dealloc == module_dealloc, and newto->tp_dealloc ==
>> subtype_dealloc, so long as newto is a subtype of oldto -- b/c this
>> means newto->tp_dealloc will end up calling oldto->tp_dealloc anyway.
>> OTOH it's not actually a guarantee of anything useful to see that
>> oldto->tp_dealloc == newto->tp_dealloc == subtype_dealloc, because
>> subtype_dealloc does totally different things depending on the
>> ancestry tree -- MyList and MyDict above pass the tp_dealloc check,
>> even though list.tp_dealloc and dict.tp_dealloc are definitely *not*
>> interchangeable.
>>
>> So I suspect that a more correct way to do this check would be something like
>>
>> PyTypeObject *old__real_deallocer = oldto, *new_real_deallocer = newto;
>> while (old_real_deallocer->tp_dealloc == subtype_dealloc)
>>     old_real_deallocer = old_real_deallocer->tp_base;
>> while (new_real_deallocer->tp_dealloc == subtype_dealloc)
>>     new_real_deallocer = new_real_deallocer->tp_base;
>> if (old_real_deallocer->tp_dealloc != new_real_deallocer)
>>     error out;
>
> Sounds good.
>
>> Module subclasses would pass this check. Alternatively it might make
>> more sense to add a check in equiv_structs that
>> (child_type->tp_dealloc == subtype_dealloc || child_type->tp_dealloc
>> == parent_type->tp_dealloc); I think that would accomplish the same
>> thing in a somewhat cleaner way.
>
> There's no "child" and "parent" types in equiv_structs().

Not as currently written, but every single call site is of the form
equiv_structs(x, x->tp_base). And equiv_structs takes advantage of
this -- e.g., checking that two types have the same tp_basicsize is
pretty uninformative if they're unrelated types, but if they're parent
and child then it tells you that they have exactly the same slots.

I wrote a patch incorporating the above ideas:
  http://bugs.python.org/issue22986

-- 
Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org