From ae118ea60005f7348da97ecca1c1ae11eaae86b7 Mon Sep 17 00:00:00 2001
From: Jhonathan Abreu <jdabreu25@gmail.com>
Date: Mon, 2 Dec 2024 17:07:20 -0400
Subject: [PATCH] Throw AttributeError in tp_getattro for dynamic classes

Python api documentation indicates it should throw AttributeError on failure
---
 src/embed_tests/TestPropertyAccess.cs   | 45 ++++++++++++++++++++++++-
 src/runtime/Types/DynamicClassObject.cs |  5 ++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/src/embed_tests/TestPropertyAccess.cs b/src/embed_tests/TestPropertyAccess.cs
index 54acc08f0..8dba383d6 100644
--- a/src/embed_tests/TestPropertyAccess.cs
+++ b/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();
diff --git a/src/runtime/Types/DynamicClassObject.cs b/src/runtime/Types/DynamicClassObject.cs
index 94e94b568..8a09bb85f 100644
--- a/src/runtime/Types/DynamicClassObject.cs
+++ b/src/runtime/Types/DynamicClassObject.cs
@@ -120,7 +120,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;