Use PyModule_AddObjectRef over PyModule_AddObject - #1059

1. Create ADD_MODULE_REF macro that will abstract away this check + goto error
2. In error: we can clean up all temporary references
3. We should define these references at the top set as NULL
4. Py_XDECREF them on error :)

Can you explain 4? Do you mean on the error check we're doing on the PyModule_AddObjectRef call, or on errors later on?

@provinzkraut

pseudo-code :)

PyObject *temp_module = NULL;
PyObject *temp_obj = NULL;

#define SET_MODULE_REF(ATTR, VALUE) \
  do { \
      if (PyModule_AddObjectRef(m, (ATTR), (VALUE)) < 0) \
          goto error; \
  } while (0)

#define IMPORT_TEMP_MODULE(MOD_NAME) \
  do { \
     Py_XDECREF(temp_module); // clean other existing temp module \
     temp_module = PyImport_ImportModule((MOD_NAME));
     if (temp_module == NULL) goto error;
  } while (0)

SET_MODULE_REF("JSONDecoder", (PyObject *)&JSONDecoder_Type);
// ...
IMPORT_TEMP_MODULE("typing");
SET_REF(typing_union, "Union");

error:
    Py_XDECREF(temp_module);
    Py_XDECREF(temp_obj);

Reviewed and tested this (built the branch, full unit suite passes). The swap is refcount-correct everywhere, and on failure it is actually safer than before (the old Py_INCREF + PyModule_AddObject leaked on error). It also fixes a small real bug: Field was added without a Py_INCREF, so it under-counted Field_Type by one on import (harmless only because it is a static type). AddObjectRef fixes that.

On @sobolevn's macro idea: I like it, but I would do it in a separate PR. The goto error + temp cleanup is a different job (it also fixes the temp_module leaks on the import paths), and those leaks only trigger on init failure, which is practically unreachable. No need to hold this clean change for it.

LGTM to merge as a first step.

@sobolevn I like the macro idea, but I agree with @Siyet that I'd prefer doing that in a separate PR