-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] Add a mypyc_attr to support interpreted children #8004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a bunch of minor comments. This is very important for enabling mixed compiled+interpreted use cases, such as compiling existing Python libraries, and gradually compiling large code bases.
# "shadow vtable" that contains implementations that delegate to | ||
# making a pycall, so that overridden methods in interpreted children | ||
# will be called. (A better strategy could dynamically generate these | ||
# vtables based on which methods are overridden in the children.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is dynamic generation possible when there could be monkey patching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think probably. Monkey patching without incurring a dict lookup in the non-monkey-patched case is gonna require some heroics anyway (I think a metaclass that catches writes and updates the vtable or something similar) and I think they will all apply here too
mypyc/test-data/commandline.test
Outdated
@@ -170,3 +174,11 @@ def f(l: List[object]) -> None: | |||
for i in l: | |||
if x is None: | |||
x = i | |||
|
|||
@mypyc_attr(allow_interpreted_children=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: Python documentation doesn't talk about children; it uses 'subclasses' and 'derived classes'. Maybe rename the flag to interpreted_subclasses=True
or something? I think that the allow_
prefix can also be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the allow_
prefix is important because otherwise it could be read as causing all subclasses to be interpreted?
# children. | ||
if class_ir.allow_interpreted_children: | ||
f = self.gen_glue(func_ir.sig, func_ir, class_ir, class_ir, fdef, do_py_ops=True) | ||
class_ir.glue_methods[(class_ir, name)] = f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a class that allow interpreted children has a native subclass? Will these glue methods cause trouble?
I wonder if putting the python glue methods in a separate data structure would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't cause trouble
This operates by generating a "shadow vtable" containing pointers to glue methods that dispatch to the appropriate method via the C API. We then install those shadow vtables in interpreted subclasses so that overridden methods will be called. This does not support directly inheriting from traits, which I think will require generating vtables dynamically (and maybe some more nonsense too.) Closes #296. (Though I will file a follow-up for traits.)
This operates by generating a "shadow vtable" containing pointers to
glue methods that dispatch to the appropriate method via the C API. We
then install those shadow vtables in interpreted subclasses so that
overridden methods will be called.
This does not support directly inheriting from traits, which I think
will require generating vtables dynamically (and maybe some more
nonsense too.)
Closes #296. (Though I will file a follow-up for traits.)