Merge pull request #415 from tjhance/static-closures

implemented kmod's suggestions

src/analysis/scoping_analysis.cpp

@@ -103,10 +103,14 @@ public:
 
     bool usesNameLookup() override { return false; }
 
-    bool isPassedToViaClosure(InternedString name) override { return false; }
-
     bool areLocalsFromModule() override { return true; }
 
+    DerefInfo getDerefInfo(InternedString) override { RELEASE_ASSERT(0, "This should never get called"); }
+    size_t getClosureOffset(InternedString) override { RELEASE_ASSERT(0, "This should never get called"); }
+    size_t getClosureSize() override { RELEASE_ASSERT(0, "This should never get called"); }
+    std::vector<std::pair<InternedString, DerefInfo>> v;
+    const std::vector<std::pair<InternedString, DerefInfo>>& getAllDerefVarsAndInfo() override { return v; }
+
     InternedString mangleName(InternedString id) override { return id; }
     InternedString internString(llvm::StringRef s) override { abort(); }
 };
@@ -165,10 +169,14 @@ public:
 
     bool usesNameLookup() override { return true; }
 
-    bool isPassedToViaClosure(InternedString name) override { return false; }
-
     bool areLocalsFromModule() override { return false; }
 
+    DerefInfo getDerefInfo(InternedString) override { RELEASE_ASSERT(0, "This should never get called"); }
+    size_t getClosureOffset(InternedString) override { RELEASE_ASSERT(0, "This should never get called"); }
+    size_t getClosureSize() override { RELEASE_ASSERT(0, "This should never get called"); }
+    std::vector<std::pair<InternedString, DerefInfo>> v;
+    const std::vector<std::pair<InternedString, DerefInfo>>& getAllDerefVarsAndInfo() override { return v; }
+
     InternedString mangleName(InternedString id) override { return id; }
     InternedString internString(llvm::StringRef s) override { abort(); }
 };
@@ -258,11 +266,22 @@ private:
     AST* ast;
     bool usesNameLookup_;
 
+    llvm::DenseMap<InternedString, size_t> closure_offsets;
+
+    std::vector<std::pair<InternedString, DerefInfo>> allDerefVarsAndInfo;
+    bool allDerefVarsAndInfoCached;
+
 public:
     ScopeInfoBase(ScopeInfo* parent, ScopingAnalysis::ScopeNameUsage* usage, AST* ast, bool usesNameLookup)
-        : parent(parent), usage(usage), ast(ast), usesNameLookup_(usesNameLookup) {
+        : parent(parent), usage(usage), ast(ast), usesNameLookup_(usesNameLookup), allDerefVarsAndInfoCached(false) {
         assert(usage);
         assert(ast);
+
+        int i = 0;
+        for (auto& p : usage->referenced_from_nested) {
+            closure_offsets[p] = i;
+            i++;
+        }
     }
 
     ~ScopeInfoBase() override { delete this->usage; }
@@ -301,20 +320,79 @@ public:
 
     bool usesNameLookup() override { return usesNameLookup_; }
 
-    bool isPassedToViaClosure(InternedString name) override {
-        if (isCompilerCreatedName(name))
-            return false;
+    bool areLocalsFromModule() override { return false; }
+
+    DerefInfo getDerefInfo(InternedString name) override {
+        assert(getScopeTypeOfName(name) == VarScopeType::DEREF);
+
+        // TODO pre-compute this?
+
+        size_t parentCounter = 0;
+        // Casting to a ScopeInfoBase* is okay because only a ScopeInfoBase can have a closure.
+        // We just walk up the scopes until we find the scope with this name. Count the number
+        // of parent links we follow, and then get the offset of the name.
+        for (ScopeInfoBase* parent = static_cast<ScopeInfoBase*>(this->parent); parent != NULL;
+             parent = static_cast<ScopeInfoBase*>(parent->parent)) {
+            if (parent->createsClosure()) {
+                auto it = parent->closure_offsets.find(name);
+                if (it != parent->closure_offsets.end()) {
+                    return DerefInfo{.num_parents_from_passed_closure = parentCounter, .offset = it->second };
+                }
+                parentCounter++;
+            }
+        }
 
-        return usage->got_from_closure.count(name) > 0 || usage->passthrough_accesses.count(name) > 0;
+        RELEASE_ASSERT(0, "Should not get here");
     }
 
-    bool areLocalsFromModule() override { return false; }
+    size_t getClosureOffset(InternedString name) override {
+        assert(getScopeTypeOfName(name) == VarScopeType::CLOSURE);
+        return closure_offsets[name];
+    }
+
+    size_t getClosureSize() override {
+        assert(createsClosure());
+        return closure_offsets.size();
+    }
 
     InternedString mangleName(const InternedString id) override {
         return pyston::mangleName(id, usage->private_name, usage->scoping->getInternedStrings());
     }
 
     InternedString internString(llvm::StringRef s) override { return usage->scoping->getInternedStrings().get(s); }
+
+    const std::vector<std::pair<InternedString, DerefInfo>>& getAllDerefVarsAndInfo() override {
+        if (!allDerefVarsAndInfoCached) {
+            allDerefVarsAndInfoCached = true;
+
+            // TODO this could probably be implemented faster
+
+            // Get all the variables that we need to return: any variable from the
+            // passed-in closure that is accessed in this scope or in a child scope.
+            StrSet allDerefs = usage->got_from_closure;
+            for (InternedString name : usage->passthrough_accesses) {
+                if (allDerefs.find(name) != allDerefs.end()) {
+                    allDerefs.insert(name);
+                }
+            }
+
+            // Call `getDerefInfo` on all of these variables and put the results in
+            // `allDerefVarsAndInfo`
+            for (InternedString name : allDerefs) {
+                allDerefVarsAndInfo.push_back({ name, getDerefInfo(name) });
+            }
+
+            // Sort in order of `num_parents_from_passed_closure`
+            std::sort(allDerefVarsAndInfo.begin(), allDerefVarsAndInfo.end(), derefComparator);
+        }
+        return allDerefVarsAndInfo;
+    }
+
+private:
+    static bool derefComparator(const std::pair<InternedString, DerefInfo>& p1,
+                                const std::pair<InternedString, DerefInfo>& p2) {
+        return p1.second.num_parents_from_passed_closure < p2.second.num_parents_from_passed_closure;
+    };
 };
 
 class NameCollectorVisitor : public ASTVisitor {

src/analysis/scoping_analysis.h

@@ -25,6 +25,16 @@ class AST_Module;
 class AST_Expression;
 class AST_Suite;
 
+// Each closure has an array (fixed-size for that particular scope) of variables
+// and a parent pointer to a parent closure. To look up a variable from the passed-in
+// closure (i.e., DEREF), you just need to know (i) how many parents up to go and
+// (ii) what offset into the array to find the variable. This struct stores that
+// information. You can query the ScopeInfo with a name to get this info.
+struct DerefInfo {
+    size_t num_parents_from_passed_closure;
+    size_t offset;
+};
+
 class ScopeInfo {
 public:
     ScopeInfo() {}
@@ -75,11 +85,28 @@ public:
     };
     virtual VarScopeType getScopeTypeOfName(InternedString name) = 0;
 
-    // Returns true if the variable should be passed via a closure to this scope.
+    // Returns true if the scope may contain NAME variables.
+    // In particular, it returns true for ClassDef scope, for any scope
+    // with an `exec` statement or `import *` statement in it, or for any
+    // `exec` or `eval` scope.
+    virtual bool usesNameLookup() = 0;
+
+    virtual bool areLocalsFromModule() = 0;
+
+    // For a variable with DEREF lookup, return the DerefInfo used to lookup
+    // the variable in a passed closure.
+    virtual DerefInfo getDerefInfo(InternedString name) = 0;
+
+    // Gets the DerefInfo for each DEREF variable accessible in the scope.
+    // The returned vector is in SORTED ORDER by the `num_parents_from_passed_closure` field
+    // (ascending). This allows the caller to iterate through the vector while also walking up
+    // the closure chain to collect all the DEREF variable values. This is useful, for example,
+    // in the implementation of locals().
+    //
     // Note that:
-    //      (a) This can be false even if there is an entry in the closure object
-    //          passed to the scope, if the variable is not actually used in this
-    //          scope or any child scopes. This can happen, because the variable
+    //      (a) This may not return a variable even if it is in the passed-in scope,
+    //          if the variable is not actually used in this scope or any child
+    //          scopes. This can happen, because the variable
     //          could be in the closure to be accessed by a different function, e.g.
     //
     //                  def f();
@@ -93,8 +120,8 @@ public:
     //                           # passed a closure object with `a` in it
     //                           print locals()
     //
-    //      (b) This can be true even if it is not used in this scope, if it
-    //          is used in a child scope. For example:
+    //      (b) This can contain a variable even if it is not access in this scope,
+    //          if it used in a child scope instead. For example:
     //
     //                  def f():
     //                       a = 0
@@ -102,19 +129,15 @@ public:
     //                           def h():
     //                               print a
     //                           print locals() # should contain `a`
-    //
-    // This is useful because it determines whether a variable from a closure
-    // into the locals() dictionary.
+    virtual const std::vector<std::pair<InternedString, DerefInfo>>& getAllDerefVarsAndInfo() = 0;
 
-    virtual bool isPassedToViaClosure(InternedString name) = 0;
-
-    // Returns true if the scope may contain NAME variables.
-    // In particular, it returns true for ClassDef scope, for any scope
-    // with an `exec` statement or `import *` statement in it, or for any
-    // `exec` or `eval` scope.
-    virtual bool usesNameLookup() = 0;
+    // For a variable with CLOSURE lookup, returns the offset within the `elts`
+    // array of a closure that this variable is stored.
+    virtual size_t getClosureOffset(InternedString name) = 0;
 
-    virtual bool areLocalsFromModule() = 0;
+    // Returns the size of the `elts` array for a closure created by this scope.
+    // Should only be called if this scope creates a closure.
+    virtual size_t getClosureSize() = 0;
 
     virtual InternedString mangleName(InternedString id) = 0;
     virtual InternedString internString(llvm::StringRef) = 0;

src/codegen/ast_interpreter.cpp

@@ -228,7 +228,7 @@ void ASTInterpreter::initArguments(int nargs, BoxedClosure* _closure, BoxedGener
     generator = _generator;
 
     if (scope_info->createsClosure())
-        created_closure = createClosure(passed_closure);
+        created_closure = createClosure(passed_closure, scope_info->getClosureSize());
 
     std::vector<Box*, StlCompatAllocator<Box*>> argsArray{ arg1, arg2, arg3 };
     for (int i = 3; i < nargs; ++i)
@@ -354,8 +354,9 @@ void ASTInterpreter::doStore(InternedString name, Value value) {
         setitem(frame_info.boxedLocals, boxString(name.str()), value.o);
     } else {
         sym_table[name] = value.o;
-        if (vst == ScopeInfo::VarScopeType::CLOSURE)
-            setattr(created_closure, name.c_str(), value.o);
+        if (vst == ScopeInfo::VarScopeType::CLOSURE) {
+            created_closure->elts[scope_info->getClosureOffset(name)] = value.o;
+        }
     }
 }
 
@@ -1086,8 +1087,20 @@ Value ASTInterpreter::visit_name(AST_Name* node) {
     switch (node->lookup_type) {
         case ScopeInfo::VarScopeType::GLOBAL:
             return getGlobal(source_info->parent_module, &node->id.str());
-        case ScopeInfo::VarScopeType::DEREF:
-            return getattr(passed_closure, node->id.c_str());
+        case ScopeInfo::VarScopeType::DEREF: {
+            DerefInfo deref_info = scope_info->getDerefInfo(node->id);
+            assert(passed_closure);
+            BoxedClosure* closure = passed_closure;
+            for (int i = 0; i < deref_info.num_parents_from_passed_closure; i++) {
+                closure = closure->parent;
+            }
+            Box* val = closure->elts[deref_info.offset];
+            if (val == NULL) {
+                raiseExcHelper(NameError, "free variable '%s' referenced before assignment in enclosing scope",
+                               node->id.c_str());
+            }
+            return val;
+        }
         case ScopeInfo::VarScopeType::FAST:
         case ScopeInfo::VarScopeType::CLOSURE: {
             SymMap::iterator it = sym_table.find(node->id);

src/codegen/compvars.cpp

@@ -1781,15 +1781,17 @@ public:
 
     CompilerVariable* getattr(IREmitter& emitter, const OpInfo& info, ConcreteCompilerVariable* var,
                               const std::string* attr, bool cls_only) override {
+        RELEASE_ASSERT(0, "should not be called\n");
+        /*
         assert(!cls_only);
         llvm::Value* bitcast = emitter.getBuilder()->CreateBitCast(var->getValue(), g.llvm_value_type_ptr);
         return ConcreteCompilerVariable(UNKNOWN, bitcast, true).getattr(emitter, info, attr, cls_only);
+        */
     }
 
     void setattr(IREmitter& emitter, const OpInfo& info, ConcreteCompilerVariable* var, const std::string* attr,
                  CompilerVariable* v) override {
-        llvm::Value* bitcast = emitter.getBuilder()->CreateBitCast(var->getValue(), g.llvm_value_type_ptr);
-        ConcreteCompilerVariable(UNKNOWN, bitcast, true).setattr(emitter, info, attr, v);
+        RELEASE_ASSERT(0, "should not be called\n");
     }
 
     ConcreteCompilerType* getConcreteType() override { return this; }

src/codegen/irgen/irgenerator.cpp

@@ -72,6 +72,21 @@ llvm::Value* IRGenState::getScratchSpace(int min_bytes) {
     return scratch_space;
 }
 
+static llvm::Value* getClosureParentGep(IREmitter& emitter, llvm::Value* closure) {
+    static_assert(sizeof(Box) == offsetof(BoxedClosure, parent), "");
+    static_assert(offsetof(BoxedClosure, parent) + sizeof(BoxedClosure*) == offsetof(BoxedClosure, nelts), "");
+    return emitter.getBuilder()->CreateConstInBoundsGEP2_32(closure, 0, 1);
+}
+
+static llvm::Value* getClosureElementGep(IREmitter& emitter, llvm::Value* closure, size_t index) {
+    static_assert(sizeof(Box) == offsetof(BoxedClosure, parent), "");
+    static_assert(offsetof(BoxedClosure, parent) + sizeof(BoxedClosure*) == offsetof(BoxedClosure, nelts), "");
+    static_assert(offsetof(BoxedClosure, nelts) + sizeof(size_t) == offsetof(BoxedClosure, elts), "");
+    return emitter.getBuilder()->CreateGEP(
+        closure,
+        { llvm::ConstantInt::get(g.i32, 0), llvm::ConstantInt::get(g.i32, 3), llvm::ConstantInt::get(g.i32, index) });
+}
+
 static llvm::Value* getBoxedLocalsGep(llvm::IRBuilder<true>& builder, llvm::Value* v) {
     static_assert(offsetof(FrameInfo, exc) == 0, "");
     static_assert(sizeof(ExcInfo) == 24, "");
@@ -898,10 +913,51 @@ private:
             assert(!is_kill);
             assert(scope_info->takesClosure());
 
+            // This is the information on how to look up the variable in the closure object.
+            DerefInfo deref_info = scope_info->getDerefInfo(node->id);
+
+            // This code is basically:
+            // closure = created_closure;
+            // closure = closure->parent;
+            // [...]
+            // closure = closure->parent;
+            // closure->elts[deref_info.offset]
+            // Where the parent lookup is done `deref_info.num_parents_from_passed_closure` times
             CompilerVariable* closure = symbol_table[internString(PASSED_CLOSURE_NAME)];
-            assert(closure);
+            llvm::Value* closureValue = closure->makeConverted(emitter, CLOSURE)->getValue();
+            closure->decvref(emitter);
+            for (int i = 0; i < deref_info.num_parents_from_passed_closure; i++) {
+                closureValue = emitter.getBuilder()->CreateLoad(getClosureParentGep(emitter, closureValue));
+            }
+            llvm::Value* lookupResult
+                = emitter.getBuilder()->CreateLoad(getClosureElementGep(emitter, closureValue, deref_info.offset));
+
+            // If the value is NULL, the variable is undefined.
+            // Create a branch on if the value is NULL.
+            llvm::BasicBlock* success_bb
+                = llvm::BasicBlock::Create(g.context, "deref_defined", irstate->getLLVMFunction());
+            success_bb->moveAfter(curblock);
+            llvm::BasicBlock* fail_bb
+                = llvm::BasicBlock::Create(g.context, "deref_undefined", irstate->getLLVMFunction());
+
+            llvm::Value* check_val
+                = emitter.getBuilder()->CreateICmpEQ(lookupResult, embedConstantPtr(NULL, g.llvm_value_type_ptr));
+            llvm::BranchInst* non_null_check = emitter.getBuilder()->CreateCondBr(check_val, fail_bb, success_bb);
+
+            // Case that it is undefined: call the assert fail function.
+            curblock = fail_bb;
+            emitter.getBuilder()->SetInsertPoint(curblock);
+
+            llvm::CallSite call = emitter.createCall(unw_info, g.funcs.assertFailDerefNameDefined,
+                                                     getStringConstantPtr(node->id.str() + '\0'));
+            call.setDoesNotReturn();
+            emitter.getBuilder()->CreateUnreachable();
+
+            // Case that it is defined: carry on in with the retrieved value.
+            curblock = success_bb;
+            emitter.getBuilder()->SetInsertPoint(curblock);
 
-            return closure->getattr(emitter, getEmptyOpInfo(unw_info), &node->id.str(), false);
+            return new ConcreteCompilerVariable(UNKNOWN, lookupResult, true);
         } else if (vst == ScopeInfo::VarScopeType::NAME) {
             llvm::Value* boxedLocals = irstate->getBoxedLocalsVar();
             llvm::Value* attr = getStringConstantPtr(node->id.str() + '\0');
@@ -1435,10 +1491,14 @@ private:
             _popFake(defined_name, true);
 
             if (vst == ScopeInfo::VarScopeType::CLOSURE) {
-                CompilerVariable* closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
-                assert(closure);
+                size_t offset = scope_info->getClosureOffset(name);
 
-                closure->setattr(emitter, getEmptyOpInfo(unw_info), &name.str(), val);
+                // This is basically `closure->elts[offset] = val;`
+                CompilerVariable* closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
+                llvm::Value* closureValue = closure->makeConverted(emitter, CLOSURE)->getValue();
+                closure->decvref(emitter);
+                llvm::Value* gep = getClosureElementGep(emitter, closureValue, offset);
+                emitter.getBuilder()->CreateStore(val->makeConverted(emitter, UNKNOWN)->getValue(), gep);
             }
         }
     }
@@ -1897,7 +1957,8 @@ private:
             assert(var->getType() != BOXED_FLOAT
                    && "should probably unbox it, but why is it boxed in the first place?");
 
-            // This line can never get hit right now for the same reason that the variables must already be concrete,
+            // This line can never get hit right now for the same reason that the variables must already be
+            // concrete,
             // because we're over-generating phis.
             ASSERT(var->isGrabbed(), "%s", p.first.c_str());
             // var->ensureGrabbed(emitter);
@@ -2157,7 +2218,8 @@ private:
             } else {
 #ifndef NDEBUG
                 if (myblock->successors.size()) {
-                    // TODO getTypeAtBlockEnd will automatically convert up to the concrete type, which we don't want
+                    // TODO getTypeAtBlockEnd will automatically convert up to the concrete type, which we don't
+                    // want
                     // here, but this is just for debugging so I guess let it happen for now:
                     ConcreteCompilerType* ending_type = types->getTypeAtBlockEnd(it->first, myblock);
                     ASSERT(it->second->canConvertTo(ending_type), "%s is supposed to be %s, but somehow is %s",
@@ -2357,7 +2419,8 @@ public:
             if (!passed_closure)
                 passed_closure = embedConstantPtr(nullptr, g.llvm_closure_type_ptr);
 
-            llvm::Value* new_closure = emitter.getBuilder()->CreateCall(g.funcs.createClosure, passed_closure);
+            llvm::Value* new_closure = emitter.getBuilder()->CreateCall2(
+                g.funcs.createClosure, passed_closure, getConstantInt(scope_info->getClosureSize(), g.i64));
             symbol_table[internString(CREATED_CLOSURE_NAME)]
                 = new ConcreteCompilerVariable(getCreatedClosureType(), new_closure, true);
         }

src/codegen/runtime_hooks.cpp

@@ -216,6 +216,7 @@ void initGlobalFuncs(GlobalState& g) {
     GET(raiseAttributeErrorStr);
     GET(raiseNotIterableError);
     GET(assertNameDefined);
+    GET(assertFailDerefNameDefined);
     GET(assertFail);
 
     GET(printFloat);

src/codegen/runtime_hooks.h

@@ -41,7 +41,7 @@ struct GlobalFuncs {
         *exceptionMatches, *yield, *getiterHelper, *hasnext;
 
     llvm::Value* unpackIntoArray, *raiseAttributeError, *raiseAttributeErrorStr, *raiseNotIterableError,
-        *assertNameDefined, *assertFail;
+        *assertNameDefined, *assertFail, *assertFailDerefNameDefined;
     llvm::Value* printFloat, *listAppendInternal, *getSysStdout;
     llvm::Value* runtimeCall0, *runtimeCall1, *runtimeCall2, *runtimeCall3, *runtimeCall, *runtimeCallN;
     llvm::Value* callattr0, *callattr1, *callattr2, *callattr3, *callattr, *callattrN;

src/codegen/unwinding.cpp

@@ -758,18 +758,21 @@ Box* fastLocalsToBoxedLocals() {
 
         // Add the locals from the closure
         // TODO in a ClassDef scope, we aren't supposed to add these
-        for (; closure != NULL; closure = closure->parent) {
-            assert(closure->cls == closure_cls);
-            for (auto& attr_offset : closure->attrs.hcls->getAttrOffsets()) {
-                const std::string& name = attr_offset.first();
-                int offset = attr_offset.second;
-                Box* val = closure->attrs.attr_list->attrs[offset];
-                if (val != NULL && scope_info->isPassedToViaClosure(scope_info->internString(name))) {
-                    Box* boxedName = boxString(name);
-                    if (d->d.count(boxedName) == 0) {
-                        d->d[boxString(name)] = val;
-                    }
-                }
+        size_t depth = 0;
+        for (auto& p : scope_info->getAllDerefVarsAndInfo()) {
+            InternedString name = p.first;
+            DerefInfo derefInfo = p.second;
+            while (depth < derefInfo.num_parents_from_passed_closure) {
+                depth++;
+                closure = closure->parent;
+            }
+            assert(closure != NULL);
+            Box* val = closure->elts[derefInfo.offset];
+            Box* boxedName = boxString(name.str());
+            if (val != NULL) {
+                d->d[boxedName] = val;
+            } else {
+                d->d.erase(boxedName);
             }
         }
 

src/runtime/inline/link_forcer.cpp

@@ -98,6 +98,7 @@ void force() {
     FORCE(raiseAttributeErrorStr);
     FORCE(raiseNotIterableError);
     FORCE(assertNameDefined);
+    FORCE(assertFailDerefNameDefined);
     FORCE(assertFail);
 
     FORCE(printFloat);

src/runtime/objmodel.cpp

@@ -233,6 +233,10 @@ extern "C" void assertNameDefined(bool b, const char* name, BoxedClass* exc_cls,
     }
 }
 
+extern "C" void assertFailDerefNameDefined(const char* name) {
+    raiseExcHelper(NameError, "free variable '%s' referenced before assignment in enclosing scope", name);
+}
+
 extern "C" void raiseAttributeErrorStr(const char* typeName, const char* attr) {
     raiseExcHelper(AttributeError, "'%s' object has no attribute '%s'", typeName, attr);
 }
@@ -1273,40 +1277,7 @@ Box* getattrInternalGeneric(Box* obj, const std::string& attr, GetattrRewriteArg
         *bind_obj_out = NULL;
     }
 
-    // TODO this should be a custom getattr
-    if (obj->cls == closure_cls) {
-        Box* val = NULL;
-        if (rewrite_args) {
-            GetattrRewriteArgs hrewrite_args(rewrite_args->rewriter, rewrite_args->obj, rewrite_args->destination);
-            val = obj->getattr(attr, &hrewrite_args);
-
-            if (!hrewrite_args.out_success) {
-                rewrite_args = NULL;
-            } else if (val) {
-                rewrite_args->out_rtn = hrewrite_args.out_rtn;
-                rewrite_args->out_success = true;
-                return val;
-            }
-        } else {
-            val = obj->getattr(attr, NULL);
-            if (val) {
-                return val;
-            }
-        }
-
-        // If val doesn't exist, then we move up to the parent closure
-        // TODO closures should get their own treatment, but now just piggy-back on the
-        // normal hidden-class IC logic.
-        // Can do better since we don't need to guard on the cls (always going to be closure)
-        BoxedClosure* closure = static_cast<BoxedClosure*>(obj);
-        if (closure->parent) {
-            if (rewrite_args) {
-                rewrite_args->obj = rewrite_args->obj->getAttr(offsetof(BoxedClosure, parent));
-            }
-            return getattrInternal(closure->parent, attr, rewrite_args);
-        }
-        raiseExcHelper(NameError, "free variable '%s' referenced before assignment in enclosing scope", attr.c_str());
-    }
+    assert(obj->cls != closure_cls);
 
     // Handle descriptor logic here.
     // A descriptor is either a data descriptor or a non-data descriptor.

src/runtime/objmodel.h

@@ -83,9 +83,10 @@ extern "C" Box* importFrom(Box* obj, const std::string* attr);
 extern "C" Box* importStar(Box* from_module, BoxedModule* to_module);
 extern "C" Box** unpackIntoArray(Box* obj, int64_t expected_size);
 extern "C" void assertNameDefined(bool b, const char* name, BoxedClass* exc_cls, bool local_var_msg);
+extern "C" void assertFailDerefNameDefined(const char* name);
 extern "C" void assertFail(BoxedModule* inModule, Box* msg);
 extern "C" bool isSubclass(BoxedClass* child, BoxedClass* parent);
-extern "C" BoxedClosure* createClosure(BoxedClosure* parent_closure);
+extern "C" BoxedClosure* createClosure(BoxedClosure* parent_closure, size_t size);
 
 Box* getiter(Box* o);
 extern "C" Box* getPystonIter(Box* o);

src/runtime/types.cpp

@@ -636,6 +636,11 @@ extern "C" void closureGCHandler(GCVisitor* v, Box* b) {
     BoxedClosure* c = (BoxedClosure*)b;
     if (c->parent)
         v->visit(c->parent);
+
+    for (int i = 0; i < c->nelts; i++) {
+        if (c->elts[i])
+            v->visit(c->elts[i]);
+    }
 }
 
 extern "C" {
@@ -849,10 +854,12 @@ extern "C" Box* createSlice(Box* start, Box* stop, Box* step) {
     return rtn;
 }
 
-extern "C" BoxedClosure* createClosure(BoxedClosure* parent_closure) {
+extern "C" BoxedClosure* createClosure(BoxedClosure* parent_closure, size_t n) {
     if (parent_closure)
         assert(parent_closure->cls == closure_cls);
-    return new BoxedClosure(parent_closure);
+    BoxedClosure* closure = new (n) BoxedClosure(parent_closure);
+    assert(closure->cls == closure_cls);
+    return closure;
 }
 
 extern "C" Box* sliceNew(Box* cls, Box* start, Box* stop, Box** args) {
@@ -2021,8 +2028,8 @@ void setupRuntime() {
                                            sizeof(BoxedSet), false, "frozenset");
     capi_getset_cls
         = BoxedHeapClass::create(type_cls, object_cls, NULL, 0, 0, sizeof(BoxedGetsetDescriptor), false, "getset");
-    closure_cls = BoxedHeapClass::create(type_cls, object_cls, &closureGCHandler, offsetof(BoxedClosure, attrs), 0,
-                                         sizeof(BoxedClosure), false, "closure");
+    closure_cls
+        = BoxedHeapClass::create(type_cls, object_cls, &closureGCHandler, 0, 0, sizeof(BoxedClosure), false, "closure");
     property_cls = BoxedHeapClass::create(type_cls, object_cls, &propertyGCHandler, 0, 0, sizeof(BoxedProperty), false,
                                           "property");
     staticmethod_cls = BoxedHeapClass::create(type_cls, object_cls, &staticmethodGCHandler, 0, 0,

src/runtime/types.h

@@ -623,15 +623,27 @@ public:
     DEFAULT_CLASS_SIMPLE(classmethod_cls);
 };
 
-// TODO is there any particular reason to make this a Box, ie a python-level object?
+// TODO is there any particular reason to make this a Box, i.e. a python-level object?
 class BoxedClosure : public Box {
 public:
-    HCAttrs attrs;
     BoxedClosure* parent;
+    size_t nelts;
+    Box* elts[0];
 
     BoxedClosure(BoxedClosure* parent) : parent(parent) {}
 
-    DEFAULT_CLASS(closure_cls);
+    void* operator new(size_t size, size_t nelts) __attribute__((visibility("default"))) {
+        /*
+        BoxedClosure* rtn
+            = static_cast<BoxedClosure*>(gc_alloc(_PyObject_VAR_SIZE(closure_cls, nelts), gc::GCKind::PYTHON));
+            */
+        BoxedClosure* rtn
+            = static_cast<BoxedClosure*>(gc_alloc(sizeof(BoxedClosure) + nelts * sizeof(Box*), gc::GCKind::PYTHON));
+        rtn->nelts = nelts;
+        rtn->cls = closure_cls;
+        memset((void*)rtn->elts, 0, sizeof(Box*) * nelts);
+        return rtn;
+    }
 };
 
 class BoxedGenerator : public Box {

test/tests/static_closure_locations.py

+# The use of c makes sure a closure gets passed through all 4 functions.
+# The use of a in g makes sure that a is in f's closure.
+# The a in j should refer to the a in h, thus throwing an exception since
+# it is undefined (that is, it should *not* access the a from f even
+# though it access via the closure).
+
+try:
+    def f():
+        c = 0
+        a = 0
+        def g():
+            print c
+            print a
+            def h():
+                print c
+                def j():
+                    print c
+                    print a
+
+                j()
+                a = 1
+            h()
+        g()
+    f()
+except NameError as ne:
+    print ne.message

tools/tester.py

@@ -162,6 +162,8 @@ def run_test(fn, check_stats, run_memcheck):
             skip = eval(skip_if)
             if skip:
                 return r + "    (skipped due to 'skip-if: %s')" % skip_if[:30]
+        elif fn.split('.')[0] in TESTS_TO_SKIP:
+                return r + "    (skipped due to command line option)"
         elif l.startswith("# allow-warning:"):
             allow_warnings.append("Warning: " + l.split(':', 1)[1].strip())
         elif l.startswith("# no-collect-stats"):