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...)

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 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

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.

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.

+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.

> 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.

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.

> 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 :-)).

> 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)

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 :)

PEP 573 proposes an alternative to PyState_FindModule.

Let me summarize the original issue with the following quote from the python-dev thead:

> doing some sys.modules acrobatics and re-importing suddenly
changes the internal state of a previously imported [_csv] module.

The issue is now solved; both reproducers from that thread pass.

If you're interested, please read PEP 630 for a general overview of where we're at w.r.t. C-API changes that started with PEP 3121.
For the API used to fix this issue, see PEP 573; for the specific fix in _csv, see GH-23224.

Thanks to everyone involved, and especially to Nick Coghlan for helping me start this journey way back in 2013 :)