neuronapi: add nrn_symbol_pop; make nrn_object_pop nil-safe (#3836)

* neuronapi: add nrn_symbol_pop and nrn_object_pop_safe

Unwinding an interpreter frame from a binding -- the HOC-to-Python
component read/write-back path (pyobj.attr) -- needs two stack pops the
public API does not have, so it reaches for the mangled internals instead.

nrn_symbol_pop wraps hoc_spop: the interpreter leaves the attribute's
Symbol on the stack during object-component access, and unwinding that
frame has to pop it.

nrn_object_pop_safe is nrn_object_pop but nil-tolerant. nrn_object_pop
unconditionally does (*obptr)->refcount++ to hand back a reference, which
dereferences NULL when the stack entry is a nil objref (an unset objref
RHS in a write-back, say). The safe variant guards that one ref; the
stack-slot handling is otherwise identical, and a non-NULL result is still
reffed and should be unref'd.

test/api/stack_pops.cpp pushes symbols and pops them LIFO, round-trips a
real object through nrn_object_pop_safe, and confirms a nil objref pops to
NULL rather than crashing, with the stack left balanced.

* neuronapi: make nrn_object_pop nil-safe; drop nrn_object_pop_safe

Address review on #3836. Rather than adding a parallel nrn_object_pop_safe,
fold the nil guard into nrn_object_pop itself: it now returns NULL for a nil
object reference (an unset objref) instead of dereferencing NULL to bump the
refcount. The crash-on-nil behavior was not useful to any caller, so this is
strictly more permissive -- identical for a real object, NULL instead of a
segfault for nil. All 12 api:: tests pass.

nrn_symbol_pop is unchanged. Docs and the stack_pops test updated to the
single function.

* stack_pops test: sentinel-below stack-hygiene check

Address review on #3836 (same fix as #3828's setpointer test): the trailing
'push 5.0, pop, expect 5.0' proved nothing -- a stack returns the most
recently pushed item, so a sentinel pushed after the pops only probes the
current top. Push the sentinel FIRST, before any symbol/object push, and
recover it at the end; that proves the pops neither over-popped into what lay
beneath nor left anything behind.

Author: ec1dev

docs/capi.rst

@@ -871,6 +871,18 @@ Functions, objects, and the stack
 
     :param sym: Pointer to the symbol to push.
 
+.. c:function:: Symbol* nrn_symbol_pop(void)
+
+    Pop a Symbol from the top of the stack.
+
+    The interpreter puts a Symbol on the stack when accessing an object
+    component (reading or assigning ``pyobj.attr``); a binding that unwinds
+    such a stack frame uses this to pop the attribute's Symbol. Use
+    :c:func:`nrn_stack_type` to confirm the top is a ``STACK_IS_SYM`` entry
+    before popping.
+
+    :returns: The Symbol from the top of the stack.
+
 .. c:function:: int nrn_symbol_type(const Symbol* sym)
 
     Get the type of a symbol (e.g., function, variable, mechanism).
@@ -1126,13 +1138,20 @@ Functions, objects, and the stack
 
     Pop an object from the stack.
 
-    :returns: Pointer to object from the top of the stack.
+    Returns ``NULL`` for a nil object reference (an unset ``objref``) rather than
+    crashing, so it is safe to use when unwinding a stack that may carry a nil
+    object -- for example a HOC-to-Python write-back whose right-hand side is an
+    unset ``objref``.
+
+    :returns: Pointer to the object from the top of the stack, or ``NULL`` if it
+        is a nil object reference.
 
     **Usage Pattern:**
 
     Used to retrieve function/method return values. Use :c:func:`nrn_stack_type` to check the type
     before popping, or use the type of the function/method to know the expected return type in
-    advance.
+    advance. A non-``NULL`` result is reference-counted and should be released with
+    :c:func:`nrn_object_unref` when no longer needed.
 
 .. c:function:: nrn_stack_types_t nrn_stack_type(void)
 

src/nrniv/neuronapi.cpp

@@ -463,6 +463,14 @@ void nrn_symbol_push(Symbol* sym) {
     hoc_pushpx(sym->u.pval);
 }
 
+Symbol* nrn_symbol_pop(void) {
+    // Pop a Symbol (a STACK_IS_SYM entry) off the interpreter stack. Interpreter
+    // frames for object-component access (e.g. reading or assigning pyobj.attr)
+    // carry the attribute's Symbol on the stack; a binding that unwinds such a
+    // frame needs to pop it. Public counterpart to the internal hoc_spop.
+    return hoc_spop();
+}
+
 void nrn_double_push(double val) {
     hoc_pushx(val);
 }
@@ -511,12 +519,19 @@ void nrn_object_ptr_push(Object** obj_ref) {
 }
 
 Object* nrn_object_pop(void) {
-    // NOTE: the returned object should be unref'd when no longer needed
+    // Returns NULL for a nil object reference (an unset objref) rather than
+    // crashing: the ref-count bump that hands back a reference would otherwise
+    // dereference NULL. This matters when unwinding a stack that may carry a nil
+    // object -- e.g. the HOC-to-Python write-back path, where an objref RHS can
+    // be nil. A non-NULL result is reference-counted and should be unref'd
+    // (nrn_object_unref) when no longer needed.
     Object** obptr = hoc_objpop();
-    Object* new_ob_ptr = *obptr;
-    new_ob_ptr->refcount++;
+    Object* ob = *obptr;
+    if (ob) {
+        ob->refcount++;
+    }
     hoc_tobj_unref(obptr);
-    return new_ob_ptr;
+    return ob;
 }
 
 nrn_stack_types_t nrn_stack_type(void) {

src/nrniv/neuronapi.h

@@ -88,6 +88,7 @@ int nrn_pp_setpointer_pop(Object* pp, const char* name, char* error_msg, size_t
  ****************************************/
 Symbol* nrn_symbol(const char* name);
 void nrn_symbol_push(Symbol* sym);
+Symbol* nrn_symbol_pop(void);
 int nrn_symbol_type(const Symbol* sym);
 int nrn_symbol_subtype(const Symbol* sym);
 double* nrn_symbol_dataptr(const Symbol* sym);

test/api/CMakeLists.txt

@@ -10,6 +10,7 @@ foreach(
   section_tree.cpp
   segment_diam.cpp
   sections.cpp
+  stack_pops.cpp
   symbol_object_string.cpp
   vclamp.cpp)
   string(REPLACE "." "_" api_test_name "${api_test_file}")

test/api/stack_pops.cpp

@@ -0,0 +1,75 @@
+// NOTE: this assumes neuronapi.h is on your CPLUS_INCLUDE_PATH
+// Exercises nrn_symbol_pop and nrn_object_pop's nil handling, the stack-pop
+// primitives used to unwind an interpreter frame from a binding (the
+// HOC-to-Python component read/write-back path). nrn_symbol_pop pops a Symbol
+// the interpreter left on the stack; nrn_object_pop pops an object but returns
+// NULL for a nil objref instead of dereferencing it to take a reference.
+#include <array>
+#include <iostream>
+#include "neuronapi.h"
+
+using std::cerr;
+using std::endl;
+
+// No public API leaves a bare Symbol (STACK_IS_SYM) on the stack -- the
+// interpreter pushes one during object-component access -- so the test uses the
+// internal push to put a Symbol there, the way nrniv would mid-expression.
+extern void hoc_pushs(Symbol*);
+
+extern "C" void modl_reg() {}
+
+static bool check(bool cond, const char* msg) {
+    if (!cond) {
+        cerr << "FAIL: " << msg << endl;
+    }
+    return cond;
+}
+
+int main(void) {
+    static std::array<const char*, 4> argv = {"stack_pops", "-nogui", "-nopython", nullptr};
+    nrn_init(3, argv.data());
+
+    bool ok = true;
+
+    // Push a sentinel FIRST, below everything else the test does. The stack is
+    // LIFO, so if every push below is matched by exactly one pop the sentinel
+    // resurfaces on top at the end; recovering it then proves the pops neither
+    // over-popped into what lay beneath them nor left anything behind. A
+    // sentinel pushed *after* the pops could prove neither -- it would only ever
+    // probe the current top.
+    const double SENTINEL = 424242.0;
+    nrn_double_push(SENTINEL);
+
+    // nrn_symbol_pop returns the Symbol on top of the stack, LIFO.
+    Symbol* v = nrn_symbol("v");
+    Symbol* t = nrn_symbol("t");
+    ok &= check(v != nullptr && t != nullptr, "symbols v and t resolve");
+    hoc_pushs(v);
+    hoc_pushs(t);
+    ok &= check(nrn_stack_type() == STACK_IS_SYM, "stack top is a symbol before nrn_symbol_pop");
+    ok &= check(nrn_symbol_pop() == t, "symbol pop returns the last pushed symbol");
+    ok &= check(nrn_symbol_pop() == v, "symbol pop returns the earlier symbol (LIFO)");
+
+    // nrn_object_pop returns a real object (reference-counted).
+    Object* vec = nrn_object_new(nrn_symbol("Vector"), 0);
+    ok &= check(vec != nullptr, "Vector constructed");
+    nrn_object_push(vec);
+    Object* got = nrn_object_pop();
+    ok &= check(got == vec, "object pop returns the pushed object");
+    if (got) {
+        nrn_object_unref(got);
+    }
+
+    // nrn_object_pop returns NULL for a nil object reference instead of
+    // crashing -- a naive pop would dereference the NULL to take a reference and
+    // segfault here.
+    nrn_object_push(nullptr);
+    ok &= check(nrn_object_pop() == nullptr, "object pop returns NULL for a nil objref");
+
+    // Balance: with every push above consumed by exactly one pop, the sentinel
+    // from the very start is what remains on top.
+    ok &= check(nrn_double_pop() == SENTINEL,
+                "sentinel from before the pops is intact -- the stack is balanced");
+
+    return ok ? 0 : 1;
+}