Add PyState_GetModuleAttr

[pitrou]

BPO	18710
Nosy	@loewis, @ncoghlan, @pitrou, @vstinner, @tiran, @encukou
Files	getmodattr.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-12-15.15:17:34.091>
created_at = <Date 2013-08-12.12:40:41.321>
labels = ['extension-modules', 'interpreter-core', 'type-feature']
title = 'Add PyState_GetModuleAttr'
updated_at = <Date 2020-12-15.15:17:34.090>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2020-12-15.15:17:34.090>
actor = 'petr.viktorin'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-15.15:17:34.091>
closer = 'petr.viktorin'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2013-08-12.12:40:41.321>
creator = 'pitrou'
dependencies = []
files = ['31244']
hgrepos = []
issue_num = 18710
keywords = ['patch']
message_count = 13.0
messages = ['194949', '194953', '194954', '194955', '194956', '194957', '194960', '194962', '194963', '194964', '194965', '352319', '383065']
nosy_count = 7.0
nosy_names = ['loewis', 'ncoghlan', 'pitrou', 'vstinner', 'christian.heimes', 'eli.bendersky', 'petr.viktorin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18710'
versions = ['Python 3.4']

[pitrou]

Attached patch adds PyState_GetModuleAttr() and converts the _csv module to use it (as an example).

As you can see, the _csv module grows a little but it now has proper error handling (previously, it didn't check for PyState_FindModule() returning NULL).

(A few lines would be removed if we added PyErr_FormatV as well...)

[vstinner]

I don't understand your change. Why do we need to change the _csv module and why storing module global variables in the module dict is better than storing them in a simple C structure?

[pitrou]

I don't understand your change. Why do we need to change the _csv
module and why storing module global variables in the module dict is
better than storing them in a simple C structure?

I won't repeat what was already said in the python-dev thread:
http://mail.python.org/pipermail/python-dev/2013-August/127862.html

[tiran]

The patch contains the new function and a patch for the CSV module. How about you split it up across two patches: one for the new feature and one for the CSV module? It makes review easier.

[pitrou]

Bundling the two was aimed at showcasing the effect the new function can have. The function itself is trivial, there's not much point in reviewing it alone.

[vstinner]

+PyObject *
+PyState_GetModuleAttr(struct PyModuleDef *def,
+ const char *name,
+ PyObject *restrict_type)

When the char* type is used, the function has usually the suffix String. I prefer the PyIdentifier API because it avoids to create a temporary Python Unicode object.

It's the first type that I see a function getting an attribute which also checks for its type. I don't know if the check should be done in PyState_GetModuleAttr() or in the _csv module.

[pitrou]

When the char* type is used, the function has usually the suffix
String. I prefer the PyIdentifier API because it avoids to create a
temporary Python Unicode object.

This is a convenience API, not a performance "optimization".

It's the first type that I see a function getting an attribute which
also checks for its type. I don't know if the check should be done
in PyState_GetModuleAttr() or in the _csv module.

The aim is to help writing extension modules, which is why the type
check is done inside the helper function.

[elibendersky]

The patched code looks better than the original one in several respects, but I think it raises some questions.

I agree with Victor that the type-checking API is unnatural, but I also think there may be a deeper issue hiding behind. You felt compelled to add the type-checking to provide some level of safety to the C code - but is it enough? Previously, the only way to add a dialect was through register_dialect that does type checking to make sure it gets a legit dialect object. Now, the _dialects dict is directly accessible to Python code and it can add arbitrary objects to it (both as keys and as values). Does this mean that the C code now has to do type checking in all internal code that accesses _dialects? I haven't had time to find a crasher example, but it's plausible that it's possible to crash the interpreter now by assigning un-expected stuff to members of _dialects.

Maybe it's not a problem in practice for _csv but something worth thinking about if we're going to switch other methods to this approach.

Also, the problem with http://mail.python.org/pipermail/python-dev/2013-August/127862.html does not go away with this patch, as expected. However, it's a step in the right direction in case we do have multiple instances of the extension module alive at the same time in the future. Although then it would be interesting to consider how to find the actually correct module instance from internal functions.

[pitrou]

Previously, the only way to add a dialect was through register_dialect that does
type checking to make sure it gets a legit dialect object. Now, the _dialects dict is
directly accessible to Python code and it can add arbitrary objects to it (both as
keys and as values). Does this mean that the C code now has to do type checking in all
internal code that accesses _dialects?

You are right, I forgot about that part. That will make the patch significantly more complicated.

However, it's a step in the right direction in case we do have multiple instances of
the extension module alive at the same time in the future. Although then it would be
interesting to consider how to find the actually correct module instance from internal
functions.

This sounds basically impossible (which is why we *can't* have multiple instances of an extension module alive in a single interpreter :-)).

[elibendersky]

Previously, the only way to add a dialect was through register_dialect
that does

> type checking to make sure it gets a legit dialect object. Now, the
_dialects dict is
> directly accessible to Python code and it can add arbitrary objects to
it (both as
> keys and as values). Does this mean that the C code now has to do type
checking in all
> internal code that accesses _dialects?

You are right, I forgot about that part. That will make the patch
significantly more complicated.

:-/

Turns out there's real merit in having a clear dividing line between the
safe world of Python and unsafe world of C. Within the C code, some
invariants are assumed to be correct (and can be efficiently assert()-ed) -
this makes the code simpler and more performant.

> However, it's a step in the right direction in case we do have multiple
instances of
> the extension module alive at the same time in the future. Although then
it would be
> interesting to consider how to find the actually correct module instance
from internal
> functions.

This sounds basically impossible (which is why we *can't* have multiple
instances of an extension module alive in a single interpreter :-)).

Well, it surely isn't possible without significant code rewriting. But I
can envision carrying such "state" around in extension-specific objects.
Although it's not entirely clear whether the memory cost is justified.

But without it, it's not clear to me how we plan to have multiple living
extension modules at all. Perhaps this is a discussion outside this
specific issue though (maybe for the PEP Stefan writes)

[ncoghlan]

Breaking the 1:1 interpreter <-> extension module mapping involves
adding custom types to sys.modules rather than module objects. Making
that work sensibly will involve larger changes to the extension
initialisation APIs. import-sig already has plans for this :)

[encukou]

PEP-573 proposes an alternative to PyState_FindModule.