From 6cb81fad3feb50a02249736685bd6c7e088afa84 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Mon, 6 Jul 2026 11:06:58 -0400 Subject: [PATCH 1/2] Revert to the miss-only __getattr__ hook for AttributeError suggestions Revert the code changes of #126, restoring the AttributeErrorHint miss-only __getattr__ hook approach from #124: successful attribute accesses go straight through CPython's native generic getattr with no managed transition; only a miss enters managed code to build the "Did you mean ...?" suggestions. The package is left at 2.0.56 (not reverted). Note: as restored here, the hook still has the off-GIL callback defect that crashed Lean's CI on 2.0.55 (the __clr_attr_msg__ delegate is invoked through MethodBinder, which releases the GIL); it is fixed in the follow-up commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/AttributeErrorHint.cs | 109 ++++++++++++++++++++++++++++++ src/runtime/PythonEngine.cs | 7 ++ src/runtime/TypeManager.cs | 4 ++ src/runtime/Types/ClassBase.cs | 73 +++++++++++++++++--- src/runtime/Types/ClassObject.cs | 15 ---- 5 files changed, 182 insertions(+), 26 deletions(-) create mode 100644 src/runtime/AttributeErrorHint.cs diff --git a/src/runtime/AttributeErrorHint.cs b/src/runtime/AttributeErrorHint.cs new file mode 100644 index 000000000..f7feec985 --- /dev/null +++ b/src/runtime/AttributeErrorHint.cs @@ -0,0 +1,109 @@ +using System; + +using Python.Runtime.Native; + +namespace Python.Runtime +{ + /// + /// Installs a miss-only __getattr__ hook on reflected .NET types so that an + /// AttributeError raised for a missing attribute is enriched with suggestions + /// of similarly-named members — without adding any cost to the (common) successful + /// attribute-access path. + /// + /// + /// CPython only invokes __getattr__ after the normal attribute lookup fails, + /// via the native slot_tp_getattr_hook: on a hit it calls the generic getattr + /// directly (no managed transition); only on a miss does it call our __getattr__. + /// pythonnet's metatype does not run CPython's slot-fixup machinery when an attribute + /// is set on a type, so simply adding __getattr__ to the type dict would not + /// rewire the slot — we therefore wire tp_getattro to the hook manually. + /// + internal static class AttributeErrorHint + { + // The shared __getattr__ function object installed on every eligible type. + private static PyObject? _getAttr; + // The managed message builder exposed to Python, kept alive for _getAttr's globals. + private static PyObject? _messageBuilder; + // Address of CPython's slot_tp_getattr_hook (extracted from a probe type). + private static IntPtr _hookSlot; + // Address of PyObject_GenericGetAttr, used to detect types we may safely redirect. + private static IntPtr _genericGetAttr; + + private static bool IsReady => _getAttr is not null && _hookSlot != IntPtr.Zero; + + internal static void Initialize() + { + try + { + _genericGetAttr = Util.ReadIntPtr(Runtime.PyBaseObjectType, TypeOffset.tp_getattro); + + Func builder = ClassBase.BuildMissingAttributeMessage; + _messageBuilder = builder.ToPython(); + + using var globals = new PyDict(); + Runtime.PyDict_SetItemString(globals.Reference, "__builtins__", Runtime.PyEval_GetBuiltins()); + globals["__clr_attr_msg__"] = _messageBuilder; + + // Define the shared hook, plus a probe class whose tp_getattro is + // slot_tp_getattr_hook so we can read that function pointer. + PythonEngine.Exec( + "def __clr_getattr__(self, name):\n" + + " raise AttributeError(__clr_attr_msg__(self, name))\n" + + "class __clr_getattr_probe__:\n" + + " def __getattr__(self, name):\n" + + " raise AttributeError(name)\n", + globals); + + _getAttr = globals["__clr_getattr__"]; + using var probe = globals["__clr_getattr_probe__"]; + _hookSlot = Util.ReadIntPtr(probe.Reference, TypeOffset.tp_getattro); + } + catch (Exception e) + { + // Degrade gracefully: without the hook, AttributeError messages are simply + // not enriched. Never let this break interpreter initialization. + DebugUtil.Print($"AttributeErrorHint.Initialize failed: {e}"); + Shutdown(); + } + } + + /// + /// Wires the miss-only hook onto if it still uses the + /// native generic getattr. Types with a custom tp_getattro (dynamic + /// objects, modules, interfaces, ...) handle misses themselves and are left + /// untouched; derived types that inherit an already-hooked base are likewise + /// skipped, since they inherit the behavior through the MRO. + /// + internal static void Install(BorrowedReference type) + { + if (!IsReady) + { + return; + } + + if (Util.ReadIntPtr(type, TypeOffset.tp_getattro) != _genericGetAttr) + { + return; + } + + if (Runtime.PyObject_SetAttrString(type, "__getattr__", _getAttr!.Reference) != 0) + { + Exceptions.Clear(); + return; + } + + Util.WriteIntPtr(type, TypeOffset.tp_getattro, _hookSlot); + Runtime.PyType_Modified(type); + } + + internal static void Shutdown() + { + _getAttr?.Dispose(); + _getAttr = null; + _messageBuilder?.Dispose(); + _messageBuilder = null; + _hookSlot = IntPtr.Zero; + _genericGetAttr = IntPtr.Zero; + } + } +} diff --git a/src/runtime/PythonEngine.cs b/src/runtime/PythonEngine.cs index eb0c98ce9..677a44978 100644 --- a/src/runtime/PythonEngine.cs +++ b/src/runtime/PythonEngine.cs @@ -263,6 +263,10 @@ public static void Initialize(IEnumerable args, bool setSysArgv = true, } ImportHook.UpdateCLRModuleDict(); + + // Set up the miss-only __getattr__ hook used to enrich AttributeError + // messages on reflected .NET types with member-name suggestions. + AttributeErrorHint.Initialize(); } static BorrowedReference DefineModule(string name) @@ -369,6 +373,9 @@ public static void Shutdown() AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; ExecuteShutdownHandlers(); + + AttributeErrorHint.Shutdown(); + // Remember to shut down the runtime. Runtime.Shutdown(); diff --git a/src/runtime/TypeManager.cs b/src/runtime/TypeManager.cs index 3b75738b2..cbaa730ca 100644 --- a/src/runtime/TypeManager.cs +++ b/src/runtime/TypeManager.cs @@ -303,6 +303,10 @@ internal static void InitializeClass(PyType type, ClassBase impl, Type clrType) Runtime.PyType_Modified(type.Reference); + // Enrich AttributeError messages for missing attributes with member-name + // suggestions, via a miss-only __getattr__ hook (no hot-path cost). + AttributeErrorHint.Install(type.Reference); + //DebugUtil.DumpType(type); } diff --git a/src/runtime/Types/ClassBase.cs b/src/runtime/Types/ClassBase.cs index 617baae49..7e831d17f 100644 --- a/src/runtime/Types/ClassBase.cs +++ b/src/runtime/Types/ClassBase.cs @@ -628,20 +628,13 @@ internal static void AppendAttributeErrorSuggestions(BorrowedReference ob, Borro } var name = Runtime.GetManagedString(key); - // Skip empty and dunder names: the latter are probed internally by CPython - // (e.g. __iter__, __len__) and are never user-facing typos worth helping with. - if (string.IsNullOrEmpty(name) || name.StartsWith("__", StringComparison.Ordinal)) + if (string.IsNullOrEmpty(name)) { return; } - if (GetManagedObject(ob) is not CLRObject clrObj || clrObj.inst is null) - { - return; - } - - var suggestions = GetSimilarMemberNames(clrObj.inst.GetType(), name); - if (suggestions.Count == 0) + var hint = GetSuggestionHint(ob, name); + if (hint.Length == 0) { return; } @@ -651,7 +644,6 @@ internal static void AppendAttributeErrorSuggestions(BorrowedReference ob, Borro try { var baseMessage = GetErrorMessage(errValue.BorrowNullable(), name); - var hint = " Did you mean: " + string.Join(", ", suggestions.Select(s => $"'{s}'")) + "?"; Exceptions.SetError(Exceptions.AttributeError, baseMessage + hint); } finally @@ -662,6 +654,65 @@ internal static void AppendAttributeErrorSuggestions(BorrowedReference ob, Borro } } + /// + /// Builds the full message for an AttributeError raised for a missing + /// attribute on a .NET object, including any "Did you mean ...?" hint. Used by + /// the miss-only __getattr__ hook installed on reflected types (see + /// ), where the original error has already been + /// cleared, so the base message is reconstructed here. + /// + internal static string BuildMissingAttributeMessage(PyObject self, string name) + { + var typeName = "object"; + try + { + using var pyType = self.GetPythonType(); + typeName = pyType.Name; + } + catch + { + // fall back to the generic type name + } + + var message = $"'{typeName}' object has no attribute '{name}'"; + try + { + return message + GetSuggestionHint(self.Reference, name); + } + catch + { + // never let suggestion building turn into a different exception + return message; + } + } + + /// + /// Returns " Did you mean: 'x', 'y'?" listing similarly-named members of the + /// managed object, or an empty string when there is nothing to suggest. Dunder + /// names are skipped: they are probed internally by CPython (e.g. __iter__, + /// __len__) and are never user-facing typos worth helping with. + /// + private static string GetSuggestionHint(BorrowedReference ob, string name) + { + if (string.IsNullOrEmpty(name) || name.StartsWith("__", StringComparison.Ordinal)) + { + return string.Empty; + } + + if (GetManagedObject(ob) is not CLRObject clrObj || clrObj.inst is null) + { + return string.Empty; + } + + var suggestions = GetSimilarMemberNames(clrObj.inst.GetType(), name); + if (suggestions.Count == 0) + { + return string.Empty; + } + + return " Did you mean: " + string.Join(", ", suggestions.Select(s => $"'{s}'")) + "?"; + } + private static string GetErrorMessage(BorrowedReference value, string fallbackName) { if (value != null) diff --git a/src/runtime/Types/ClassObject.cs b/src/runtime/Types/ClassObject.cs index 48a975898..b57378a32 100644 --- a/src/runtime/Types/ClassObject.cs +++ b/src/runtime/Types/ClassObject.cs @@ -167,21 +167,6 @@ public override void InitializeSlots(BorrowedReference pyType, SlotsHolder slots protected virtual NewReference NewObjectToPython(object obj, BorrowedReference tp) => CLRObject.GetReference(obj, tp); - /// - /// Type __getattro__ implementation. Delegates to the generic CLR attribute - /// lookup, but enriches the AttributeError raised for a missing attribute with - /// suggestions of similarly-named members of the managed type. - /// - public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference key) - { - var result = Runtime.PyObject_GenericGetAttr(ob, key); - if (result.IsNull()) - { - AppendAttributeErrorSuggestions(ob, key); - } - return result; - } - private static NewReference NewEnum(Type type, BorrowedReference args, BorrowedReference tp) { nint argCount = Runtime.PyTuple_Size(args); From d29855ec667d5213020bd5c64da7fdaf2de0cf5a Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Mon, 6 Jul 2026 11:17:24 -0400 Subject: [PATCH 2/2] Fix off-GIL AttributeError-hint callback: use a native __getattr__ thunk The __getattr__ hook restored from #124 exposed the hint builder to Python as a .NET delegate (Func). Python invoked it through DelegateObject.tp_call -> MethodBinder.Invoke, and MethodBinder releases the GIL around every reflected invocation (allow_threads defaults to true). The callback therefore ran CPython C-API calls (GetPythonType, GetManagedString, ...) without holding the GIL on every attribute miss (hasattr, getattr with default, typos). It usually survived by luck, but segfaulted whenever the pythonnet Finalizer fired mid-callback: Finalizer.DisposeAll() starts with PyErr_Fetch, which dereferences the current thread state - NULL when the GIL is not held. This is what crashed Lean's CI test host on 2.0.55 after all 35k tests passed. Replace the Python-function-plus-delegate pair with a native method descriptor: a PyMethodDef (METH_VARARGS) around a managed thunk, turned into __getattr__ via PyDescr_NewMethod (newly bound). CPython's slot_tp_getattr_hook now calls the managed hook directly as a native method call with the GIL held - MethodBinder is never involved. The PyMethodDef and thunk are allocated once and kept for the process lifetime, since descriptors reference them and can outlive engine shutdown bookkeeping. Verified with an instrumented build (PyGILState_Check inside BuildMissingAttributeMessage): the delegate-based hook reports "GIL held: False" on every miss; this version reports "GIL held: True". Also survives a 20s stress run of concurrent attribute misses plus finalizer churn across 6 threads, and behaves identically for hasattr/ getattr-with-default, dunder probes, Python subclasses with their own __getattr__, and suggestion messages. Add a regression test asserting the installed __getattr__ is a native method_descriptor, which is the property that keeps the callback out of MethodBinder's allow-threads path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/AttributeErrorHint.cs | 92 ++++++++++++++++++++++++------- src/runtime/Runtime.Delegates.cs | 2 + src/runtime/Runtime.cs | 7 +++ tests/test_class.py | 17 ++++++ 4 files changed, 99 insertions(+), 19 deletions(-) diff --git a/src/runtime/AttributeErrorHint.cs b/src/runtime/AttributeErrorHint.cs index f7feec985..7faa10996 100644 --- a/src/runtime/AttributeErrorHint.cs +++ b/src/runtime/AttributeErrorHint.cs @@ -1,4 +1,6 @@ using System; +using System.Reflection; +using System.Runtime.InteropServices; using Python.Runtime.Native; @@ -17,19 +19,29 @@ namespace Python.Runtime /// pythonnet's metatype does not run CPython's slot-fixup machinery when an attribute /// is set on a type, so simply adding __getattr__ to the type dict would not /// rewire the slot — we therefore wire tp_getattro to the hook manually. + /// + /// The __getattr__ itself is a native method descriptor (PyDescr_NewMethod) + /// around a managed thunk, NOT a .NET delegate exposed to Python: delegate calls go + /// through , which releases the GIL around the invocation + /// (allow_threads), so the callback would run CPython C-API calls off-GIL and crash + /// whenever the fires mid-callback. The native thunk is + /// called directly by the interpreter with the GIL held. /// internal static class AttributeErrorHint { - // The shared __getattr__ function object installed on every eligible type. - private static PyObject? _getAttr; - // The managed message builder exposed to Python, kept alive for _getAttr's globals. - private static PyObject? _messageBuilder; + // Unmanaged PyMethodDef backing the shared __getattr__ method descriptors. + // Descriptors keep a raw pointer to it (d_method) and can outlive engine + // shutdown bookkeeping, so it is allocated once and kept for the process + // lifetime (as are the thunks in Interop.allocatedThunks). + private static IntPtr _methodDef; + // Keeps the thunk delegate for GetAttrHook alive. + private static ThunkInfo? _thunk; // Address of CPython's slot_tp_getattr_hook (extracted from a probe type). private static IntPtr _hookSlot; // Address of PyObject_GenericGetAttr, used to detect types we may safely redirect. private static IntPtr _genericGetAttr; - private static bool IsReady => _getAttr is not null && _hookSlot != IntPtr.Zero; + private static bool IsReady => _methodDef != IntPtr.Zero && _hookSlot != IntPtr.Zero; internal static void Initialize() { @@ -37,24 +49,25 @@ internal static void Initialize() { _genericGetAttr = Util.ReadIntPtr(Runtime.PyBaseObjectType, TypeOffset.tp_getattro); - Func builder = ClassBase.BuildMissingAttributeMessage; - _messageBuilder = builder.ToPython(); + if (_methodDef == IntPtr.Zero) + { + _thunk = Interop.GetThunk(typeof(AttributeErrorHint).GetMethod( + nameof(GetAttrHook), BindingFlags.Static | BindingFlags.Public)!); + IntPtr methodDef = Marshal.AllocHGlobal(4 * IntPtr.Size); + TypeManager.WriteMethodDef(methodDef, "__getattr__", _thunk.Address); + _methodDef = methodDef; + } + // Define a probe class whose tp_getattro is slot_tp_getattr_hook so we + // can read that function pointer. using var globals = new PyDict(); Runtime.PyDict_SetItemString(globals.Reference, "__builtins__", Runtime.PyEval_GetBuiltins()); - globals["__clr_attr_msg__"] = _messageBuilder; - - // Define the shared hook, plus a probe class whose tp_getattro is - // slot_tp_getattr_hook so we can read that function pointer. PythonEngine.Exec( - "def __clr_getattr__(self, name):\n" + - " raise AttributeError(__clr_attr_msg__(self, name))\n" + "class __clr_getattr_probe__:\n" + " def __getattr__(self, name):\n" + " raise AttributeError(name)\n", globals); - _getAttr = globals["__clr_getattr__"]; using var probe = globals["__clr_getattr_probe__"]; _hookSlot = Util.ReadIntPtr(probe.Reference, TypeOffset.tp_getattro); } @@ -86,7 +99,15 @@ internal static void Install(BorrowedReference type) return; } - if (Runtime.PyObject_SetAttrString(type, "__getattr__", _getAttr!.Reference) != 0) + using var descr = Runtime.PyDescr_NewMethod(type, _methodDef); + if (descr.IsNull()) + { + Exceptions.Clear(); + return; + } + + BorrowedReference dict = Util.ReadRef(type, TypeOffset.tp_dict); + if (Runtime.PyDict_SetItemString(dict, "__getattr__", descr.Borrow()) != 0) { Exceptions.Clear(); return; @@ -96,12 +117,45 @@ internal static void Install(BorrowedReference type) Runtime.PyType_Modified(type); } + /// + /// The __getattr__(self, name) implementation (METH_VARARGS). CPython's + /// slot_tp_getattr_hook only calls it after the normal lookup has failed + /// and the original AttributeError has been cleared, so the full message is + /// rebuilt here. Runs as a direct native method call with the GIL held. + /// + public static NewReference GetAttrHook(BorrowedReference ob, BorrowedReference args) + { + string? name = null; + string message; + try + { + if (Runtime.PyTuple_Size(args) == 1) + { + BorrowedReference key = Runtime.PyTuple_GetItem(args, 0); + if (Runtime.PyString_Check(key)) + { + name = Runtime.GetManagedString(key); + } + } + + using var self = new PyObject(ob); + message = ClassBase.BuildMissingAttributeMessage(self, name ?? "?"); + } + catch + { + // Never let message building turn into a different exception. + message = $"object has no attribute '{name ?? "?"}'"; + } + + Exceptions.SetError(Exceptions.AttributeError, message); + return default; + } + internal static void Shutdown() { - _getAttr?.Dispose(); - _getAttr = null; - _messageBuilder?.Dispose(); - _messageBuilder = null; + // _methodDef and _thunk are deliberately kept: method descriptors created + // from them may still be reachable during interpreter teardown, and both + // are reused by the next Initialize. _hookSlot = IntPtr.Zero; _genericGetAttr = IntPtr.Zero; } diff --git a/src/runtime/Runtime.Delegates.cs b/src/runtime/Runtime.Delegates.cs index 5a6e0507d..bcb1192c4 100644 --- a/src/runtime/Runtime.Delegates.cs +++ b/src/runtime/Runtime.Delegates.cs @@ -230,6 +230,7 @@ static Delegates() PyObject_GenericGetAttr = (delegate* unmanaged[Cdecl])GetFunctionByName(nameof(PyObject_GenericGetAttr), GetUnmanagedDll(_PythonDll)); PyObject_GenericGetDict = (delegate* unmanaged[Cdecl])GetFunctionByName(nameof(PyObject_GenericGetDict), GetUnmanagedDll(PythonDLL)); PyObject_GenericSetAttr = (delegate* unmanaged[Cdecl])GetFunctionByName(nameof(PyObject_GenericSetAttr), GetUnmanagedDll(_PythonDll)); + PyDescr_NewMethod = (delegate* unmanaged[Cdecl])GetFunctionByName(nameof(PyDescr_NewMethod), GetUnmanagedDll(_PythonDll)); PyObject_GC_Del = (delegate* unmanaged[Cdecl])GetFunctionByName(nameof(PyObject_GC_Del), GetUnmanagedDll(_PythonDll)); try { @@ -504,6 +505,7 @@ static Delegates() internal static delegate* unmanaged[Cdecl] _PyType_Lookup { get; } internal static delegate* unmanaged[Cdecl] PyObject_GenericGetAttr { get; } internal static delegate* unmanaged[Cdecl] PyObject_GenericSetAttr { get; } + internal static delegate* unmanaged[Cdecl] PyDescr_NewMethod { get; } internal static delegate* unmanaged[Cdecl] PyObject_GC_Del { get; } internal static delegate* unmanaged[Cdecl] PyObject_GC_IsTracked { get; } internal static delegate* unmanaged[Cdecl] PyObject_GC_Track { get; } diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs index ff081e893..b2ae25dce 100644 --- a/src/runtime/Runtime.cs +++ b/src/runtime/Runtime.cs @@ -1738,6 +1738,13 @@ internal static bool PyType_IsSameAsOrSubtype(BorrowedReference type, BorrowedRe internal static int PyObject_GenericSetAttr(BorrowedReference obj, BorrowedReference name, BorrowedReference value) => Delegates.PyObject_GenericSetAttr(obj, name, value); + + /// + /// Creates a method descriptor for from an unmanaged + /// PyMethodDef*, which must remain valid for the descriptor's lifetime. + /// + internal static NewReference PyDescr_NewMethod(BorrowedReference type, IntPtr methodDef) => Delegates.PyDescr_NewMethod(type, methodDef); + internal static NewReference PyObject_GenericGetDict(BorrowedReference o) => PyObject_GenericGetDict(o, IntPtr.Zero); internal static NewReference PyObject_GenericGetDict(BorrowedReference o, IntPtr context) => Delegates.PyObject_GenericGetDict(o, context); diff --git a/tests/test_class.py b/tests/test_class.py index 4f0effecd..bfa40714c 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -104,6 +104,23 @@ def test_missing_attribute_hasattr_still_false(): assert hasattr(s, "Length") +def test_missing_attribute_hook_is_native(): + """The __getattr__ hook must be a native method descriptor. + + If it were a Python function calling into .NET through a delegate, the call + would go through MethodBinder, which releases the GIL around the invocation: + the hint-building callback would then run CPython C-API calls off-GIL and + crash whenever the pythonnet Finalizer fires mid-callback (the Lean CI crash + on 2.0.55). + """ + hook = next( + c.__dict__["__getattr__"] + for c in type(System.String("x")).__mro__ + if "__getattr__" in c.__dict__ + ) + assert type(hook).__name__ == "method_descriptor" + + def test_basic_subclass(): """Test basic subclass of a managed class.""" from System.Collections import Hashtable