Change how we handle setattr() wrt subclassing

Previously there was no way to reliably run the default setattr
behavior, which can be requested by calling object.__setattr__
or PyObject_GenericSetattr.  When those were called, we would
check to see if there is a custom __setattr__ defined and then
try to call that.  But users can and do define custom __setattr__
functions that defer to object.__setattr__, so we need a way to
run the default behavior without deferring to a custom setattr.

src/capi/object.cpp

@@ -145,7 +145,7 @@ extern "C" PyObject* PyObject_SelfIter(PyObject* obj) noexcept {
 
 extern "C" int PyObject_GenericSetAttr(PyObject* obj, PyObject* name, PyObject* value) noexcept {
     try {
-        setattr(obj, static_cast<BoxedString*>(name)->s.c_str(), value);
+        setattrGeneric(obj, static_cast<BoxedString*>(name)->s.c_str(), value, NULL);
     } catch (ExcInfo e) {
         setCAPIException(e);
         return -1;

src/runtime/builtin_modules/builtins.cpp

@@ -548,7 +548,7 @@ Box* setattrFunc(Box* obj, Box* _str, Box* value) {
     }
 
     BoxedString* str = static_cast<BoxedString*>(_str);
-    setattrInternal(obj, str->s, value, NULL);
+    setattr(obj, str->s.c_str(), value);
     return None;
 }
 

src/runtime/objmodel.cpp

@@ -187,7 +187,7 @@ extern "C" bool softspace(Box* b, bool newval) {
         } else {
             r = nonzero(gotten);
         }
-        setattrInternal(b, "softspace", boxInt(newval), NULL);
+        setattr(b, "softspace", boxInt(newval));
     } catch (ExcInfo e) {
         r = 0;
     }
@@ -1638,9 +1638,10 @@ bool dataDescriptorSetSpecialCases(Box* obj, Box* val, Box* descr, SetattrRewrit
     return false;
 }
 
-void setattrInternal(Box* obj, const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args) {
+void setattrGeneric(Box* obj, const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args) {
     assert(gc::isValidGCObject(val));
 
+    // TODO this should be in type_setattro
     if (obj->cls == type_cls) {
         BoxedClass* cobj = static_cast<BoxedClass*>(obj);
         if (!isUserDefined(cobj)) {
@@ -1710,38 +1711,13 @@ void setattrInternal(Box* obj, const std::string& attr, Box* val, SetattrRewrite
         // We don't need to to the invalidation stuff in this case.
         return;
     } else {
-        // Finally, check __setattr__
-        if (obj->cls->tp_setattr) {
-            rewrite_args = NULL;
-            REWRITE_ABORTED("");
-
-            int rtn = obj->cls->tp_setattr(obj, const_cast<char*>(attr.c_str()), val);
-            if (rtn)
-                throwCAPIException();
-            return;
-        }
-        Box* setattr = typeLookup(obj->cls, setattr_str, NULL);
-        if (setattr) {
-            rewrite_args = NULL;
-            REWRITE_ABORTED("");
-
-            // if we're dealing with a BoxedWrapperDescriptor wrapping
-            // PyObject_GenericSetAttr, skip calling __setattr__, as
-            // that will just re-enter us.
-            if (setattr->cls != wrapperdescr_cls
-                || ((BoxedWrapperDescriptor*)setattr)->wrapped != PyObject_GenericSetAttr) {
-                Box* boxstr = boxString(attr);
-                runtimeCallInternal(setattr, NULL, ArgPassSpec(3), obj, boxstr, val, NULL, NULL);
-                return;
-            }
-        }
-
         if (!obj->cls->instancesHaveHCAttrs() && !obj->cls->instancesHaveDictAttrs())
             raiseAttributeError(obj, attr.c_str());
 
         obj->setattr(attr, val, rewrite_args);
     }
 
+    // TODO this should be in type_setattro
     if (isSubclass(obj->cls, type_cls)) {
         BoxedClass* self = static_cast<BoxedClass*>(obj);
 
@@ -1771,19 +1747,64 @@ extern "C" void setattr(Box* obj, const char* attr, Box* attr_val) {
     static StatCounter slowpath_setattr("slowpath_setattr");
     slowpath_setattr.log();
 
+    if (obj->cls->tp_setattr) {
+        int rtn = obj->cls->tp_setattr(obj, const_cast<char*>(attr), attr_val);
+        if (rtn)
+            throwCAPIException();
+        return;
+    }
+
     std::unique_ptr<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
 
     if (rewriter.get()) {
         // rewriter->trap();
-        SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
-        setattrInternal(obj, attr, attr_val, &rewrite_args);
+        rewriter->getArg(0)->getAttr(offsetof(Box, cls))->addAttrGuard(offsetof(BoxedClass, tp_setattr), 0);
+    }
+
+    Box* setattr;
+    RewriterVar* r_setattr;
+    if (rewriter.get()) {
+        GetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0)->getAttr(offsetof(Box, cls)),
+                                        Location::any());
+        setattr = typeLookup(obj->cls, setattr_str, &rewrite_args);
+
         if (rewrite_args.out_success) {
-            rewriter->commit();
+            r_setattr = rewrite_args.out_rtn;
+            // TODO this is not good enough, since the object could get collected:
+            r_setattr->addGuard((intptr_t)setattr);
+        } else {
+            rewriter.reset(NULL);
         }
     } else {
-        setattrInternal(obj, attr, attr_val, NULL);
+        setattr = typeLookup(obj->cls, setattr_str, NULL);
     }
+    assert(setattr);
+
+    // We should probably add this as a GC root, but we can cheat a little bit since
+    // we know it's not going to get deallocated:
+    static Box* object_setattr = object_cls->getattr("__setattr__");
+    assert(object_setattr);
+
+    // I guess this check makes it ok for us to just rely on having guarded on the value of setattr without
+    // invalidating on deallocation, since we assume that object.__setattr__ will never get deallocated.
+    if (setattr == object_setattr) {
+        if (rewriter.get()) {
+            // rewriter->trap();
+            SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
+            setattrGeneric(obj, attr, attr_val, &rewrite_args);
+            if (rewrite_args.out_success) {
+                rewriter->commit();
+            }
+        } else {
+            setattrGeneric(obj, attr, attr_val, NULL);
+        }
+        return;
+    }
+
+    setattr = processDescriptor(setattr, obj, obj->cls);
+    Box* boxstr = boxString(attr);
+    runtimeCallInternal(setattr, NULL, ArgPassSpec(2), boxstr, attr_val, NULL, NULL, NULL);
 }
 
 bool isUserDefined(BoxedClass* cls) {
@@ -2934,6 +2955,12 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
         // Some functions are sufficiently important that we want them to be able to patchpoint themselves;
         // they can do this by setting the "internal_callable" field:
         CLFunction::InternalCallable callable = f->f->internal_callable;
+        if (rewrite_args && !rewrite_args->func_guarded) {
+            rewrite_args->obj->addGuard((intptr_t)f);
+            rewrite_args->func_guarded = true;
+            rewrite_args->rewriter->addDependenceOn(f->dependent_ics);
+        }
+
         if (callable == NULL) {
             callable = callFunc;
         }
@@ -3791,6 +3818,9 @@ Box* typeCallInternal(BoxedFunctionBase* f, CallRewriteArgs* rewrite_args, ArgPa
                       Box* arg3, Box** args, const std::vector<const std::string*>* keyword_names) {
     int npassed_args = argspec.totalPassed();
 
+    if (rewrite_args)
+        assert(rewrite_args->func_guarded);
+
     static StatCounter slowpath_typecall("slowpath_typecall");
     slowpath_typecall.log();
 

src/runtime/objmodel.h

@@ -94,7 +94,7 @@ extern "C" Box* createBoxedIterWrapperIfNeeded(Box* o);
 extern "C" void dump(void* p);
 
 struct SetattrRewriteArgs;
-void setattrInternal(Box* obj, const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args);
+void setattrGeneric(Box* obj, const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args);
 
 struct BinopRewriteArgs;
 extern "C" Box* binopInternal(Box* lhs, Box* rhs, int op_type, bool inplace, BinopRewriteArgs* rewrite_args);

src/runtime/types.cpp

@@ -1271,6 +1271,17 @@ Box* objectStr(Box* obj) {
     return obj->reprIC();
 }
 
+Box* objectSetattr(Box* obj, Box* attr, Box* value) {
+    attr = coerceUnicodeToStr(attr);
+    if (attr->cls != str_cls) {
+        raiseExcHelper(TypeError, "attribute name must be string, not '%s'", attr->cls->tp_name);
+    }
+
+    BoxedString* attr_str = static_cast<BoxedString*>(attr);
+    setattrGeneric(obj, attr_str->s, value, NULL);
+    return None;
+}
+
 static PyObject* import_copyreg(void) noexcept {
     static PyObject* copyreg_str;
 
@@ -1817,6 +1828,7 @@ void setupRuntime() {
     object_cls->giveAttr("__init__", new BoxedFunction(boxRTFunction((void*)objectInit, UNKNOWN, 1, 0, true, false)));
     object_cls->giveAttr("__repr__", new BoxedFunction(boxRTFunction((void*)objectRepr, UNKNOWN, 1, 0, false, false)));
     object_cls->giveAttr("__str__", new BoxedFunction(boxRTFunction((void*)objectStr, UNKNOWN, 1, 0, false, false)));
+    object_cls->giveAttr("__setattr__", new BoxedFunction(boxRTFunction((void*)objectSetattr, UNKNOWN, 3)));
 
     auto typeCallObj = boxRTFunction((void*)typeCall, UNKNOWN, 1, 0, true, true);
     typeCallObj->internal_callable = &typeCallInternal;

src/runtime/types.h

@@ -603,6 +603,7 @@ public:
 extern "C" void boxGCHandler(GCVisitor* v, Box* b);
 
 Box* objectNewNoArgs(BoxedClass* cls);
+Box* objectSetattr(Box* obj, Box* attr, Box* value);
 
 Box* makeAttrWrapper(Box* b);
 

test/tests/object_default_behavior.py

+class C(object):
+    def __setattr__(self, attr, value):
+        print attr, value
+        if attr.startswith("c"):
+            print "yum"
+        else:
+            object.__setattr__(self, attr, value)
+
+c = C()
+c.a = 1
+c.b = 2
+c.c = 3
+print sorted(c.__dict__.items())

test/tests/setattr.py

+class MyDescr(object):
+    def __set__(self, inst, val):
+        print type(self), type(inst), val
 
 class Test(object):
-    def __setattr__(self, name, val):
+    def _setattr__(self, name, val):
         print name, val
+        object.__setattr__(self, name, val)
+
+    foo = MyDescr()
+
+def test(t):
+    print "testing..."
+    t.hello = "world1"
+    t.hello = "world2"
+    t.foo = 2
+
+test(Test())
+Test.__setattr__ = object.__dict__['__setattr__']
+print "set setattr to object setattr"
+test(Test())
+Test.__setattr__ = Test._setattr__
+print "changed setattr to custom setattr"
+test(Test())
+del Test.__setattr__
+test(Test())
+
+
+class MyDescriptor(object):
+    def __get__(self, inst, val):
+        print type(self), type(inst), type(val)
+        return self
+
+    def __call__(self, *args):
+        print args
+class Test(object):
+    __setattr__ = MyDescriptor()
 
 t = Test()
-t.hello = "world"
+t.a = 1

test/tests/setattr_ics.py

+# skip-if: '-n' not in EXTRA_JIT_ARGS and '-O' not in EXTRA_JIT_ARGS
+# statcheck: noninit_count('slowpath_setattr') < 50
+
+class MyDescr(object):
+    def __set__(self, inst, val):
+        print type(self), type(inst), val
+
+class Test(object):
+    def _setattr__(self, name, val):
+        print name, val
+        object.__setattr__(self, name, val)
+
+    foo = MyDescr()
+
+def test(t):
+    print "testing..."
+    t.hello = "world"
+    t.hello = "world"
+    t.foo = 2
+
+for i in xrange(100):
+    test(Test())
+Test.__setattr__ = object.__dict__['__setattr__']
+print "set setattr to object setattr"
+for i in xrange(100):
+    test(Test())
+Test.__setattr__ = Test._setattr__
+print "changed setattr to custom setattr"
+test(Test())
+del Test.__setattr__
+for i in xrange(100):
+    test(Test())