[mypyc] Fix vtable pointer with inherited dunder new #20302
Conversation
p-sawicki
force-pushed
the
fix-vtable-with-inherited-dunder-new
branch
from
a0ae332 to
3e8d126
Compare
for more information, see https://pre-commit.ci
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
I think this is a viable fix, but will leave up to @JukkaL to double-check.
mypyc/lib-rt/generic_ops.c
Outdated
| continue; | ||
| } | ||
|
|
||
| while (def->ml_name && strcmp(def->ml_name, "__internal_mypyc_setup")) { |
There was a problem hiding this comment.
This search may be slow, we know that this method should be first if it is there, so maybe only check the first method, and if it has a different name, move to next base?
| if isinstance(typ_arg, NameExpr) and len(method_args) > 0 and method_args[0] == typ_arg.name: | ||
| subtype = builder.accept(expr.args[0]) | ||
| return builder.add(Call(ir.setup, [subtype], expr.line)) | ||
| return builder.call_c(setup_object, [subtype], expr.line) |
There was a problem hiding this comment.
Add a comment here explaining why a dynamic method dispatch is needed here.
There was a problem hiding this comment.
Can you add a fast path if class is known to not have subclasses -- we would still be able to use the old direct call, right?
| if isinstance(typ_arg, NameExpr) and len(method_args) > 0 and method_args[0] == typ_arg.name: | ||
| subtype = builder.accept(expr.args[0]) | ||
| return builder.add(Call(ir.setup, [subtype], expr.line)) | ||
| return builder.call_c(setup_object, [subtype], expr.line) |
There was a problem hiding this comment.
Can you add a fast path if class is known to not have subclasses -- we would still be able to use the old direct call, right?
mypyc/lib-rt/generic_ops.c
Outdated
| } | ||
| } | ||
| if (!def || !def->ml_name) { | ||
| PyErr_SetString(PyExc_LookupError, "Internal error: Unable to find object setup function"); |
There was a problem hiding this comment.
Add something to the error message that indicates that this error is related to mypyc, and not CPython.
mypyc/irbuild/specialize.py
Outdated
| if isinstance(typ_arg, NameExpr) and len(method_args) > 0 and method_args[0] == typ_arg.name: | ||
| subtype = builder.accept(expr.args[0]) | ||
| return builder.add(Call(ir.setup, [subtype], expr.line)) | ||
| classes = all_concrete_classes(ir) |
There was a problem hiding this comment.
ir.subclasses() seems like the correct way -- we need to use the slow path for an ABC that has only a single concrete subclass, I think?
There was a problem hiding this comment.
ok that makes sense. i didn't add a test for this because for now __new__ cannot be called in an abstract class anyway as it inherits from a python type.
Fixes an issue where a subclass would have its vtable pointer set to the base class' vtable when there is a `__new__` method defined in the base class. This resulted in the subclass constructor calling the setup function of the base class because mypyc transforms `object.__new__` into the setup function. The fix is to store the pointers to the setup functions in `tp_methods` of type objects and look them up dynamically when instantiating new objects. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
3 participants
Fixes an issue where a subclass would have its vtable pointer set to the base class' vtable when there is a
__new__method defined in the base class. This resulted in the subclass constructor calling the setup function of the base class because mypyc transformsobject.__new__into the setup function.The fix is to store the pointers to the setup functions in
tp_methodsof type objects and look them up dynamically when instantiating new objects.