-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] Wrap async functions with function-like type #20260
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
889a267 to
585f818
Compare
JukkaL
left a comment
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.
Some return values aren't checked (not a full review).
mypyc/lib-rt/CPyFunction.c
Outdated
| PyMethodDef *method = CPyMethodDef_New(funcname, func, func_flags, func_doc); | ||
| PyObject *code = CPyCode_New(filename, funcname, first_line, code_flags); | ||
| PyObject *op = (PyObject *)CPyFunction_Init(PyObject_GC_New(CPyFunction, CPyFunctionType), | ||
| method, PyUnicode_FromString(funcname), module, code); |
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.
Check return values of all function calls that may fail due to out of memory.
| } | ||
|
|
||
| static PyMethodDef* CPyMethodDef_New(const char *name, PyCFunction func, int flags, const char *doc) { | ||
| PyMethodDef *method = (PyMethodDef *)PyMem_Malloc(sizeof(PyMethodDef)); |
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.
Check return value.
| } | ||
|
|
||
| static PyObject* CPyCode_New(const char *filename, const char *funcname, int first_line, int flags) { | ||
| PyCodeObject *code_obj = PyCode_NewEmpty(filename, funcname, first_line); |
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.
Check return value.
mypyc/codegen/emitmodule.py
Outdated
| wrapper_name = emitter.emit_cpyfunction_instance(fn, filepath) | ||
| name_obj = f'PyUnicode_FromString("{fn.name}")' | ||
| emitter.emit_line( | ||
| f"if(PyDict_SetItem({globals}, {name_obj}, (PyObject *){wrapper_name}) < 0)" |
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.
Style nit: add space after if, before (.
mypyc/codegen/emitmodule.py
Outdated
|
|
||
| filepath = self.source_paths[module.fullname] | ||
| wrapper_name = emitter.emit_cpyfunction_instance(fn, filepath) | ||
| name_obj = f'PyUnicode_FromString("{fn.name}")' |
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.
Check return value.
mypyc/codegen/emitclass.py
Outdated
| filepath = emitter.filepath or "" | ||
| wrapper_name = emitter.emit_cpyfunction_instance(fn, filepath) | ||
| emitter.emit_line( | ||
| f'if (PyDict_SetItem(tp->tp_dict, PyUnicode_FromString("{fn.name}"), {wrapper_name}) < 0)' |
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.
Check return value of PyUnicode_FromString.
|
|
||
| code_flags = "CO_COROUTINE" | ||
| self.emit_line( | ||
| f'PyObject* {wrapper_name} = CPyFunction_New({module}, "{filepath}", "{name}", {cfunc}, {func_flags}, {doc}, {fn.line}, {code_flags});' |
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.
Check the return value of CPyFunction_New.
JukkaL
left a comment
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.
Another round of review -- still not a full review.
| @@ -0,0 +1,235 @@ | |||
| #define PY_SSIZE_T_CLEAN | |||
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.
Style nit: the file name is inconsistent. Use snake_case.c, such as function_wrapper.c.
mypyc/lib-rt/CPyFunction.c
Outdated
| int first_line, int code_flags) { | ||
| if (!CPyFunctionType) { | ||
| CPyFunctionType = (PyTypeObject *)PyType_FromSpec(&CPyFunction_spec); | ||
| assert(CPyFunctionType); |
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.
If this fails, return NULL instead of failing an assertion. Another option, since this will likely only fail if running out of memory, is to call CPyError_OutOfMemory on NULL return which aborts.
| } | ||
|
|
||
| static PyObject* CPyFunction_repr(CPyFunction *op) { | ||
| return PyUnicode_FromFormat("<function %U at %p>", op->func_name, (void *)op); |
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.
Test this in a run test.
| static void CPyFunction_dealloc(CPyFunction *m) { | ||
| PyObject_GC_UnTrack(m); | ||
| if (CPyFunction_weakreflist(m) != NULL) | ||
| PyObject_ClearWeakRefs((PyObject *) m); |
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.
Maybe test a weak reference in a function object in a run test?
| assert is_coroutine(identity_async) | ||
|
|
||
| assert not is_coroutine(wrapped) | ||
| assert is_coroutine(wrapped_async) |
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.
Test also calling the functions that use the new wrapper, just in case. I'm not sure if all of these use cases have a similar existing test case.
JukkaL
left a comment
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.
This concludes my first review pass. I'll do another lighter weight pass after the current round of comments has been addressed.
This is an important feature for mypyc, and I think a fairly complex implementation is the only way to address compatibility issues. However, the complexity makes this hard to test and review, but this seems unavoidable.
| "__annotations__": cpyfunction_get_annotations, | ||
| "__defaults__": cpyfunction_get_defaults, | ||
| "__kwdefaults__": cpyfunction_get_kwdefaults, | ||
| } |
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.
Add at least basic testing of all the new properties. Also test the attributes of the wrapper objects when relevant. Simple tests are sufficient, but it would be good that each code path has at least one test that explicitly uses it.
JukkaL
left a comment
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.
Thanks for the updates!
fdf1e97 to
816924d
Compare
Currently, introspection functions from the
inspectmodule likeiscoroutinefunction(fn)don't return the expected result for functions compiled with mypyc. mypyc defines the functions usingPyMethodDefin C for which cpython makes objects with typePyCFunction_Typeand notPyFunction_Type. The latter corresponds totypes.FunctionTypein python. So theisfunction(fn)check fails.iscoroutinefunction(fn)and some others accept function-like objects as long as they are callable and have some required attributes to support compiled functions.The most important attribute is
__code__which is a code object that stores flags for the function. For example foriscoroutinefunction(fn), theCO_COROUTINEflag is checked.In this PR, mypyc adds a wrapper
CPyFunctionwhose__call__calls the same function that would be defined withPyMethodDef. The wrapper is added to__dict__of either the module (for functions) or the containing class (for methods) with the original name of the function so the wrapper is returned in place of thePyCFunction_Typeobjects.For nested functions, mypyc already creates a callable class so the function-like attributes are added as properties. A static
CPyFunctionobject is created separately for each class that is used in the getter and setter methods of the callable class.For now
CPyFunctions are created only for async functions to supportiscoroutinefunction(fn)but the implementation could be extended to support other introspection functions in future PRs. For non-async functions the wrapper does not seem necessary for now because the default behavior makes the function always return false. So the behavior is as-expected for non-async functions.This implementation was inspired by Cython which generates much richer function wrappers.