Apply clang-tidy fixes to subinterpreter support code (#5929)

* Fix PyObject_HasAttrString return value

Signed-off-by: cyy <[email protected]>

* Tidy unchecked files

Signed-off-by: cyy <[email protected]>

* [skip ci] Handle PyObject_HasAttrString error when probing __notes__

PyObject_HasAttrString may return -1 to signal an error and set a
Python exception. The previous logic only checked for "!= 0", which
meant that the error path was treated the same as "attribute exists",
causing two problems: misreporting the presence of __notes__ and
leaving a spurious exception pending.

The earlier PR tightened the condition to "== 1" so that only a
successful lookup marks the error string as [WITH __notes__], but it
still left the -1 case unhandled. In the context of
error_fetch_and_normalize, we are already dealing with an active
exception and only want to best-effort detect whether normalization
attached any __notes__. If the attribute probe itself fails, we do not
want that secondary failure to affect later C-API calls or the error
we ultimately report.

This change stores the PyObject_HasAttrString return value, treats
"== 1" as "has __notes__", and explicitly calls PyErr_Clear() when
it returns -1. That way, we avoid leaking a secondary error while
still preserving the original exception information and hinting
[WITH __notes__] only when we can determine it reliably.

* Run clang-tidy with -DPYBIND11_HAS_SUBINTERPRETER_SUPPORT

* [skip ci] Revert "Run clang-tidy with -DPYBIND11_HAS_SUBINTERPRETER_SUPPORT"

This reverts commit bb6e751.

---------

Signed-off-by: cyy <[email protected]>
Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>

Author: cyyever

include/pybind11/pytypes.h

@@ -547,8 +547,13 @@ struct error_fetch_and_normalize {
         // The presence of __notes__ is likely due to exception normalization
         // errors, although that is not necessarily true, therefore insert a
         // hint only:
-        if (PyObject_HasAttrString(m_value.ptr(), "__notes__") != 0) {
+        const int has_notes = PyObject_HasAttrString(m_value.ptr(), "__notes__");
+        if (has_notes == 1) {
             m_lazy_error_string += "[WITH __notes__]";
+        } else if (has_notes == -1) {
+            // Ignore secondary errors when probing for __notes__ to avoid leaking a
+            // spurious exception while still reporting the original error.
+            PyErr_Clear();
         }
 #else
         // PyErr_NormalizeException() may change the exception type if there are cascading

include/pybind11/subinterpreter.h

@@ -22,11 +22,11 @@
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 PYBIND11_NAMESPACE_BEGIN(detail)
 inline PyInterpreterState *get_interpreter_state_unchecked() {
-    auto cur_tstate = get_thread_state_unchecked();
-    if (cur_tstate)
+    auto *cur_tstate = get_thread_state_unchecked();
+    if (cur_tstate) {
         return cur_tstate->interp;
-    else
-        return nullptr;
+    }
+    return nullptr;
 }
 PYBIND11_NAMESPACE_END(detail)
 
@@ -76,15 +76,15 @@ class subinterpreter {
     /// Create a new subinterpreter with the specified configuration
     /// @note This function acquires (and then releases) the main interpreter GIL, but the main
     /// interpreter and its GIL are not required to be held prior to calling this function.
-    static inline subinterpreter create(PyInterpreterConfig const &cfg) {
+    static subinterpreter create(PyInterpreterConfig const &cfg) {
 
         error_scope err_scope;
         subinterpreter result;
         {
             // we must hold the main GIL in order to create a subinterpreter
             subinterpreter_scoped_activate main_guard(main());
 
-            auto prev_tstate = PyThreadState_Get();
+            auto *prev_tstate = PyThreadState_Get();
 
             PyStatus status;
 
@@ -103,7 +103,7 @@ class subinterpreter {
             }
 
             // this doesn't raise a normal Python exception, it provides an exit() status code.
-            if (PyStatus_Exception(status)) {
+            if (PyStatus_Exception(status) != 0) {
                 pybind11_fail("failed to create new sub-interpreter");
             }
 
@@ -128,7 +128,7 @@ class subinterpreter {
 
     /// Calls create() with a default configuration of an isolated interpreter that disallows fork,
     /// exec, and Python threads.
-    static inline subinterpreter create() {
+    static subinterpreter create() {
         // same as the default config in the python docs
         PyInterpreterConfig cfg;
         std::memset(&cfg, 0, sizeof(cfg));
@@ -144,8 +144,8 @@ class subinterpreter {
             return;
         }
 
-        PyThreadState *destroy_tstate;
-        PyThreadState *old_tstate;
+        PyThreadState *destroy_tstate = nullptr;
+        PyThreadState *old_tstate = nullptr;
 
         // Python 3.12 requires us to keep the original PyThreadState alive until we are ready to
         // destroy the interpreter.  We prefer to use that to destroy the interpreter.
@@ -173,7 +173,7 @@ class subinterpreter {
         old_tstate = PyThreadState_Swap(destroy_tstate);
 #endif
 
-        bool switch_back = old_tstate && old_tstate->interp != istate_;
+        bool switch_back = (old_tstate != nullptr) && old_tstate->interp != istate_;
 
         // Internals always exists in the subinterpreter, this class enforces it when it creates
         // the subinterpreter. Even if it didn't, this only creates the pointer-to-pointer, not the
@@ -190,8 +190,9 @@ class subinterpreter {
         detail::get_local_internals_pp_manager().destroy();
 
         // switch back to the old tstate and old GIL (if there was one)
-        if (switch_back)
+        if (switch_back) {
             PyThreadState_Swap(old_tstate);
+        }
     }
 
     /// Get a handle to the main interpreter that can be used with subinterpreter_scoped_activate
@@ -214,11 +215,11 @@ class subinterpreter {
 
     /// Get the numerical identifier for the sub-interpreter
     int64_t id() const {
-        if (istate_ != nullptr)
+        if (istate_ != nullptr) {
             return PyInterpreterState_GetID(istate_);
-        else
-            return -1; // CPython uses one-up numbers from 0, so negative should be safe to return
-                       // here.
+        }
+        return -1; // CPython uses one-up numbers from 0, so negative should be safe to return
+                   // here.
     }
 
     /// Get the interpreter's state dict.  This interpreter's GIL must be held before calling!