
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2016-08-17 06:04 byxiang.zhang, last changed2022-04-11 14:58 byadmin. This issue is nowclosed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| issue27782.patch | xiang.zhang,2016-08-17 17:50 | review | ||
| Messages (13) | |||
|---|---|---|---|
| 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. InPEP489, it is explicitly stated that "Py_mod_create slot is not responsible for setting import-related attributes specified inPEP 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 changeset913268337886 by Nick Coghlan in branch '3.5':Issue#27782: Fix m_methods handling in multiphase inithttps://hg.python.org/cpython/rev/913268337886New changesetfb509792dffc by Nick Coghlan in branch 'default':Merge#27782 fix from 3.5https://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.htmlWith 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. | |||
| History | |||
|---|---|---|---|
| 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 | |