Check return convention everywhere

This checking should probably be enforced by the API.

src/capi/typeobject.cpp

@@ -933,11 +933,16 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         getattribute = typeLookup(self->cls, _getattribute_str, &grewrite_args);
         if (!grewrite_args.out_success)
             rewrite_args = NULL;
-        else if (getattribute)
+        else if (getattribute) {
+            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
             r_getattribute = grewrite_args.out_rtn;
+        } else {
+            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
+        }
     } else {
         getattribute = typeLookup(self->cls, _getattribute_str, NULL);
     }
+
     // Not sure why CPython checks if getattribute is NULL since I don't think that should happen.
     // Is there some legacy way of creating types that don't inherit from object?  Anyway, I think we
     // have the right behavior even if getattribute was somehow NULL, but add an assert because that

src/runtime/classobj.cpp

@@ -313,6 +313,8 @@ static Box* instanceGetattributeSimple(BoxedInstance* inst, BoxedString* attr_st
     r = classLookup(inst->inst_cls, attr_str, rewriter_args ? &grewriter_inst_args : NULL);
     if (!grewriter_inst_args.out_success)
         rewriter_args = NULL;
+    else
+        assert(grewriter_inst_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
 
     if (r) {
         Box* rtn = processDescriptor(r, inst, inst->inst_cls);

src/runtime/objmodel.cpp

@@ -1592,6 +1592,7 @@ extern "C" Box* getclsattr(Box* obj, BoxedString* attr) {
         gotten = getclsattrInternal(obj, attr, &rewrite_args);
 
         if (rewrite_args.out_success && gotten) {
+            assert(rewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
             rewriter->commitReturning(rewrite_args.out_rtn);
         }
 #endif
@@ -1715,6 +1716,7 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
                     if (!grewrite_args.out_success) {
                         rewrite_args = NULL;
                     } else if (_get_) {
+                        assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
                         r_get = grewrite_args.out_rtn;
                     }
                 } else {
@@ -1737,6 +1739,9 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
                     _set_ = typeLookup(descr->cls, set_str, &grewrite_args);
                     if (!grewrite_args.out_success) {
                         rewrite_args = NULL;
+                    } else {
+                        assert(grewrite_args.out_return_convention
+                               == (_set_ ? GetattrRewriteArgs::VALID_RETURN : GetattrRewriteArgs::NO_RETURN));
                     }
                 } else {
                     _set_ = typeLookup(descr->cls, set_str, NULL);
@@ -1847,6 +1852,7 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
                 if (!grewrite_args.out_success) {
                     rewrite_args = NULL;
                 } else if (val) {
+                    assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
                     r_val = grewrite_args.out_rtn;
                 }
             } else {
@@ -2190,13 +2196,14 @@ void setattrGeneric(Box* obj, BoxedString* attr, Box* val, SetattrRewriteArgs* r
     // (otherwise no need to check descriptor logic)
     if (rewrite_args) {
         RewriterVar* r_cls = rewrite_args->obj->getAttr(offsetof(Box, cls), Location::any());
-        GetattrRewriteArgs crewrite_args(rewrite_args->rewriter, r_cls, rewrite_args->rewriter->getReturnDestination());
-        descr = typeLookup(obj->cls, attr, &crewrite_args);
+        GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_cls, rewrite_args->rewriter->getReturnDestination());
+        descr = typeLookup(obj->cls, attr, &grewrite_args);
 
-        if (!crewrite_args.out_success) {
+        if (!grewrite_args.out_success) {
             rewrite_args = NULL;
         } else if (descr) {
-            r_descr = crewrite_args.out_rtn;
+            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
+            r_descr = grewrite_args.out_rtn;
         }
     } else {
         descr = typeLookup(obj->cls, attr, NULL);
@@ -2213,12 +2220,13 @@ void setattrGeneric(Box* obj, BoxedString* attr, Box* val, SetattrRewriteArgs* r
 
         if (rewrite_args) {
             RewriterVar* r_cls = r_descr->getAttr(offsetof(Box, cls), Location::any());
-            GetattrRewriteArgs trewrite_args(rewrite_args->rewriter, r_cls, Location::any());
-            _set_ = typeLookup(descr->cls, set_str, &trewrite_args);
-            if (!trewrite_args.out_success) {
+            GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_cls, Location::any());
+            _set_ = typeLookup(descr->cls, set_str, &grewrite_args);
+            if (!grewrite_args.out_success) {
                 rewrite_args = NULL;
             } else if (_set_) {
-                r_set = trewrite_args.out_rtn;
+                assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
+                r_set = grewrite_args.out_rtn;
             }
         } else {
             _set_ = typeLookup(descr->cls, set_str, NULL);
@@ -2313,6 +2321,7 @@ extern "C" void setattr(Box* obj, BoxedString* attr, Box* attr_val) {
 
             if (rewrite_args.out_success) {
                 r_setattr = rewrite_args.out_rtn;
+                assert(rewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
                 // TODO this is not good enough, since the object could get collected:
                 r_setattr->addGuard((intptr_t)setattr);
             } else {
@@ -2841,6 +2850,7 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallRewrit
         if (!grewrite_args.out_success || grewrite_args.out_return_convention == GetattrRewriteArgs::NOEXC_POSSIBLE) {
             rewrite_args = NULL;
         } else if (val) {
+            assert(grewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
             r_val = grewrite_args.out_rtn;
         }
     } else {
@@ -4752,6 +4762,9 @@ static Box* callItemOrSliceAttr(Box* target, BoxedString* item_str, BoxedString*
         slice_attr = typeLookup(target->cls, slice_str, &grewrite_args);
         if (!grewrite_args.out_success) {
             rewrite_args = NULL;
+        } else {
+            assert(grewrite_args.out_return_convention
+                   == (slice_attr ? GetattrRewriteArgs::VALID_RETURN : GetattrRewriteArgs::NO_RETURN));
         }
     } else {
         slice_attr = typeLookup(target->cls, slice_str, NULL);
@@ -5228,12 +5241,14 @@ extern "C" Box* createBoxedIterWrapperIfNeeded(Box* o) {
         if (!rewrite_args.out_success) {
             rewriter.reset(NULL);
         } else if (r) {
+            assert(rewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
             rewrite_args.out_rtn->addGuard((uint64_t)r);
             if (rewrite_args.out_success) {
                 rewriter->commitReturning(r_o);
                 return o;
             }
         } else if (!r) {
+            assert(rewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
             RewriterVar* var = rewriter.get()->call(true, (void*)createBoxedIterWrapper, rewriter->getArg(0));
             if (rewrite_args.out_success) {
                 rewriter->commitReturning(var);
@@ -5735,6 +5750,11 @@ extern "C" Box* getGlobal(Box* globals, BoxedString* name) {
                 r = m->getattr(name, &rewrite_args);
                 if (!rewrite_args.out_success) {
                     rewriter.reset(NULL);
+                } else {
+                    if (r)
+                        assert(rewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
+                    else
+                        assert(rewrite_args.out_return_convention == GetattrRewriteArgs::NO_RETURN);
                 }
                 if (r) {
                     if (rewriter.get()) {
@@ -5774,6 +5794,8 @@ extern "C" Box* getGlobal(Box* globals, BoxedString* name) {
 
             if (!rtn || !rewrite_args.out_success) {
                 rewriter.reset(NULL);
+            } else {
+                assert(rewrite_args.out_return_convention == GetattrRewriteArgs::VALID_RETURN);
             }
 
             if (rewriter.get()) {

src/runtime/types.cpp

@@ -909,6 +909,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
             rewrite_args = NULL;
         else {
             assert(new_attr);
+            assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
             r_new = grewrite_args.out_rtn;
             r_new->addGuard((intptr_t)new_attr);
         }
@@ -1051,8 +1052,11 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
                 rewrite_args = NULL;
             else {
                 if (init_attr) {
+                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::VALID_RETURN);
                     r_init = grewrite_args.out_rtn;
                     r_init->addGuard((intptr_t)init_attr);
+                } else {
+                    assert(grewrite_args.out_return_convention = GetattrRewriteArgs::NO_RETURN);
                 }
             }
         } else {