Merge branch 'tp_setattro'

src/capi/object.cpp

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

src/runtime/classobj.cpp

@@ -250,6 +250,8 @@ Box* instanceSetattr(Box* _inst, Box* _attr, Box* value) {
     RELEASE_ASSERT(_attr->cls == str_cls, "");
     BoxedString* attr = static_cast<BoxedString*>(_attr);
 
+    assert(value);
+
     // These are special cases in CPython as well:
     if (attr->s[0] == '_' && attr->s[1] == '_') {
         if (attr->s == "__dict__")
@@ -276,6 +278,20 @@ Box* instanceSetattr(Box* _inst, Box* _attr, Box* value) {
     return None;
 }
 
+static int instance_setattro(Box* cls, Box* attr, Box* value) noexcept {
+    try {
+        if (value) {
+            instanceSetattr(cls, attr, value);
+            return 0;
+        } else {
+            RELEASE_ASSERT(0, "");
+        }
+    } catch (ExcInfo e) {
+        setCAPIException(e);
+        return -1;
+    }
+}
+
 Box* instanceRepr(Box* _inst) {
     RELEASE_ASSERT(_inst->cls == instance_cls, "");
     BoxedInstance* inst = static_cast<BoxedInstance*>(_inst);
@@ -454,5 +470,6 @@ void setupClassobj() {
 
     instance_cls->freeze();
     instance_cls->tp_getattro = instance_getattro;
+    instance_cls->tp_setattro = instance_setattro;
 }
 }

src/runtime/objmodel.cpp

@@ -551,9 +551,15 @@ Box* Box::getattr(const std::string& attr, GetattrRewriteArgs* rewrite_args) {
 
 #if 0
     if (attr[0] == '_' && attr[1] == '_') {
-        std::string per_name_stat_name = "slowpath_box_getattr." + std::string(attr);
-        int id = Stats::getStatId(per_name_stat_name);
-        Stats::log(id);
+        // Only do this logging for potentially-avoidable cases:
+        if (!rewrite_args && cls != classobj_cls) {
+            if (attr == "__setattr__")
+                printf("");
+
+            std::string per_name_stat_name = "slowpath_box_getattr." + std::string(attr);
+            int id = Stats::getStatId(per_name_stat_name);
+            Stats::log(id);
+        }
     }
 #endif
     box_getattr_slowpath.log();
@@ -1679,6 +1685,7 @@ bool dataDescriptorSetSpecialCases(Box* obj, Box* val, Box* descr, SetattrRewrit
 }
 
 void setattrGeneric(Box* obj, const std::string& attr, Box* val, SetattrRewriteArgs* rewrite_args) {
+    assert(val);
     assert(gc::isValidGCObject(val));
 
     // TODO this should be in type_setattro
@@ -1797,38 +1804,54 @@ extern "C" void setattr(Box* obj, const char* attr, Box* attr_val) {
     std::unique_ptr<Rewriter> rewriter(
         Rewriter::createRewriter(__builtin_extract_return_addr(__builtin_return_address(0)), 3, "setattr"));
 
+    setattrofunc tp_setattro = obj->cls->tp_setattro;
+    assert(tp_setattro);
+
+    assert(!obj->cls->tp_setattr);
+
     if (rewriter.get()) {
+        auto r_cls = rewriter->getArg(0)->getAttr(offsetof(Box, cls));
         // rewriter->trap();
-        rewriter->getArg(0)->getAttr(offsetof(Box, cls))->addAttrGuard(offsetof(BoxedClass, tp_setattr), 0);
+        r_cls->addAttrGuard(offsetof(BoxedClass, tp_setattr), 0);
+        r_cls->addAttrGuard(offsetof(BoxedClass, tp_setattro), (intptr_t)tp_setattro);
     }
 
-    Box* setattr;
+
+    // Note: setattr will only be retrieved if we think it will be profitable to try calling that as opposed to
+    // the tp_setattr function pointer.
+    Box* setattr = NULL;
     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) {
-            r_setattr = rewrite_args.out_rtn;
-            // TODO this is not good enough, since the object could get collected:
-            r_setattr->addGuard((intptr_t)setattr);
+    if (tp_setattro != PyObject_GenericSetAttr) {
+        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);
+            assert(setattr);
+
+            if (rewrite_args.out_success) {
+                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 {
-            rewriter.reset(NULL);
+            // setattr = typeLookup(obj->cls, setattr_str, NULL);
         }
-    } else {
-        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);
 
+    if (DEBUG >= 2) {
+        assert((typeLookup(obj->cls, setattr_str, NULL) == object_setattr) == (tp_setattro == PyObject_GenericSetAttr));
+    }
+
     // 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 (tp_setattro == PyObject_GenericSetAttr) {
         if (rewriter.get()) {
             // rewriter->trap();
             SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
@@ -1842,9 +1865,18 @@ extern "C" void setattr(Box* obj, const char* attr, Box* attr_val) {
         return;
     }
 
-    setattr = processDescriptor(setattr, obj, obj->cls);
     Box* boxstr = boxString(attr);
-    runtimeCallInternal(setattr, NULL, ArgPassSpec(2), boxstr, attr_val, NULL, NULL, NULL);
+    if (rewriter.get()) {
+        assert(setattr);
+
+        // TODO actually rewrite this?
+        setattr = processDescriptor(setattr, obj, obj->cls);
+        runtimeCallInternal(setattr, NULL, ArgPassSpec(2), boxstr, attr_val, NULL, NULL, NULL);
+    } else {
+        int r = tp_setattro(obj, boxstr, attr_val);
+        if (r)
+            throwCAPIException();
+    }
 }
 
 bool isUserDefined(BoxedClass* cls) {
@@ -3624,18 +3656,7 @@ void Box::delattr(const std::string& attr, DelattrRewriteArgs* rewrite_args) {
     abort();
 }
 
-extern "C" void delattrInternal(Box* obj, const std::string& attr, bool allow_custom,
-                                DelattrRewriteArgs* rewrite_args) {
-    // custom __delattr__
-    if (allow_custom) {
-        Box* delAttr = typeLookup(obj->cls, delattr_str, NULL);
-        if (delAttr != NULL) {
-            Box* boxstr = boxString(attr);
-            Box* rtn = runtimeCall2(delAttr, ArgPassSpec(2), obj, boxstr);
-            return;
-        }
-    }
-
+extern "C" void delattrGeneric(Box* obj, const std::string& attr, DelattrRewriteArgs* rewrite_args) {
     // first check whether the deleting attribute is a descriptor
     Box* clsAttr = typeLookup(obj->cls, attr, NULL);
     if (clsAttr != NULL) {
@@ -3687,6 +3708,17 @@ extern "C" void delattrInternal(Box* obj, const std::string& attr, bool allow_cu
     }
 }
 
+extern "C" void delattrInternal(Box* obj, const std::string& attr, DelattrRewriteArgs* rewrite_args) {
+    Box* delAttr = typeLookup(obj->cls, delattr_str, NULL);
+    if (delAttr != NULL) {
+        Box* boxstr = boxString(attr);
+        Box* rtn = runtimeCall2(delAttr, ArgPassSpec(2), obj, boxstr);
+        return;
+    }
+
+    delattrGeneric(obj, attr, rewrite_args);
+}
+
 // del target.attr
 extern "C" void delattr(Box* obj, const char* attr) {
     static StatCounter slowpath_delattr("slowpath_delattr");
@@ -3700,7 +3732,7 @@ extern "C" void delattr(Box* obj, const char* attr) {
     }
 
 
-    delattrInternal(obj, attr, true, NULL);
+    delattrInternal(obj, attr, NULL);
 }
 
 extern "C" Box* createBoxedIterWrapper(Box* o) {

src/runtime/objmodel.h

@@ -51,6 +51,7 @@ extern "C" void my_assert(bool b);
 extern "C" Box* getattr(Box* obj, const char* attr);
 extern "C" void setattr(Box* obj, const char* attr, Box* attr_val);
 extern "C" void delattr(Box* obj, const char* attr);
+extern "C" void delattrGeneric(Box* obj, const std::string& attr, DelattrRewriteArgs* rewrite_args);
 extern "C" bool nonzero(Box* obj);
 extern "C" Box* runtimeCall(Box*, ArgPassSpec, Box*, Box*, Box*, Box**, const std::vector<const std::string*>*);
 extern "C" Box* callattr(Box*, const std::string*, CallattrFlags, ArgPassSpec, Box*, Box*, Box*, Box**,

src/runtime/types.cpp

@@ -1821,6 +1821,10 @@ void setupRuntime() {
     // Weakrefs are used for tp_subclasses:
     init_weakref();
 
+    object_cls->tp_getattro = PyObject_GenericGetAttr;
+    object_cls->tp_setattro = PyObject_GenericSetAttr;
+    add_operators(object_cls);
+
     object_cls->finishInitialization();
     type_cls->finishInitialization();
     basestring_cls->finishInitialization();
@@ -1846,9 +1850,6 @@ void setupRuntime() {
     wrapperobject_cls->finishInitialization();
     wrapperdescr_cls->finishInitialization();
 
-    object_cls->tp_getattro = PyObject_GenericGetAttr;
-    add_operators(object_cls);
-
     str_cls->tp_flags |= Py_TPFLAGS_HAVE_NEWBUFFER;
 
     dict_descr = new (pyston_getset_cls) BoxedGetsetDescriptor(typeDict, typeSetDict, NULL);
@@ -1890,7 +1891,10 @@ 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)));
+    // __setattr__ was already set to a WrapperDescriptor; it'd be nice to set this to a faster BoxedFunction
+    // object_cls->setattr("__setattr__", new BoxedFunction(boxRTFunction((void*)objectSetattr, UNKNOWN, 3)), NULL);
+    // but unfortunately that will set tp_setattro to slot_tp_setattro on object_cls and all already-made subclasses!
+    // Punting on that until needed; hopefully by then we will have better Pyston slots support.
 
     auto typeCallObj = boxRTFunction((void*)typeCall, UNKNOWN, 1, 0, true, true);
     typeCallObj->internal_callable = &typeCallInternal;
@@ -2090,6 +2094,9 @@ void setupRuntime() {
     weakref_callableproxy->simple_destructor = proxy_to_tp_clear;
     weakref_callableproxy->is_pyston_class = true;
 
+    assert(object_cls->tp_setattro == PyObject_GenericSetAttr);
+    assert(none_cls->tp_setattro == PyObject_GenericSetAttr);
+
     setupSysEnd();
 
     Stats::endOfInit();

test/tests/object_default_behavior.py

 class C(object):
+    def __delattr__(self, attr):
+        print "delattr", attr
+        if attr.startswith("c"):
+            print "yum"
+        else:
+            object.__delattr__(self, attr)
+
     def __setattr__(self, attr, value):
         print attr, value
         if attr.startswith("c"):
@@ -18,3 +25,8 @@ c.b = 2
 c.c = 3
 print sorted(c.__dict__.items())
 print c.a, c.b, c.c
+
+del c.a
+del c.b
+del c.c
+print sorted(c.__dict__.items())