Throw AttributeError in tp_setattro for dynamic classes (#97)

* Throw AttributeError in tp_getattro for dynamic classes

Python api documentation indicates it should throw AttributeError on failure

* Cleanup

Author: jhonabreul

src/embed_tests/TestPropertyAccess.cs

@@ -1420,6 +1420,16 @@ public override bool TryGetMember(GetMemberBinder binder, out object result)
                 }
                 return true;
             }
+
+            public override bool TrySetMember(SetMemberBinder binder, object value)
+            {
+                if (value is PyObject pyValue && PyString.IsStringType(pyValue))
+                {
+                    throw new InvalidOperationException("Cannot set string value");
+        }
+
+                return base.TrySetMember(binder, value);
+            }
         }
 
         [Test]
@@ -1430,7 +1440,6 @@ public void TestHasAttrShouldNotThrowIfAttributeIsNotPresentForDynamicClassObjec
             dynamic module = PyModule.FromString("TestHasAttrShouldNotThrowIfAttributeIsNotPresentForDynamicClassObjects", @"
 from clr import AddReference
 AddReference(""Python.EmbeddingTest"")
-AddReference(""System"")
 
 from Python.EmbeddingTest import TestPropertyAccess
 
@@ -1466,6 +1475,40 @@ def has_attribute(obj, attribute):
             Assert.IsFalse(hasAttributeResult);
         }
 
+        [Test]
+        public void TestSetAttrShouldThrowPythonExceptionOnFailure()
+        {
+            using var _ = Py.GIL();
+
+            dynamic module = PyModule.FromString("TestHasAttrShouldNotThrowIfAttributeIsNotPresentForDynamicClassObjects", @"
+from clr import AddReference
+AddReference(""Python.EmbeddingTest"")
+
+from Python.EmbeddingTest import TestPropertyAccess
+
+class TestDynamicClass(TestPropertyAccess.ThrowingDynamicFixture):
+    pass
+
+def set_attribute(obj):
+    obj.int_attribute = 11
+
+def set_string_attribute(obj):
+    obj.string_attribute = 'string'
+");
+
+            dynamic fixture = module.GetAttr("TestDynamicClass")();
+
+            dynamic setAttribute = module.GetAttr("set_attribute");
+            Assert.DoesNotThrow(() => setAttribute(fixture));
+
+            dynamic setStringAttribute = module.GetAttr("set_string_attribute");
+            var exception = Assert.Throws<PythonException>(() => setStringAttribute(fixture));
+            Assert.AreEqual("Cannot set string value", exception.Message);
+
+            using var expectedExceptionType = new PyType(Exceptions.AttributeError);
+            Assert.AreEqual(expectedExceptionType, exception.Type);
+        }
+
         public interface IModel
         {
             void InvokeModel();

src/runtime/Types/DynamicClassObject.cs

@@ -1,7 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Dynamic;
-using System.Reflection;
 using System.Runtime.CompilerServices;
 
 using RuntimeBinder = Microsoft.CSharp.RuntimeBinder;
@@ -94,6 +92,7 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k
                     // e.g hasattr uses this method to check if the attribute exists. If we throw anything other than AttributeError,
                     // hasattr will throw instead of catching and returning False.
                     Exceptions.SetError(Exceptions.AttributeError, exception.Message);
+                    return default;
                 }
             }
 
@@ -120,7 +119,10 @@ public static int tp_setattro(BorrowedReference ob, BorrowedReference key, Borro
             // Catch C# exceptions and raise them as Python exceptions.
             catch (Exception exception)
             {
-                Exceptions.SetError(exception);
+                // tp_setattro should call PyObject_GenericSetAttr (which we already did)
+                // which must throw AttributeError on failure and return -1 (see https://docs.python.org/3/c-api/object.html#c.PyObject_GenericSetAttr)
+                Exceptions.SetError(Exceptions.AttributeError, exception.Message);
+                return -1;
             }
 
             return 0;