msg272903 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-17 06:04 |
From doc [1], when create_module returns a non-module instance, m_methods, m_traverse, m_clear, m_free must be NULL. But actually in the codes, only m_traverse, m_clear, m_free are checked and emitting consistent errors. If m_methods is NULL, it will fail in [2] and emit an inconsistent misleading argument error. And what's more confusing is, in [3], it says "regardless of type, the module's functions are initialized from m_methods, if any", which I think conflicts with the codes and doc.
[1] https://docs.python.org/3.6/c-api/module.html#c.Py_mod_create
[2] https://hg.python.org/cpython/file/tip/Objects/moduleobject.c#l300
[3] https://www.python.org/dev/peps/pep-0489/#post-creation-steps
|
msg272906 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2016-08-17 06:52 |
I think the intended behaviour here is that which was documented in the PEP: that m_methods should still work based on ducktyping for reading a __name__ attribute and setting the method attributes, even if the result of Py_create_mod isn't a module type object.
However, it isn't currently working due to the PyModule_Check call in PyModule_GetNameObject, which is in turn called from PyModule_AddFunctions. The Py_mod_create docs then reflect that limitation of the implementation.
The cleanest way to refactor and fix this that comes to mind would be to make static _get_object_name() and _add_methods_to_object() functions in moduleobject.c (which omit any strict type checks), and then call those from PyModule_GetNameObject and PyModule_AddFunctions with the explicit typecheck.
That way we don't have to support this for arbitrary third party code calling in to the C API, while still allowing it for the specific case of objects returned from Py_mod_create.
|
msg272907 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-17 07:11 |
> The cleanest way to refactor and fix this that comes to mind would be to make static _get_object_name() and _add_methods_to_object() functions in moduleobject.c (which omit any strict type checks), and then call those from PyModule_GetNameObject and PyModule_AddFunctions with the explicit typecheck.
That's one solution. How about loosing PyModule_GetNameObject's constraint? Let it accept non-ModuleType objects? Actually without the constraint of PyModule_GetNameObject, PyModule_AddFunctions can handle non-ModuleType objects.
|
msg272908 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2016-08-17 07:26 |
Loosening the constraint on PyModule_GetNameObject would indeed work, but it means the code still has a readability problem: the convention in the C API is that officially ducktyped APIs use the PyObject_* prefix, or one of the other abstract protocols (PyNumber_*, PyMapping_*, etc), rather than a concrete type name like PyModule_*. Relying on folks to "just know" that these particular APIs deliberately don't enforce the type constraint is a recipe for future confusion, even if it's documented that way.
Such a change also has potential ripple effects on other implementations that emulate the C API, and hence isn't something I'd be comfortable with changing in a maintenance release, whereas fixing the implementation to match the PEP could be done for the next 3.5.x update.
|
msg272909 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-17 07:38 |
Nice point. I'll write a patch to fix this these days.
|
msg272957 - (view) |
Author: Petr Viktorin (petr.viktorin) *  |
Date: 2016-08-17 14:55 |
Hi! I'm on a tight schedule this week, so I'm not looking into this in detail. But please let me know if you need any help and I'll raise the priority.
|
msg272977 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-17 17:50 |
Thanks Petr. I'd appreciate it if you are willing to review the patch.
Upload a patch to fix this, along with tests and doc updating.
But here is something different. In PEP489, it is explicitly stated that "Py_mod_create slot is not responsible for setting import-related attributes specified in PEP 451 (such as __name__ or __loader__ ) on the new module". So when an object(even ModuleType instances) is returned, it's __name__ attribute is not set and we can't rely on it (which means we can't even use PyModule_GetNameObject). I then use the name attribute of the spec. Looking forward to feedback.
|
msg273269 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2016-08-21 05:57 |
The patch looks good to me, and the relevant contributor licensing is in place, so I'll be applying this shortly :)
|
msg273275 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-08-21 07:44 |
New changeset 913268337886 by Nick Coghlan in branch '3.5':
Issue #27782: Fix m_methods handling in multiphase init
https://hg.python.org/cpython/rev/913268337886
New changeset fb509792dffc by Nick Coghlan in branch 'default':
Merge #27782 fix from 3.5
https://hg.python.org/cpython/rev/fb509792dffc
|
msg273276 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2016-08-21 07:46 |
Thanks for the bug report and the patch!
These kinds of collaborative interactions are my favourite aspect of open source participation :)
|
msg273280 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-08-21 08:21 |
Thanks for your work too, Nick! :) Active reply from the core devs always gives me more motivation to open source.
|
msg274762 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2016-09-07 05:32 |
Xiang, if multi-phase initialisation is an area you're interested in, it occurs to me you may also want to take a look at Petr's proposal to provide efficient access to global module state from methods of extension types: https://mail.python.org/pipermail/import-sig/2016-August/001065.html
With the 3.6 feature freeze deadline imminent it's looking like that will be deferred to 3.7, but it's still a significant improvement that should encourage greater use of the newer more submodule and reloading friendly approach to C extension module initialisation.
|
msg274767 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
Date: 2016-09-07 06:06 |
Thanks for your notice, Nick. :) Of course I am interested. I'll start following import-sig and reading Petr's good idea.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:34 | admin | set | github: 71969 |
2016-09-07 06:06:00 | xiang.zhang | set | messages:
+ msg274767 |
2016-09-07 05:32:42 | ncoghlan | set | messages:
+ msg274762 |
2016-08-21 08:21:19 | xiang.zhang | set | messages:
+ msg273280 |
2016-08-21 07:46:56 | ncoghlan | set | status: open -> closed resolution: fixed messages:
+ msg273276
stage: commit review -> resolved |
2016-08-21 07:44:13 | python-dev | set | nosy:
+ python-dev messages:
+ msg273275
|
2016-08-21 05:57:07 | ncoghlan | set | messages:
+ msg273269 stage: commit review |
2016-08-21 05:47:34 | ncoghlan | set | assignee: ncoghlan |
2016-08-17 17:50:02 | xiang.zhang | set | files:
+ issue27782.patch keywords:
+ patch messages:
+ msg272977
|
2016-08-17 14:55:49 | petr.viktorin | set | messages:
+ msg272957 |
2016-08-17 07:38:34 | xiang.zhang | set | messages:
+ msg272909 |
2016-08-17 07:26:17 | ncoghlan | set | messages:
+ msg272908 |
2016-08-17 07:11:00 | xiang.zhang | set | messages:
+ msg272907 |
2016-08-17 06:52:38 | ncoghlan | set | messages:
+ msg272906 |
2016-08-17 06:04:37 | xiang.zhang | create | |