Catch errors in setting/deleting properties

filmor · filmor · commit 2408c4317723 · 2026-05-15T21:57:36.000+02:00
- Catch exceptions in TrySet/DeleteMember
- Convert the exceptions into Python exceptions
- Add tests for the remaining cases
- Add a note on why the field has to be lazily initialized (general
  issue with derived classes)
diff --git a/src/runtime/TypeManager.cs b/src/runtime/TypeManager.cs
@@ -169,19 +169,32 @@ public static int tp_setattro_dlr_proxy(BorrowedReference ob, BorrowedReference
             if (!HasClrMember(instance, memberName) && !IsPythonSpecialAttributeName(memberName))
             {
                 // Try DLR member storage first
-                bool handled = false;
+                bool handled;
 
-                if (val.IsNull)
+                try
                 {
-                    handled = dynamicMemberAccessor.TryDeleteMember(dynamicObject, memberName);
+                    if (val.IsNull)
+                    {
+                        handled = dynamicMemberAccessor.TryDeleteMember(dynamicObject, memberName);
+                    }
+                    else
+                    {
+                        object? managedValue = null;
+                        if (val != Runtime.PyNone && !Converter.ToManaged(val, typeof(object), out managedValue, true))
+                            return -1;
+
+                        handled = dynamicMemberAccessor.TrySetMember(dynamicObject, memberName, managedValue);
+                        if (!handled)
+                        {
+                            Exceptions.SetError(Exceptions.AttributeError, $"'{instance.GetType().Name}' object has no attribute '{memberName}'");
+                            return -1;
+                        }
+                    }
                 }
-                else
+                catch (Exception e)
                 {
-                    object? managedValue = null;
-                    if (val != Runtime.PyNone && !Converter.ToManaged(val, typeof(object), out managedValue, true))
-                        return -1;
-
-                    handled = dynamicMemberAccessor.TrySetMember(dynamicObject, memberName, managedValue);
+                    Exceptions.SetError(e);
+                    return -1;
                 }
 
                 if (handled)
diff --git a/src/testing/dlrtest.cs b/src/testing/dlrtest.cs
@@ -4,12 +4,40 @@
 
 namespace Python.Test;
 
-public class DynamicMappingObject : DynamicObject
+/// <summary>
+/// Base class for dynamic test helpers. Uses lazy storage initialization so that
+/// Python-derived subclasses can safely call DynamicObject member hooks before
+/// managed field initializers have run.
+/// </summary>
+public class DynamicStorageObject : DynamicObject
 {
     Dictionary<string, object> storage;
 
-    Dictionary<string, object> Storage => storage ??= [];
+    // Python-defined subclasses may reach this type without running managed field
+    // initializers (see ClassDerivedObject.NewObjectToPython). Via the lazy init
+    // we can ensure that the access is still safe, even when the constructor has
+    // not run.
+    protected Dictionary<string, object> Storage => storage ??= [];
 
+    public void AddDynamicMember(string name, object value) => Storage[name] = value;
+
+    public override bool TryGetMember(GetMemberBinder binder, out object result)
+        => Storage.TryGetValue(binder.Name, out result);
+
+    public override bool TrySetMember(SetMemberBinder binder, object value)
+    {
+        Storage[binder.Name] = value;
+        return true;
+    }
+
+    public override bool TryDeleteMember(DeleteMemberBinder binder)
+        => Storage.Remove(binder.Name);
+
+    public override IEnumerable<string> GetDynamicMemberNames() => Storage.Keys;
+}
+
+public class DynamicMappingObject : DynamicStorageObject
+{
     // Native members for testing that regular CLR access is unaffected.
     public string Label = "default";
     public int Multiplier { get; set; } = 1;
@@ -20,20 +48,39 @@ public class DynamicMappingObject : DynamicObject
 
     // Test helper: retrieve the actual value stored in C# (for verification that None was stored as null)
     public object GetDynamicValue(string name) => Storage.TryGetValue(name, out var value) ? value : null;
+}
 
-
-
-    public override bool TryGetMember(GetMemberBinder binder, out object result)
-        => Storage.TryGetValue(binder.Name, out result);
-
+public class RejectingSetDynamicObject : DynamicStorageObject
+{
     public override bool TrySetMember(SetMemberBinder binder, object value)
     {
+        if (!Storage.ContainsKey(binder.Name))
+            return false;
+
         Storage[binder.Name] = value;
         return true;
     }
+}
+
+public class ThrowingSetDynamicObject : DynamicStorageObject
+{
+    public override bool TrySetMember(SetMemberBinder binder, object value)
+        => throw new InvalidOperationException($"TrySetMember failed for '{binder.Name}'");
+}
 
+public class RejectingDeleteDynamicObject : DynamicStorageObject
+{
     public override bool TryDeleteMember(DeleteMemberBinder binder)
-        => binder is not null && Storage.Remove(binder.Name);
+    {
+        if (!Storage.ContainsKey(binder.Name))
+            return false;
 
-    public override IEnumerable<string> GetDynamicMemberNames() => Storage.Keys;
+        return Storage.Remove(binder.Name);
+    }
+}
+
+public class ThrowingDeleteDynamicObject : DynamicStorageObject
+{
+    public override bool TryDeleteMember(DeleteMemberBinder binder)
+        => throw new InvalidOperationException($"TryDeleteMember failed for '{binder.Name}'");
 }
diff --git a/tests/test_dynamic.py b/tests/test_dynamic.py
@@ -5,6 +5,10 @@
 from System.Dynamic import ExpandoObject
 
 from Python.Test import DynamicMappingObject
+from Python.Test import RejectingDeleteDynamicObject
+from Python.Test import RejectingSetDynamicObject
+from Python.Test import ThrowingDeleteDynamicObject
+from Python.Test import ThrowingSetDynamicObject
 
 
 def _mro_names(obj):
@@ -170,4 +174,36 @@ def custom_property(self, i):
     assert obj.custom_property == 10
 
     obj.other_property = None
-    assert obj.other_property is None
+    assert obj.other_property is None
+
+
+def test_trysetmember_false_raises_attributeerror_instead_of_silent_python_setattr():
+    obj = RejectingSetDynamicObject()
+
+    with pytest.raises(AttributeError):
+        obj.typoed_name = 42
+
+    assert not hasattr(obj, "typoed_name")
+
+
+def test_trysetmember_exception_is_raised_in_python():
+    obj = ThrowingSetDynamicObject()
+
+    with pytest.raises(Exception, match="TrySetMember failed for 'bad_name'"):
+        obj.bad_name = 42
+
+
+def test_trydeletemember_false_raises_attributeerror():
+    obj = RejectingDeleteDynamicObject()
+    obj.AddDynamicMember("existing_name", 42)
+
+    with pytest.raises(AttributeError):
+        del obj.missing_name
+
+
+def test_trydeletemember_exception_is_raised_in_python():
+    obj = ThrowingDeleteDynamicObject()
+    obj.bad_name = 42
+
+    with pytest.raises(Exception, match="TryDeleteMember failed for 'bad_name'"):
+        del obj.bad_name
