Refactor Py2NRNString. (#3249)

The `Py2NRNString` consisted of (what's now) a `unique_cstr`. The ctor would
create the cstr, and `{g,s}et_pyerr` were non-static methods. They needed to
check if the cstr as a `nullptr`, but where only ever called after checking that
it was a `nullptr`.

This commit separates several notions:
  * Ownership is handled by `unique_cstr`.
  * An ASCII string is extracted from a Python object using `as_ascii`.
  * Both `{g,s}et_pyerr` were made static an are independent of each other
    and from converting a Python object to an (ASCII) C-string.
  * The `PyErr2NRNString` was removed.

src/neuron/unique_cstr.hpp

@@ -4,6 +4,8 @@
 #include <utility>
 #include <ostream>
 
+#include "nrnpython/nrn_export.hpp"
+
 namespace neuron {
 
 /** A RAII wrapper for C-style strings.
@@ -13,7 +15,7 @@ namespace neuron {
  * ownership, this is achieved using `.release()`, which returns the contained C-string and makes
  * this object invalid.
  */
-class unique_cstr {
+class NRN_EXPORT unique_cstr {
   public:
     unique_cstr(const unique_cstr&) = delete;
     unique_cstr(unique_cstr&& other) noexcept {

src/nrnpython/inithoc.cpp

@@ -113,8 +113,9 @@ static int add_neuron_options() {
             PySys_WriteStdout("A neuron_options key:value is not a string:string or string:None\n");
             continue;
         }
-        Py2NRNString skey(key);
-        Py2NRNString sval(value);
+
+        auto skey = Py2NRNString::as_ascii(key);
+        auto sval = Py2NRNString::as_ascii(value);
         if (strcmp(skey.c_str(), "-print-options") == 0) {
             rval = 1;
             continue;

src/nrnpython/nrnpy_hoc.cpp

@@ -419,21 +419,19 @@ int hocobj_pushargs(PyObject* args, std::vector<neuron::unique_cstr>& s2free) {
             hoc_pushx(nb::cast<double>(po));
         } else if (is_python_string(po.ptr())) {
             char** ts = hoc_temp_charptr();
-            Py2NRNString str(po.ptr(), /* disable_release */ true);
-            if (str.err()) {
+            auto str = Py2NRNString::as_ascii(po.ptr());
+            if (!str.is_valid()) {
                 // Since Python error has been set, need to clear, or hoc_execerror
                 // printing with Printf will generate a
                 // Exception ignored on calling ctypes callback function.
                 // So get the message, clear, and make the message
                 // part of the execerror.
-                auto err = neuron::unique_cstr(str.get_pyerr());
-                *ts = err.c_str();
-                s2free.push_back(std::move(err));
+                auto err = Py2NRNString::get_pyerr();
                 hoc_execerr_ext("python string arg cannot decode into c_str. Pyerr message: %s",
-                                *ts);
+                                err.c_str());
             }
             *ts = str.c_str();
-            s2free.push_back(neuron::unique_cstr(*ts));
+            s2free.push_back(std::move(str));
             hoc_pushstr(ts);
         } else if (PyObject_TypeCheck(po.ptr(), hocobject_type)) {
             // The PyObject_TypeCheck above used to be PyObject_IsInstance. The
@@ -1042,10 +1040,10 @@ static PyObject* hocobj_getattr(PyObject* subself, PyObject* pyname) {
 
     nb::object result;
     int isptr = 0;
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
     if (!n) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return nullptr;
     }
 
@@ -1433,10 +1431,10 @@ static int hocobj_setattro(PyObject* subself, PyObject* pyname, PyObject* value)
     if (self->type_ == PyHoc::HocObject && !self->ho_) {
         return 1;
     }
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
     if (!n) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return -1;
     }
     // printf("hocobj_setattro %s\n", n);
@@ -2244,9 +2242,10 @@ static PyObject* mkref(PyObject* self, PyObject* args) {
         } else if (is_python_string(pa)) {
             result->type_ = PyHoc::HocRefStr;
             result->u.s_ = 0;
-            Py2NRNString str(pa);
-            if (str.err()) {
-                str.set_pyerr(PyExc_TypeError, "string arg must have only ascii characters");
+            auto str = Py2NRNString::as_ascii(pa);
+            if (!str.is_valid()) {
+                Py2NRNString::set_pyerr(PyExc_TypeError,
+                                        "string arg must have only ascii characters");
                 return NULL;
             }
             char* cpa = str.c_str();
@@ -2301,10 +2300,11 @@ static PyObject* setpointer(PyObject* self, PyObject* args) {
             if (hpp->type_ != PyHoc::HocObject) {
                 goto done;
             }
-            Py2NRNString str(name);
+            auto str = Py2NRNString::as_ascii(name);
             char* n = str.c_str();
-            if (str.err()) {
-                str.set_pyerr(PyExc_TypeError, "POINTER name can contain only ascii characters");
+            if (!str.is_valid()) {
+                Py2NRNString::set_pyerr(PyExc_TypeError,
+                                        "POINTER name can contain only ascii characters");
                 return NULL;
             }
             Symbol* sym = getsym(n, hpp->ho_, 0);
@@ -2488,7 +2488,7 @@ static char* double_array_interface(PyObject* po, long& stride) {
     PyObject* psize;
     if (PyObject_HasAttrString(po, "__array_interface__")) {
         auto ai = nb::steal(PyObject_GetAttrString(po, "__array_interface__"));
-        Py2NRNString typestr(PyDict_GetItemString(ai.ptr(), "typestr"));
+        auto typestr = Py2NRNString::as_ascii(PyDict_GetItemString(ai.ptr(), "typestr"));
         if (strcmp(typestr.c_str(), array_interface_typestr) == 0) {
             data = PyLong_AsVoidPtr(PyTuple_GetItem(PyDict_GetItemString(ai.ptr(), "data"), 0));
             // printf("double_array_interface idata = %ld\n", idata);
@@ -2755,8 +2755,7 @@ static char** gui_helper_3_str_(const char* name, Object* obj, int handle_strptr
     if (gui_callback) {
         auto po = nb::steal(gui_helper_3_helper_(name, obj, handle_strptr));
         char** ts = hoc_temp_charptr();
-        Py2NRNString str(po.ptr(), true);
-        *ts = str.c_str();
+        *ts = Py2NRNString::as_ascii(po.ptr()).release();
         // TODO: is there a memory leak here? do I need to: s2free.push_back(*ts);
         return ts;
     }
@@ -3190,8 +3189,8 @@ char get_endian_character() {
         return 0;
     }
 
-    Py2NRNString byteorder(pbo.ptr());
-    if (byteorder.c_str() == NULL) {
+    auto byteorder = Py2NRNString::as_ascii(pbo.ptr());
+    if (!byteorder.is_valid()) {
         return 0;
     }
 
@@ -3295,9 +3294,9 @@ static char* nrncore_arg(double tstop) {
                 if (ts) {
                     auto arg = nb::steal(PyObject_CallObject(callable.ptr(), ts.ptr()));
                     if (arg) {
-                        Py2NRNString str(arg.ptr());
-                        if (str.err()) {
-                            str.set_pyerr(
+                        auto str = Py2NRNString::as_ascii(arg.ptr());
+                        if (!str.is_valid()) {
+                            Py2NRNString::set_pyerr(
                                 PyExc_TypeError,
                                 "neuron.coreneuron.nrncore_arg() must return an ascii string");
                             return nullptr;

src/nrnpython/nrnpy_nrn.cpp

@@ -409,9 +409,10 @@ static int NPySecObj_init(NPySecObj* self, PyObject* args, PyObject* kwds) {
                     Py_XDECREF(self->cell_weakref_);
                     return -1;
                 }
-                Py2NRNString str(cell_str.ptr());
-                if (str.err()) {
-                    str.set_pyerr(PyExc_TypeError, "cell name contains non ascii character");
+                auto str = Py2NRNString::as_ascii(cell_str.ptr());
+                if (!str.is_valid()) {
+                    Py2NRNString::set_pyerr(PyExc_TypeError,
+                                            "cell name contains non ascii character");
                     Py_XDECREF(self->cell_weakref_);
                     return -1;
                 }
@@ -1965,10 +1966,10 @@ static PyObject* section_getattro(NPySecObj* self, PyObject* pyname) {
     CHECK_SEC_INVALID(sec);
     PyObject* rv;
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
-    if (name.err()) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+    if (!name.is_valid()) {
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return nullptr;
     }
     // printf("section_getattr %s\n", n);
@@ -2025,10 +2026,10 @@ static int section_setattro(NPySecObj* self, PyObject* pyname, PyObject* value)
     PyObject* rv;
     int err = 0;
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
-    if (name.err()) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+    if (!name.is_valid()) {
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return -1;
     }
     // printf("section_setattro %s\n", n);
@@ -2179,10 +2180,10 @@ static PyObject* segment_getattro(NPySegObj* self, PyObject* pyname) {
 
     Symbol* sym;
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
-    if (name.err()) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+    if (!name.is_valid()) {
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return nullptr;
     }
     // printf("segment_getattr %s\n", n);
@@ -2322,10 +2323,10 @@ static int segment_setattro(NPySegObj* self, PyObject* pyname, PyObject* value)
     Symbol* sym;
     int err = 0;
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
-    if (name.err()) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+    if (!name.is_valid()) {
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return -1;
     }
     // printf("segment_setattro %s\n", n);
@@ -2472,10 +2473,10 @@ static PyObject* mech_getattro(NPyMechObj* self, PyObject* pyname) {
     CHECK_SEC_INVALID(sec)
     CHECK_PROP_INVALID(self->prop_id_);
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
     if (!n) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return nullptr;
     }
     // printf("mech_getattro %s\n", n);
@@ -2566,10 +2567,10 @@ static int mech_setattro(NPyMechObj* self, PyObject* pyname, PyObject* value) {
 
     int err = 0;
     auto _pyname_tracker = nb::borrow(pyname);  // keep refcount+1 during use
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
-    if (name.err()) {
-        name.set_pyerr(PyExc_TypeError, "attribute name must be a string");
+    if (!name.is_valid()) {
+        Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
         return -1;
     }
     // printf("mech_setattro %s\n", n);
@@ -2619,7 +2620,7 @@ neuron::container::generic_data_handle* nrnpy_setpointer_helper(PyObject* pyname
     NPyMechObj* m = (NPyMechObj*) mech;
     Symbol* msym = memb_func[m->type_].sym;
     char buf[200];
-    Py2NRNString name(pyname);
+    auto name = Py2NRNString::as_ascii(pyname);
     char* n = name.c_str();
     if (!n) {
         return nullptr;

src/nrnpython/nrnpy_p2h.cpp

@@ -172,9 +172,8 @@ static void py2n_component(Object* ob, Symbol* sym, int nindex, int isfunc) {
         for (i = 0; i < nindex; ++i) {
             nb::object arg = nb::steal(nrnpy_hoc_pop("isfunc py2n_component"));
             if (!arg) {
-                PyErr2NRNString e;
-                e.get_pyerr();
-                hoc_execerr_ext("arg %d error: %s", i, e.c_str());
+                auto err = Py2NRNString::get_pyerr();
+                hoc_execerr_ext("arg %d error: %s", i, err.c_str());
             }
             args.append(arg);
         }
@@ -231,8 +230,8 @@ static void py2n_component(Object* ob, Symbol* sym, int nindex, int isfunc) {
         hoc_pushx(d);
     } else if (is_python_string(result.ptr())) {
         char** ts = hoc_temp_charptr();
-        Py2NRNString str(result.ptr(), true);
-        *ts = str.c_str();
+        // TODO double check that this doesn't leak.
+        *ts = Py2NRNString::as_ascii(result.ptr()).release();
         hoc_pop_defer();
         hoc_pushstr(ts);
     } else {
@@ -363,7 +362,7 @@ static int hoccommand_exec_strret(Object* ho, char* buf, int size) {
     nb::object r = hoccommand_exec_help(ho);
     if (r.is_valid()) {
         nb::str pn(r);
-        Py2NRNString str(pn.ptr());
+        auto str = Py2NRNString::as_ascii(pn.ptr());
         strncpy(buf, str.c_str(), size);
         buf[size - 1] = '\0';
     } else {

src/nrnpython/nrnpy_utils.cpp

@@ -14,10 +14,8 @@ inline std::tuple<nb::object, nb::object, nb::object> fetch_pyerr() {
     return std::make_tuple(nb::steal(ptype), nb::steal(pvalue), nb::steal(ptraceback));
 }
 
-
-Py2NRNString::Py2NRNString(PyObject* python_string, bool disable_release) {
-    disable_release_ = disable_release;
-    str_ = NULL;
+neuron::unique_cstr Py2NRNString::as_ascii(PyObject* python_string) {
+    char* str_ = nullptr;
     if (PyUnicode_Check(python_string)) {
         auto py_bytes = nb::steal(PyUnicode_AsASCIIString(python_string));
         if (py_bytes) {
@@ -36,16 +34,13 @@ Py2NRNString::Py2NRNString(PyObject* python_string, bool disable_release) {
     } else {  // Neither Unicode or PyBytes
         PyErr_SetString(PyExc_TypeError, "Neither Unicode or PyBytes");
     }
+
+    return neuron::unique_cstr(str_);
 }
 
 void Py2NRNString::set_pyerr(PyObject* type, const char* message) {
-    nb::object err_type;
-    nb::object err_value;
-    nb::object err_traceback;
+    auto [err_type, err_value, err_traceback] = fetch_pyerr();
 
-    if (err()) {
-        std::tie(err_type, err_value, err_traceback) = fetch_pyerr();
-    }
     if (err_value && err_type) {
         auto umes = nb::steal(
             PyUnicode_FromFormat("%s (Note: %S: %S)", message, err_type.ptr(), err_value.ptr()));
@@ -55,48 +50,27 @@ void Py2NRNString::set_pyerr(PyObject* type, const char* message) {
     }
 }
 
-char* Py2NRNString::get_pyerr() {
-    if (err()) {
-        auto [ptype, pvalue, ptraceback] = fetch_pyerr();
-        if (pvalue) {
-            auto pstr = nb::steal(PyObject_Str(pvalue.ptr()));
-            if (pstr) {
-                const char* err_msg = PyUnicode_AsUTF8(pstr.ptr());
-                if (err_msg) {
-                    str_ = strdup(err_msg);
-                } else {
-                    str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8");
-                }
-            } else {
-                str_ = strdup("get_pyerr failed at PyObject_Str");
-            }
-        } else {
-            str_ = strdup("get_pyerr failed at PyErr_Fetch");
-        }
-    }
-    PyErr_Clear();  // in case could not turn pvalue into c_str.
-    return str_;
-}
+neuron::unique_cstr Py2NRNString::get_pyerr() {
+    // Must be called after an error happend.
+    char* str_ = nullptr;
 
-char* PyErr2NRNString::get_pyerr() {
-    if (PyErr_Occurred()) {
-        auto [ptype, pvalue, ptraceback] = fetch_pyerr();
-        if (pvalue) {
-            auto pstr = nb::steal(PyObject_Str(pvalue.ptr()));
-            if (pstr) {
-                const char* err_msg = PyUnicode_AsUTF8(pstr.ptr());
-                if (err_msg) {
-                    str_ = strdup(err_msg);
-                } else {
-                    str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8");
-                }
+    auto [ptype, pvalue, ptraceback] = fetch_pyerr();
+    if (pvalue) {
+        auto pstr = nb::steal(PyObject_Str(pvalue.ptr()));
+        if (pstr) {
+            const char* err_msg = PyUnicode_AsUTF8(pstr.ptr());
+            if (err_msg) {
+                str_ = strdup(err_msg);
             } else {
-                str_ = strdup("get_pyerr failed at PyObject_Str");
+                str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8");
             }
         } else {
-            str_ = strdup("get_pyerr failed at PyErr_Fetch");
+            str_ = strdup("get_pyerr failed at PyObject_Str");
         }
+    } else {
+        str_ = strdup("get_pyerr failed at PyErr_Fetch");
     }
+
     PyErr_Clear();  // in case could not turn pvalue into c_str.
-    return str_;
+    return neuron::unique_cstr(str_);
 }