Pass closures through classdefs

Ie if a class is defined in a function, and that class contains
a function which references variables from the enclosing function,
previously the classdef would not pass the closure through and
things would fail.

Now classdefs (and other scopes) can pass through closures even if
they don't add their own variables to the closured-set.

Also, fix some bugs along the way.

src/analysis/scoping_analysis.cpp

@@ -32,6 +32,8 @@ public:
 
     virtual bool takesClosure() { return false; }
 
+    bool passesThroughClosure() override { return false; }
+
     virtual bool refersToGlobal(const std::string& name) {
         if (isCompilerCreatedName(name))
             return false;
@@ -59,6 +61,7 @@ struct ScopingAnalysis::ScopeNameUsage {
     // Properties determined by looking at other scopes as well:
     StrSet referenced_from_nested;
     StrSet got_from_closure;
+    StrSet passthrough_accesses; // what names a child scope accesses a name from a parent scope
 
     ScopeNameUsage(AST* node, ScopeNameUsage* parent) : node(node), parent(parent) {
         if (node->type == AST_TYPE::ClassDef) {
@@ -94,7 +97,9 @@ public:
 
     virtual bool createsClosure() { return usage->referenced_from_nested.size() > 0; }
 
-    virtual bool takesClosure() { return usage->got_from_closure.size() > 0; }
+    virtual bool takesClosure() { return usage->got_from_closure.size() > 0 || usage->passthrough_accesses.size() > 0; }
+
+    bool passesThroughClosure() override { return usage->passthrough_accesses.size() > 0 && !createsClosure(); }
 
     virtual bool refersToGlobal(const std::string& name) {
         // HAX
@@ -334,6 +339,7 @@ void ScopingAnalysis::processNameUsages(ScopingAnalysis::NameUsageMap* usages) {
             ScopeNameUsage* parent = usage->parent;
             while (parent) {
                 if (parent->node->type == AST_TYPE::ClassDef) {
+                    intermediate_parents.push_back(parent);
                     parent = parent->parent;
                 } else if (parent->forced_globals.count(name)) {
                     break;
@@ -342,8 +348,7 @@ void ScopingAnalysis::processNameUsages(ScopingAnalysis::NameUsageMap* usages) {
                     parent->referenced_from_nested.insert(name);
 
                     for (ScopeNameUsage* iparent : intermediate_parents) {
-                        iparent->referenced_from_nested.insert(name);
-                        iparent->got_from_closure.insert(name);
+                        iparent->passthrough_accesses.insert(name);
                     }
 
                     break;

src/analysis/scoping_analysis.h

@@ -29,6 +29,7 @@ public:
 
     virtual bool createsClosure() = 0;
     virtual bool takesClosure() = 0;
+    virtual bool passesThroughClosure() = 0;
 
     virtual bool refersToGlobal(const std::string& name) = 0;
     virtual bool refersToClosure(const std::string name) = 0;

src/codegen/irgen/irgenerator.cpp

@@ -1368,9 +1368,25 @@ private:
         }
 
         CompilerVariable* created_closure = NULL;
-        ScopeInfo* scope_info = irstate->getSourceInfo()->scoping->getScopeInfoForNode(node);
-        if (scope_info->takesClosure()) {
-            created_closure = _getFake(CREATED_CLOSURE_NAME, false);
+
+        bool takes_closure;
+        // Optimization: when compiling a module, it's nice to not have to run analyses into the
+        // entire module's source code.
+        // If we call getScopeInfoForNode, that will trigger an analysis of that function tree,
+        // but we're only using it here to figure out if that function takes a closure.
+        // Top level functions never take a closure, so we can skip the analysis.
+        if (irstate->getSourceInfo()->ast->type == AST_TYPE::Module)
+            takes_closure = false;
+        else
+            takes_closure = irstate->getSourceInfo()->scoping->getScopeInfoForNode(node)->takesClosure();
+
+        if (takes_closure) {
+            if (irstate->getScopeInfo()->createsClosure()) {
+                created_closure = _getFake(CREATED_CLOSURE_NAME, false);
+            } else {
+                assert(irstate->getScopeInfo()->passesThroughClosure());
+                created_closure = _getFake(PASSED_CLOSURE_NAME, false);
+            }
             assert(created_closure);
         }
 

src/core/cfg.cpp

@@ -626,24 +626,13 @@ private:
     }
 
     AST_expr* remapLambda(AST_Lambda* node) {
-        if (node->args->defaults.empty()) {
-            return node;
-        }
-
-        AST_Lambda* rtn = new AST_Lambda();
-        rtn->lineno = node->lineno;
-        rtn->col_offset = node->col_offset;
+        // Remap in place: see note in visit_functiondef for why.
 
-        rtn->args = new AST_arguments();
-        rtn->args->args = node->args->args;
-        rtn->args->vararg = node->args->vararg;
-        rtn->args->kwarg = node->args->kwarg;
-        for (auto d : node->args->defaults) {
-            rtn->args->defaults.push_back(remapExpr(d));
+        for (int i = 0; i < node->args->defaults.size(); i++) {
+            node->args->defaults[i] = remapExpr(node->args->defaults[i]);
         }
 
-        rtn->body = node->body;
-        return rtn;
+        return node;
     }
 
     AST_expr* remapLangPrimitive(AST_LangPrimitive* node) {
@@ -871,36 +860,42 @@ public:
     }
 
     virtual bool visit_classdef(AST_ClassDef* node) {
+        // Remap in place: see note in visit_functiondef for why.
+
+        // Decorators are evaluated before the defaults:
+        for (int i = 0; i < node->decorator_list.size(); i++) {
+            node->decorator_list[i] = remapExpr(node->decorator_list[i]);
+        }
+
+        for (int i = 0; i < node->bases.size(); i++) {
+            node->bases[i] = remapExpr(node->bases[i]);
+        }
+
         push_back(node);
         return true;
     }
 
     virtual bool visit_functiondef(AST_FunctionDef* node) {
-        if (node->args->defaults.size() == 0 && node->decorator_list.size() == 0) {
-            push_back(node);
-        } else {
-            AST_FunctionDef* remapped = new AST_FunctionDef();
-
-            remapped->name = node->name;
-            remapped->lineno = node->lineno;
-            remapped->col_offset = node->col_offset;
-            remapped->args = new AST_arguments();
-            remapped->body = node->body; // hmm shouldnt have to copy this
-
-            // Decorators are evaluated before the defaults:
-            for (auto d : node->decorator_list) {
-                remapped->decorator_list.push_back(remapExpr(d));
-            }
+        // As much as I don't like it, for now we're remapping these in place.
+        // This is because we do certain analyses pre-remapping, and associate the
+        // results with the node.  We can either do some refactoring and have a way
+        // of associating the new node with the same results, or just do the remapping
+        // in-place.
+        // Doing it in-place seems ugly, but I can't think of anything it should break,
+        // so just do that for now.
+        // TODO If we remap these (functiondefs, lambdas, classdefs) in place, we should probably
+        // remap everything in place?
 
-            remapped->args->args = node->args->args;
-            remapped->args->vararg = node->args->vararg;
-            remapped->args->kwarg = node->args->kwarg;
-            for (auto d : node->args->defaults) {
-                remapped->args->defaults.push_back(remapExpr(d));
-            }
+        // Decorators are evaluated before the defaults:
+        for (int i = 0; i < node->decorator_list.size(); i++) {
+            node->decorator_list[i] = remapExpr(node->decorator_list[i]);
+        }
 
-            push_back(remapped);
+        for (int i = 0; i < node->args->defaults.size(); i++) {
+            node->args->defaults[i] = remapExpr(node->args->defaults[i]);
         }
+
+        push_back(node);
         return true;
     }
 

test/tests/class_scoping.py

-# expected: fail
-# - WIP
-
 X = 0
 Y = 0
 Z = 0
@@ -37,10 +34,10 @@ def wrapper():
         # The defaults for a and b should resolve to the classdef definitions, and the default
         # for c should resolve to the wrapper() definition
         def f(self, a=X, b=Y, c=Z, d=W):
-            print a, b, c, W # "2 2 0 1"
+            print a, b, c, d # "2 2 0 1"
             # These references should skip all of the classdef directives,
             # and hit the definitions in the wrapper() function
-            print X, Y # "1 1"
+            print X, Y, Z, W # "1 1 1 1"
     print "done with classdef"
 
     print hasattr(C, 'X')

test/tests/classdef_ordering.py

+# expected: fail
+# - decorators
+
+def f(o, msg):
+    print msg
+    return o
+
+@f(lambda c: f(c, "calling decorator"), "evaluating decorator object")
+class C(f(object, "evaluating base")):
+    print "in classdef"

test/tests/closure_test.py

@@ -49,3 +49,23 @@ def f4(args):
          print a
      return inner
 print f4([1, 2, 3])()
+
+def f5():
+    x = 12039
+    def i1():
+        def i2():
+            print x
+        i2()
+    i1()
+f5()
+
+def f6():
+    x = 131
+    # Regression test:
+    # default args shouldn't mess with closure analysis
+    def inner(a=1):
+        print x
+    inner()
+
+    print (lambda a=1: x)()
+f6()