upgrade_pythoncapi.py: Wrong transformations for Py_Is

[skirpichev]

Consider an example:

if (((PyObject*)obj)->data == Py_None) {
    111;
}
if (Py_None == ((PyObject*)obj)->data) {
    111;
}

After upgrade_pythoncapi.py I got:

#include "pythoncapi_compat.h"

if (((PyObject*)obj)->Py_IsNone(data)) {
    111;
}
if (Py_None == ((PyObject*)obj)->data) {
    111;
}

Real world example:
https://github.com/aleaxit/gmpy/blob/eb8dfcbd84abcfcb36b4adcb0d5c6d050731dd75/src/gmpy2_xmpz_misc.c#L237

I'm not sure if this is a bug. This kind of issues is not easy to avoid, using regexps for code transformations. Have you considered to use something like the pycparser?

Thanks for the project.

[vstinner]

I'm not sure if this is a bug.

It's a bug :-)

This kind of issues is not easy to avoid, using regexps for code transformations. Have you considered to use something like the pycparser?

Many parsers cannot be used to modify source code, only to parse it. This project needs to modify the C code.

I know that the regex approach causes such surprising issues, but usually a very low number of lines is impacted, and it's quick to fix it by hand.

A better approach would be to use https://coccinelle.gitlabpages.inria.fr/website/ which is used in the Linux kernel for refactoring. It uses a DSL to match code and then replace code with an expression.

I tried to avoid a dependency to coccinelle to make the project easier to install (only need Python). If too many users complain, maybe I should consider moving to coccinelle.

[ncoghlan]

I wondered if zig might offer a different path to allowing C source transformations without introducing a non-Python dependency, but ziglang/zig#2082 indicates that's not likely to be a viable option.

Reading https://src.fedoraproject.org/rpms/coccinelle/blob/rawhide/f/coccinelle.spec makes it clear why you're reluctant to add a hard dependency, though. When the nicest potential approach I can see involves a git submodule, cibuildwheel, and an OCaml dependency for source builds... yikes.

If the dual maintenance burden wouldn't be too bad, maybe coccinelle could be used if shutil.which finds it, and otherwise fall back to the existing regex based approach?