diff --git a/src/runtime/AttributeErrorHint.cs b/src/runtime/AttributeErrorHint.cs new file mode 100644 index 000000000..7faa10996 --- /dev/null +++ b/src/runtime/AttributeErrorHint.cs @@ -0,0 +1,163 @@ +using System; +using System.Reflection; +using System.Runtime.InteropServices; + +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. + /// + /// 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 + { + // 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 => _methodDef != IntPtr.Zero && _hookSlot != IntPtr.Zero; + + internal static void Initialize() + { + try + { + _genericGetAttr = Util.ReadIntPtr(Runtime.PyBaseObjectType, TypeOffset.tp_getattro); + + 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()); + PythonEngine.Exec( + "class __clr_getattr_probe__:\n" + + " def __getattr__(self, name):\n" + + " raise AttributeError(name)\n", + globals); + + 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; + } + + 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; + } + + Util.WriteIntPtr(type, TypeOffset.tp_getattro, _hookSlot); + 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() + { + // _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/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/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/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); 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