capi-workgroup / decisions

Discussion and voting on specific issues
4 stars 1 forks source link

C API for simplifying module attribute declaration #6

Closed erlend-aasland closed 5 months ago

erlend-aasland commented 5 months ago

Some years ago, Christian created an issue, and a proposed implementation, for C API that could reduce the amount of boiler-plate code needed when implementing extension modules. Quoting the issue description:

It's currently inconvenient and sometimes error-prone to initialize module members in C. Even stdlib modules are sometimes missing error checks or have reference counting issues in error path. I propose add three helpers to deal with common cases:

1) declaration of simple attributes (int, str, bool, float) in an array of structs 2) creation and addition of a new type from a type spec 3) creation and addition a new exception object

The issue and PR received some initial interest (and I remember being positive to adding these helpers), but the discussion came to a halt after Christian took a break from CPython. Can the C API WG help decide if we should add such APIs? If you suggest these APIs are worth it to implement and maintain, I can volunteer to pick up Christan's work update the PR. If you suggest this is not worth it, we can close the issue and the PR.

encukou commented 5 months ago

Hello, This is sitting in my TODO list, but I wanted to tackle it in a roundabout way:

AFAIK, Christian's less controversial proposals were merged, and the remaining PR has the ones with potential issues.

vstinner commented 5 months ago

Extract of proposed PyModule_AddConstants() API:

enum _PyModuleConst_type {
    _PyModuleConst_none_type = 1,
    _PyModuleConst_long_type = 2,
    _PyModuleConst_ulong_type = 3,
    _PyModuleConst_bool_type = 4,
    _PyModuleConst_double_type = 5,
    _PyModuleConst_string_type = 6,
    _PyModuleConst_call_type = 7,
};

typedef struct PyModuleConst_Def {
    const char *name;
    enum _PyModuleConst_type type;
    union {
        const char *m_str;
        long m_long;
        unsigned long m_ulong;
        double m_double;
        PyObject* (*m_call)(PyObject *module);
    } value;
} PyModuleConst_Def;

PyAPI_FUNC(int) PyModule_AddConstants(PyObject *, PyModuleConst_Def *);

#define PyModuleConst_None(name) \
    {(name), _PyModuleConst_none_type, {.m_long=0}}
#define PyModuleConst_Long(name, value) \
    {(name), _PyModuleConst_long_type, {.m_long=(value)}}

I have some concerns:

Maybe it's ok to not expose such "convenient API" in the stable ABI and make it usable for other programming languages, since it's only a layer on top of the existing API such as PyModule_AddIntConstant().

vstinner commented 5 months ago

PyAPI_FUNC(PyTypeObject *) PyModule_AddNewTypeFromSpec(PyObject *module, PyType_Spec *spec, PyObject *base);

Python has many functions to create a type from a spec. I would prefer to not duplicate them in the PyModule_Add API family. Can't we add types in PyModule_AddConstants() API instead?

Overall, I like the "PyModule_AddConstants" API. It's just difficult to make it generic and support the stable ABI and most programming languages.

For the simple cases, maybe we can pass an array of PyModuleConst_Def as a "slot" in PyModuleDef.m_slots, to even avoid having to call explicitly PyModule_AddConstants().

gvanrossum commented 5 months ago

So PyModule_AddConstants is just a convenience, right? You could just as well write a bunch of code that creates the objects and adds them to the module one by one -- it's just a bit of boilerplate.

So on the one hand that's a reason why the "but what about Rust" argument doesn't bother me too much -- Rust code can do what it needs to do, and they could even define their own type-safe macros.

OTOH, I'm not convinced that this is a big enough issue to warrant the complexity of yet another convenience API. I'd say that modules with lots of simple global constants are probably more a code smell than a pattern we want to encourage? At the very least it should be just as easy to add a list of such constants to another namespace (e.g. a class or perhaps an enum).

erlend-aasland commented 5 months ago

FTR, I'm not interested in following up the issue or PR. Currently we use C macros for simplifying extension module initialization; I'm fine with that. ISTM there is little interest in driving this forward, so I would suggest to close the python/cpython issue/PR as not-planned. If/when Christan comes back to CPython and wishes to follow this up, he can easily reopen the issue and rebase his branch.

encukou commented 5 months ago

Not just Christian. It might well be me. But I agree that there's no reason to keep the issue on CPython's tracker.

Proposal: close the PR and issue.

Since this is easy to revert and I don't see anyone opposed, I'll go ahead and close them. We can reopen if anyone disagrees.

We can close this issue after everyone votes.

vstinner commented 5 months ago

The current API to initialize a module is really annoying to use, since each call must be checked for error. I'm interested by anything which make it easier.

Currently, since it's annoying to check for errors, many stdlib C extensions simply don't check for errors. That's bad. Others use complex macros to inject error handling ("return" or "goto" on error), without making the code too verbose to use.

erlend-aasland commented 5 months ago

The current API to initialize a module is really annoying to use, since each call must be checked for error.

That goes for pretty much any API; most APIs can fail, so you have to add error handling.

vstinner commented 5 months ago

@erlend-aasland:

FTR, I'm not interested in following up the issue or PR.

Oh, I first understood that you was interested to pursue the effort. Sadly, without any sponsor, this issue is not going anywhere. Ok to close it.