Merge pull request #398 from rntz/with-exceptions

in merge:
- with_ctxclass_instance_attrs now succeeds due to d1387d74
- merge ~conflict in LangPrimitive opcodes

.gitignore

@@ -36,6 +36,7 @@ stdlib*.ll
 oprofile_data
 pprof.jit
 tags
+TAGS
 *.pyc
 
 perf.data

Makefile

@@ -37,6 +37,9 @@ VERBOSE := 0
 
 ENABLE_INTEL_JIT_EVENTS := 0
 
+CTAGS := ctags
+ETAGS := ctags-exuberant -e
+
 # Setting this to 1 will set the Makefile to use binaries from the trunk
 # directory, even if USE_TEST_LLVM is set to 1.
 # This is useful if clang isn't installed into the test directory, ex due
@@ -408,7 +411,11 @@ all: pyston_dbg pyston_release pyston_prof ext_python ext_pyston unittests
 ALL_HEADERS := $(wildcard src/*/*.h) $(wildcard src/*/*/*.h) $(wildcard from_cpython/Include/*.h)
 tags: $(SRCS) $(OPTIONAL_SRCS) $(FROM_CPYTHON_SRCS) $(ALL_HEADERS)
 	$(ECHO) Calculating tags...
-	$(VERB) ctags $^
+	$(VERB) $(CTAGS) $^
+
+TAGS: $(SRCS) $(OPTIONAL_SRCS) $(FROM_CPYTHON_SRCS) $(ALL_HEADERS)
+	$(ECHO) Calculating TAGS...
+	$(VERB) $(ETAGS) $^
 
 NON_ENTRY_OBJS := $(filter-out src/jit.o,$(OBJS))
 

src/analysis/type_analysis.cpp

@@ -511,7 +511,9 @@ private:
         }
     }
 
-    void visit_classdef(AST_ClassDef* node) override {
+    void* visit_makeclass(AST_MakeClass* mkclass) override {
+        AST_ClassDef* node = mkclass->class_def;
+
         for (auto d : node->decorator_list) {
             getType(d);
         }
@@ -521,9 +523,8 @@ private:
         }
 
         // TODO should we speculate that classdefs will generally return a class?
-        // CompilerType* t = typeFromClass(type_cls);
-        CompilerType* t = UNKNOWN;
-        _doSet(scope_info->mangleName(node->name), t);
+        // return typeFromClass(type_cls);
+        return UNKNOWN;
     }
 
     void visit_delete(AST_Delete* node) override {
@@ -551,7 +552,9 @@ private:
         }
     }
 
-    void visit_functiondef(AST_FunctionDef* node) override {
+    void* visit_makefunction(AST_MakeFunction* mkfn) override {
+        AST_FunctionDef* node = mkfn->function_def;
+
         for (auto d : node->decorator_list) {
             getType(d);
         }
@@ -563,7 +566,7 @@ private:
         CompilerType* t = UNKNOWN;
         if (node->decorator_list.empty())
             t = typeFromClass(function_cls);
-        _doSet(scope_info->mangleName(node->name), t);
+        return t;
     }
 
     void visit_global(AST_Global* node) override {}

src/codegen/ast_interpreter.cpp

@@ -88,10 +88,8 @@ private:
     Value visit_assign(AST_Assign* node);
     Value visit_binop(AST_BinOp* node);
     Value visit_call(AST_Call* node);
-    Value visit_classDef(AST_ClassDef* node);
     Value visit_compare(AST_Compare* node);
     Value visit_delete(AST_Delete* node);
-    Value visit_functionDef(AST_FunctionDef* node);
     Value visit_global(AST_Global* node);
     Value visit_module(AST_Module* node);
     Value visit_print(AST_Print* node);
@@ -117,6 +115,8 @@ private:
     Value visit_tuple(AST_Tuple* node);
     Value visit_yield(AST_Yield* node);
 
+    Value visit_makeClass(AST_MakeClass* node);
+    Value visit_makeFunction(AST_MakeFunction* node);
 
     // pseudo
     Value visit_augBinOp(AST_AugBinOp* node);
@@ -605,14 +605,10 @@ Value ASTInterpreter::visit_stmt(AST_stmt* node) {
             return visit_assert((AST_Assert*)node);
         case AST_TYPE::Assign:
             return visit_assign((AST_Assign*)node);
-        case AST_TYPE::ClassDef:
-            return visit_classDef((AST_ClassDef*)node);
         case AST_TYPE::Delete:
             return visit_delete((AST_Delete*)node);
         case AST_TYPE::Expr:
             return visit_expr((AST_Expr*)node);
-        case AST_TYPE::FunctionDef:
-            return visit_functionDef((AST_FunctionDef*)node);
         case AST_TYPE::Pass:
             return Value(); // nothing todo
         case AST_TYPE::Print:
@@ -690,7 +686,8 @@ Box* ASTInterpreter::createFunction(AST* node, AST_arguments* args, const std::v
     return boxCLFunction(cl, closure, is_generator, u.il);
 }
 
-Value ASTInterpreter::visit_functionDef(AST_FunctionDef* node) {
+Value ASTInterpreter::visit_makeFunction(AST_MakeFunction* mkfn) {
+    AST_FunctionDef* node = mkfn->function_def;
     AST_arguments* args = node->args;
 
     std::vector<Box*, StlCompatAllocator<Box*>> decorators;
@@ -702,11 +699,11 @@ Value ASTInterpreter::visit_functionDef(AST_FunctionDef* node) {
     for (int i = decorators.size() - 1; i >= 0; i--)
         func = runtimeCall(decorators[i], ArgPassSpec(1), func, 0, 0, 0, 0);
 
-    doStore(source_info->mangleName(node->name), func);
-    return Value();
+    return Value(func);
 }
 
-Value ASTInterpreter::visit_classDef(AST_ClassDef* node) {
+Value ASTInterpreter::visit_makeClass(AST_MakeClass* mkclass) {
+    AST_ClassDef* node = mkclass->class_def;
     ScopeInfo* scope_info = source_info->scoping->getScopeInfoForNode(node);
     assert(scope_info);
 
@@ -729,8 +726,7 @@ Value ASTInterpreter::visit_classDef(AST_ClassDef* node) {
     for (int i = decorators.size() - 1; i >= 0; i--)
         classobj = runtimeCall(decorators[i], ArgPassSpec(1), classobj, 0, 0, 0, 0);
 
-    doStore(source_info->mangleName(node->name), classobj);
-    return Value();
+    return Value(classobj);
 }
 
 Value ASTInterpreter::visit_raise(AST_Raise* node) {
@@ -896,6 +892,10 @@ Value ASTInterpreter::visit_expr(AST_expr* node) {
             return visit_clsAttribute((AST_ClsAttribute*)node);
         case AST_TYPE::LangPrimitive:
             return visit_langPrimitive((AST_LangPrimitive*)node);
+        case AST_TYPE::MakeClass:
+            return visit_makeClass((AST_MakeClass*)node);
+        case AST_TYPE::MakeFunction:
+            return visit_makeFunction((AST_MakeFunction*)node);
         default:
             RELEASE_ASSERT(0, "");
     };

src/codegen/compvars.cpp

@@ -757,6 +757,8 @@ CompilerVariable* makeFunction(IREmitter& emitter, CLFunction* f, CompilerVariab
 
     llvm::Value* isGenerator_v = llvm::ConstantInt::get(g.i1, isGenerator, false);
 
+    // We know this function call can't throw, so it's safe to use emitter.getBuilder()->CreateCall() rather than
+    // emitter.createCall().
     llvm::Value* boxed = emitter.getBuilder()->CreateCall(
         g.funcs.boxCLFunction,
         std::vector<llvm::Value*>{ embedConstantPtr(f, g.llvm_clfunction_type_ptr), closure_v, isGenerator_v, scratch,

src/codegen/compvars.h

@@ -291,7 +291,7 @@ private:
 
 protected:
     void drop(IREmitter& emitter) override { type->drop(emitter, this); }
-    void grab(IREmitter& emmitter) override { type->grab(emmitter, this); }
+    void grab(IREmitter& emitter) override { type->grab(emitter, this); }
 
 public:
     ValuedCompilerVariable(T* type, V value, bool grabbed) : CompilerVariable(grabbed), type(type), value(value) {

src/codegen/irgen.cpp

@@ -358,8 +358,8 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
         llvm_entry_blocks[block] = llvm::BasicBlock::Create(g.context, buf, irstate->getLLVMFunction());
     }
 
-    llvm::BasicBlock* osr_entry_block
-        = NULL; // the function entry block, where we add the type guards [no guards anymore]
+    // the function entry block, where we add the type guards [no guards anymore]
+    llvm::BasicBlock* osr_entry_block = NULL;
     llvm::BasicBlock* osr_unbox_block_end = NULL; // the block after type guards where we up/down-convert things
     ConcreteSymbolTable* osr_syms = NULL;         // syms after conversion
     if (entry_descriptor != NULL) {
@@ -518,6 +518,8 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
         created_phis[block] = phis;
 
         // Set initial symbol table:
+        // If we're in the starting block, no phis or symbol table changes for us.
+        // Generate function entry code instead.
         if (block == source->cfg->getStartingBlock()) {
             assert(entry_descriptor == NULL);
 
@@ -596,7 +598,7 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
                 ConcreteCompilerType* analyzed_type = getTypeAtBlockStart(types, p.first, block);
 
                 // printf("For %s, given %s, analyzed for %s\n", p.first.c_str(), p.second->debugName().c_str(),
-                // analyzed_type->debugName().c_str());
+                //        analyzed_type->debugName().c_str());
 
                 llvm::PHINode* phi = emitter->getBuilder()->CreatePHI(analyzed_type->llvmType(),
                                                                       block->predecessors.size() + 1, p.first.str());
@@ -652,28 +654,56 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
                 ASSERT(phi_ending_symbol_tables[pred]->size() == 0, "%d %d", block->idx, pred->idx);
                 assert(ending_symbol_tables.count(pred));
 
-                // Filter out any names set by an invoke statement at the end
-                // of the previous block, if we're in the unwind path.
-                // This definitely doesn't seem like the most elegant way to do this,
-                // but the rest of the analysis frameworks can't (yet) support the idea of
-                // a block flowing differently to its different predecessors.
+                // Filter out any names set by an invoke statement at the end of the previous block, if we're in the
+                // unwind path. This definitely doesn't seem like the most elegant way to do this, but the rest of the
+                // analysis frameworks can't (yet) support the idea of a block flowing differently to its different
+                // successors.
+                //
+                // There are four kinds of AST statements which can set a name:
+                // - Assign
+                // - ClassDef
+                // - FunctionDef
+                // - Import, ImportFrom
+                //
+                // However, all of these get translated away into Assigns, so we only need to worry about those. Also,
+                // as an invariant, all assigns that can fail assign to a temporary rather than a python name. This
+                // ensures that we interoperate properly with definedness analysis.
+                //
+                // We only need to do this in the case that we have exactly one predecessor, because:
+                // - a block ending in an invoke will have multiple successors
+                // - critical edges (block with multiple successors -> block with multiple predecessors)
+                //   are disallowed
+
                 auto pred = block->predecessors[0];
                 auto last_inst = pred->body.back();
 
                 SymbolTable* sym_table = ending_symbol_tables[pred];
                 bool created_new_sym_table = false;
-                if (last_inst->type == AST_TYPE::Invoke) {
-                    auto invoke = ast_cast<AST_Invoke>(last_inst);
-                    if (invoke->exc_dest == block && invoke->stmt->type == AST_TYPE::Assign) {
-                        auto asgn = ast_cast<AST_Assign>(invoke->stmt);
+                if (last_inst->type == AST_TYPE::Invoke && ast_cast<AST_Invoke>(last_inst)->exc_dest == block) {
+                    AST_stmt* stmt = ast_cast<AST_Invoke>(last_inst)->stmt;
+
+                    // The CFG pass translates away these statements, so we should never encounter them.
+                    // If we did, we'd need to remove a name here.
+                    assert(stmt->type != AST_TYPE::ClassDef);
+                    assert(stmt->type != AST_TYPE::FunctionDef);
+                    assert(stmt->type != AST_TYPE::Import);
+                    assert(stmt->type != AST_TYPE::ImportFrom);
+
+                    if (stmt->type == AST_TYPE::Assign) {
+                        auto asgn = ast_cast<AST_Assign>(stmt);
                         assert(asgn->targets.size() == 1);
                         if (asgn->targets[0]->type == AST_TYPE::Name) {
-                            auto name = ast_cast<AST_Name>(asgn->targets[0]);
-
-                            // TODO: inneficient
+                            InternedString name = ast_cast<AST_Name>(asgn->targets[0])->id;
+                            assert(name.c_str()[0] == '#'); // it must be a temporary
+                            // You might think I need to check whether `name' is being assigned globally or locally,
+                            // since a global assign doesn't affect the symbol table. However, the CFG pass only
+                            // generates invoke-assigns to temporary variables. Just to be sure, we assert:
+                            assert(!source->getScopeInfo()->refersToGlobal(name));
+
+                            // TODO: inefficient
                             sym_table = new SymbolTable(*sym_table);
-                            ASSERT(sym_table->count(name->id), "%d %s\n", block->idx, name->id.c_str());
-                            sym_table->erase(name->id);
+                            ASSERT(sym_table->count(name), "%d %s\n", block->idx, name.c_str());
+                            sym_table->erase(name);
                             created_new_sym_table = true;
                         }
                     }
@@ -691,21 +721,25 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
                 // Start off with the non-phi ones:
                 generator->copySymbolsFrom(ending_symbol_tables[pred]);
 
+                // NB. This is where most `typical' phi nodes get added.
                 // And go through and add phi nodes:
                 ConcreteSymbolTable* pred_st = phi_ending_symbol_tables[pred];
-                for (ConcreteSymbolTable::iterator it = pred_st->begin(); it != pred_st->end(); ++it) {
-                    // printf("adding phi for %s\n", it->first.c_str());
-                    llvm::PHINode* phi = emitter->getBuilder()->CreatePHI(it->second->getType()->llvmType(),
-                                                                          block->predecessors.size(), it->first.str());
+                for (auto it = pred_st->begin(); it != pred_st->end(); ++it) {
+                    InternedString name = it->first;
+                    ConcreteCompilerVariable* cv = it->second; // incoming CCV from predecessor block
+                    // printf("block %d: adding phi for %s from pred %d\n", block->idx, name.c_str(), pred->idx);
+                    llvm::PHINode* phi = emitter->getBuilder()->CreatePHI(cv->getType()->llvmType(),
+                                                                          block->predecessors.size(), name.str());
                     // emitter->getBuilder()->CreateCall(g.funcs.dump, phi);
-                    ConcreteCompilerVariable* var = new ConcreteCompilerVariable(it->second->getType(), phi, true);
-                    generator->giveLocalSymbol(it->first, var);
+                    ConcreteCompilerVariable* var = new ConcreteCompilerVariable(cv->getType(), phi, true);
+                    generator->giveLocalSymbol(name, var);
 
                     (*phis)[it->first] = std::make_pair(it->second->getType(), phi);
                 }
             }
         }
 
+        // Generate loop safepoints on backedges.
         for (CFGBlock* predecessor : block->predecessors) {
             if (predecessor->idx > block->idx) {
                 // Loop safepoint:
@@ -715,6 +749,7 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
             }
         }
 
+        // Generate the IR for the block.
         generator->run(block);
 
         const IRGenerator::EndingState& ending_st = generator->getEndingSymbolTable();
@@ -727,7 +762,7 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
     }
 
     ////
-    // Phi generation.
+    // Phi population.
     // We don't know the exact ssa values to back-propagate to the phi nodes until we've generated
     // the relevant IR, so after we have done all of it, go back through and populate the phi nodes.
     // Also, do some checking to make sure that the phi analysis stuff worked out, and that all blocks
@@ -739,19 +774,23 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
 
         bool this_is_osr_entry = (entry_descriptor && b == entry_descriptor->backedge->target);
 
+#ifndef NDEBUG
+        // Check to see that all blocks agree on what symbols + types they should be propagating for phis.
         for (int j = 0; j < b->predecessors.size(); j++) {
-            CFGBlock* b2 = b->predecessors[j];
-            if (blocks.count(b2) == 0)
+            CFGBlock* bpred = b->predecessors[j];
+            if (blocks.count(bpred) == 0)
                 continue;
 
-            // printf("(%d %ld) -> (%d %ld)\n", b2->idx, phi_ending_symbol_tables[b2]->size(), b->idx, phis->size());
-            compareKeyset(phi_ending_symbol_tables[b2], phis);
-            assert(phi_ending_symbol_tables[b2]->size() == phis->size());
+            // printf("(%d %ld) -> (%d %ld)\n", bpred->idx, phi_ending_symbol_tables[bpred]->size(), b->idx,
+            // phis->size());
+            assert(sameKeyset(phi_ending_symbol_tables[bpred], phis));
+            assert(phi_ending_symbol_tables[bpred]->size() == phis->size());
         }
 
         if (this_is_osr_entry) {
-            compareKeyset(osr_syms, phis);
+            assert(sameKeyset(osr_syms, phis));
         }
+#endif // end checking phi agreement.
 
         // Can't always add the phi incoming value right away, since we may have to create more
         // basic blocks as part of type coercion.
@@ -759,21 +798,22 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
         // which we won't read until after all new BBs have been added.
         std::vector<std::tuple<llvm::PHINode*, llvm::Value*, llvm::BasicBlock*&>> phi_args;
 
-        for (PHITable::iterator it = phis->begin(); it != phis->end(); ++it) {
+        for (auto it = phis->begin(); it != phis->end(); ++it) {
             llvm::PHINode* llvm_phi = it->second.second;
             for (int j = 0; j < b->predecessors.size(); j++) {
-                CFGBlock* b2 = b->predecessors[j];
-                if (blocks.count(b2) == 0)
+                CFGBlock* bpred = b->predecessors[j];
+                if (blocks.count(bpred) == 0)
                     continue;
 
-                ConcreteCompilerVariable* v = (*phi_ending_symbol_tables[b2])[it->first];
+                ConcreteCompilerVariable* v = (*phi_ending_symbol_tables[bpred])[it->first];
                 assert(v);
                 assert(v->isGrabbed());
 
                 // Make sure they all prepared for the same type:
-                ASSERT(it->second.first == v->getType(), "%d %d: %s %s %s", b->idx, b2->idx, it->first.c_str(),
+                ASSERT(it->second.first == v->getType(), "%d %d: %s %s %s", b->idx, bpred->idx, it->first.c_str(),
                        it->second.first->debugName().c_str(), v->getType()->debugName().c_str());
 
+                llvm::Value* val = v->getValue();
                 llvm_phi->addIncoming(v->getValue(), llvm_exit_blocks[b->predecessors[j]]);
             }
 
@@ -785,14 +825,13 @@ static void emitBBs(IRGenState* irstate, TypeAnalysis* types, const OSREntryDesc
                 ASSERT(it->second.first == v->getType(), "");
                 llvm_phi->addIncoming(v->getValue(), osr_unbox_block_end);
             }
-
-            InternedString is_defined_name = getIsDefinedName(it->first, source->getInternedStrings());
         }
         for (auto t : phi_args) {
             std::get<0>(t)->addIncoming(std::get<1>(t), std::get<2>(t));
         }
     }
 
+    // deallocate/dereference memory
     for (CFGBlock* b : source->cfg->blocks) {
         if (ending_symbol_tables[b] == NULL)
             continue;

src/codegen/irgen/irgenerator.cpp

@@ -291,6 +291,7 @@ private:
 
     llvm::BasicBlock* curblock;
     IREmitterImpl emitter;
+    // symbol_table tracks which (non-global) python variables are bound to which CompilerVariables
     SymbolTable symbol_table;
     std::unordered_map<CFGBlock*, llvm::BasicBlock*>& entry_blocks;
     CFGBlock* myblock;
@@ -882,6 +883,7 @@ private:
 
             return closure->getattr(emitter, getEmptyOpInfo(unw_info), &node->id.str(), false);
         } else {
+            // vst is one of {FAST, CLOSURE, NAME}
             if (symbol_table.find(node->id) == symbol_table.end()) {
                 // classdefs have different scoping rules than functions:
                 if (vst == ScopeInfo::VarScopeType::NAME) {
@@ -1087,6 +1089,133 @@ private:
         return new ConcreteCompilerVariable(UNKNOWN, rtn, true);
     }
 
+    CompilerVariable* evalMakeClass(AST_MakeClass* mkclass, UnwindInfo unw_info) {
+        assert(mkclass->type == AST_TYPE::MakeClass && mkclass->class_def->type == AST_TYPE::ClassDef);
+        AST_ClassDef* node = mkclass->class_def;
+        ScopeInfo* scope_info = irstate->getScopeInfoForNode(node);
+        assert(scope_info);
+
+        std::vector<CompilerVariable*> bases;
+        for (auto b : node->bases) {
+            CompilerVariable* base = evalExpr(b, unw_info);
+            bases.push_back(base);
+        }
+
+        CompilerVariable* _bases_tuple = makeTuple(bases);
+        for (auto b : bases) {
+            b->decvref(emitter);
+        }
+
+        ConcreteCompilerVariable* bases_tuple = _bases_tuple->makeConverted(emitter, _bases_tuple->getBoxType());
+        _bases_tuple->decvref(emitter);
+
+        std::vector<CompilerVariable*> decorators;
+        for (auto d : node->decorator_list) {
+            decorators.push_back(evalExpr(d, unw_info));
+        }
+
+        CLFunction* cl = wrapFunction(node, nullptr, node->body, irstate->getSourceInfo());
+
+        // TODO duplication with _createFunction:
+        CompilerVariable* created_closure = NULL;
+        if (scope_info->takesClosure()) {
+            created_closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
+            assert(created_closure);
+        }
+
+        // TODO kind of silly to create the function just to usually-delete it afterwards;
+        // one reason to do this is to pass the closure through if necessary,
+        // but since the classdef can't create its own closure, shouldn't need to explicitly
+        // create that scope to pass the closure through.
+        CompilerVariable* func = makeFunction(emitter, cl, created_closure, false, {});
+
+        CompilerVariable* attr_dict = func->call(emitter, getEmptyOpInfo(unw_info), ArgPassSpec(0), {}, NULL);
+
+        func->decvref(emitter);
+
+        ConcreteCompilerVariable* converted_attr_dict = attr_dict->makeConverted(emitter, attr_dict->getBoxType());
+        attr_dict->decvref(emitter);
+
+        llvm::Value* classobj = emitter.createCall3(unw_info, g.funcs.createUserClass,
+                                                    embedConstantPtr(&node->name.str(), g.llvm_str_type_ptr),
+                                                    bases_tuple->getValue(), converted_attr_dict->getValue());
+
+        // Note: createuserClass is free to manufacture non-class objects
+        CompilerVariable* cls = new ConcreteCompilerVariable(UNKNOWN, classobj, true);
+
+        for (int i = decorators.size() - 1; i >= 0; i--) {
+            cls = decorators[i]->call(emitter, getOpInfoForNode(node, unw_info), ArgPassSpec(1), { cls }, NULL);
+            decorators[i]->decvref(emitter);
+        }
+
+        // do we need to decvref this?
+        return cls;
+    }
+
+    CompilerVariable* _createFunction(AST* node, UnwindInfo unw_info, AST_arguments* args,
+                                      const std::vector<AST_stmt*>& body) {
+        CLFunction* cl = wrapFunction(node, args, body, irstate->getSourceInfo());
+
+        std::vector<ConcreteCompilerVariable*> defaults;
+        for (auto d : args->defaults) {
+            CompilerVariable* e = evalExpr(d, unw_info);
+            ConcreteCompilerVariable* converted = e->makeConverted(emitter, e->getBoxType());
+            e->decvref(emitter);
+            defaults.push_back(converted);
+        }
+
+        CompilerVariable* created_closure = NULL;
+
+        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->getScopeInfoForNode(node)->takesClosure();
+        }
+
+        bool is_generator = cl->source->is_generator;
+
+        if (takes_closure) {
+            if (irstate->getScopeInfo()->createsClosure()) {
+                created_closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
+            } else {
+                assert(irstate->getScopeInfo()->passesThroughClosure());
+                created_closure = symbol_table[internString(PASSED_CLOSURE_NAME)];
+            }
+            assert(created_closure);
+        }
+
+        CompilerVariable* func = makeFunction(emitter, cl, created_closure, is_generator, defaults);
+
+        for (auto d : defaults) {
+            d->decvref(emitter);
+        }
+
+        return func;
+    }
+
+    CompilerVariable* evalMakeFunction(AST_MakeFunction* mkfn, UnwindInfo unw_info) {
+        AST_FunctionDef* node = mkfn->function_def;
+        std::vector<CompilerVariable*> decorators;
+        for (auto d : node->decorator_list) {
+            decorators.push_back(evalExpr(d, unw_info));
+        }
+
+        CompilerVariable* func = _createFunction(node, unw_info, node->args, node->body);
+
+        for (int i = decorators.size() - 1; i >= 0; i--) {
+            func = decorators[i]->call(emitter, getOpInfoForNode(node, unw_info), ArgPassSpec(1), { func }, NULL);
+            decorators[i]->decvref(emitter);
+        }
+
+        return func;
+    }
+
     ConcreteCompilerVariable* unboxVar(ConcreteCompilerType* t, llvm::Value* v, bool grabbed) {
         if (t == BOXED_INT) {
             llvm::Value* unboxed = emitter.getBuilder()->CreateCall(g.funcs.unboxInt, v);
@@ -1172,12 +1301,19 @@ private:
                 rtn = evalYield(ast_cast<AST_Yield>(node), unw_info);
                 break;
 
+            // pseudo-nodes
             case AST_TYPE::ClsAttribute:
                 rtn = evalClsAttribute(ast_cast<AST_ClsAttribute>(node), unw_info);
                 break;
             case AST_TYPE::LangPrimitive:
                 rtn = evalLangPrimitive(ast_cast<AST_LangPrimitive>(node), unw_info);
                 break;
+            case AST_TYPE::MakeClass:
+                rtn = evalMakeClass(ast_cast<AST_MakeClass>(node), unw_info);
+                break;
+            case AST_TYPE::MakeFunction:
+                rtn = evalMakeFunction(ast_cast<AST_MakeFunction>(node), unw_info);
+                break;
             default:
                 printf("Unhandled expr type: %d (irgenerator.cpp:" STRINGIFY(__LINE__) ")\n", node->type);
                 exit(1);
@@ -1225,6 +1361,12 @@ private:
         cur = val;
     }
 
+    // whether a Python variable FOO might be undefined or not is determined by whether the corresponding is_defined_FOO
+    // variable is present in our symbol table. If it is, then it *might* be undefined. If it isn't, then it either is
+    // definitely defined, or definitely isn't.
+    //
+    // to check whether a variable is in our symbol table, call _getFake with allow_missing = true and check whether the
+    // result is NULL.
     CompilerVariable* _getFake(InternedString name, bool allow_missing = false) {
         assert(name.str()[0] == '!');
         auto it = symbol_table.find(name);
@@ -1243,6 +1385,7 @@ private:
         return rtn;
     }
 
+    // only updates symbol_table if we're *not* setting a global
     void _doSet(InternedString name, CompilerVariable* val, UnwindInfo unw_info) {
         assert(name.str() != "None");
 
@@ -1390,69 +1533,6 @@ private:
         val->decvref(emitter);
     }
 
-    void doClassDef(AST_ClassDef* node, UnwindInfo unw_info) {
-        assert(node->type == AST_TYPE::ClassDef);
-        ScopeInfo* scope_info = irstate->getScopeInfoForNode(node);
-        assert(scope_info);
-
-        std::vector<CompilerVariable*> bases;
-        for (auto b : node->bases) {
-            CompilerVariable* base = evalExpr(b, unw_info);
-            bases.push_back(base);
-        }
-
-        CompilerVariable* _bases_tuple = makeTuple(bases);
-        for (auto b : bases) {
-            b->decvref(emitter);
-        }
-
-        ConcreteCompilerVariable* bases_tuple = _bases_tuple->makeConverted(emitter, _bases_tuple->getBoxType());
-        _bases_tuple->decvref(emitter);
-
-        std::vector<CompilerVariable*> decorators;
-        for (auto d : node->decorator_list) {
-            decorators.push_back(evalExpr(d, unw_info));
-        }
-
-        CLFunction* cl = wrapFunction(node, nullptr, node->body, irstate->getSourceInfo());
-
-        // TODO duplication with _createFunction:
-        CompilerVariable* created_closure = NULL;
-        if (scope_info->takesClosure()) {
-            created_closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
-            assert(created_closure);
-        }
-
-        // TODO kind of silly to create the function just to usually-delete it afterwards;
-        // one reason to do this is to pass the closure through if necessary,
-        // but since the classdef can't create its own closure, shouldn't need to explicitly
-        // create that scope to pass the closure through.
-        CompilerVariable* func = makeFunction(emitter, cl, created_closure, false, {});
-
-        CompilerVariable* attr_dict = func->call(emitter, getEmptyOpInfo(unw_info), ArgPassSpec(0), {}, NULL);
-
-        func->decvref(emitter);
-
-        ConcreteCompilerVariable* converted_attr_dict = attr_dict->makeConverted(emitter, attr_dict->getBoxType());
-        attr_dict->decvref(emitter);
-
-
-        llvm::Value* classobj = emitter.createCall3(unw_info, g.funcs.createUserClass,
-                                                    embedConstantPtr(&node->name.str(), g.llvm_str_type_ptr),
-                                                    bases_tuple->getValue(), converted_attr_dict->getValue());
-
-        // Note: createuserClass is free to manufacture non-class objects
-        CompilerVariable* cls = new ConcreteCompilerVariable(UNKNOWN, classobj, true);
-
-        for (int i = decorators.size() - 1; i >= 0; i--) {
-            cls = decorators[i]->call(emitter, getOpInfoForNode(node, unw_info), ArgPassSpec(1), { cls }, NULL);
-            decorators[i]->decvref(emitter);
-        }
-
-        _doSet(irstate->getSourceInfo()->mangleName(node->name), cls, unw_info);
-        cls->decvref(emitter);
-    }
-
     void doDelete(AST_Delete* node, UnwindInfo unw_info) {
         for (AST_expr* target : node->targets) {
             switch (target->type) {
@@ -1544,70 +1624,6 @@ private:
         symbol_table.erase(target->id);
     }
 
-    CompilerVariable* _createFunction(AST* node, UnwindInfo unw_info, AST_arguments* args,
-                                      const std::vector<AST_stmt*>& body) {
-        CLFunction* cl = wrapFunction(node, args, body, irstate->getSourceInfo());
-
-        std::vector<ConcreteCompilerVariable*> defaults;
-        for (auto d : args->defaults) {
-            CompilerVariable* e = evalExpr(d, unw_info);
-            ConcreteCompilerVariable* converted = e->makeConverted(emitter, e->getBoxType());
-            e->decvref(emitter);
-            defaults.push_back(converted);
-        }
-
-        CompilerVariable* created_closure = NULL;
-
-        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->getScopeInfoForNode(node)->takesClosure();
-        }
-
-        bool is_generator = cl->source->is_generator;
-
-        if (takes_closure) {
-            if (irstate->getScopeInfo()->createsClosure()) {
-                created_closure = symbol_table[internString(CREATED_CLOSURE_NAME)];
-            } else {
-                assert(irstate->getScopeInfo()->passesThroughClosure());
-                created_closure = symbol_table[internString(PASSED_CLOSURE_NAME)];
-            }
-            assert(created_closure);
-        }
-
-        CompilerVariable* func = makeFunction(emitter, cl, created_closure, is_generator, defaults);
-
-        for (auto d : defaults) {
-            d->decvref(emitter);
-        }
-
-        return func;
-    }
-
-    void doFunctionDef(AST_FunctionDef* node, UnwindInfo unw_info) {
-        std::vector<CompilerVariable*> decorators;
-        for (auto d : node->decorator_list) {
-            decorators.push_back(evalExpr(d, unw_info));
-        }
-
-        CompilerVariable* func = _createFunction(node, unw_info, node->args, node->body);
-
-        for (int i = decorators.size() - 1; i >= 0; i--) {
-            func = decorators[i]->call(emitter, getOpInfoForNode(node, unw_info), ArgPassSpec(1), { func }, NULL);
-            decorators[i]->decvref(emitter);
-        }
-
-        _doSet(irstate->getSourceInfo()->mangleName(node->name), func, unw_info);
-        func->decvref(emitter);
-    }
-
     void doPrint(AST_Print* node, UnwindInfo unw_info) {
         ConcreteCompilerVariable* dest = NULL;
         if (node->dest) {
@@ -1702,7 +1718,7 @@ private:
         // that case asking it to convert to itself ends up just being an incvref
         // and doesn't end up emitting an incref+decref pair.
         // This could also be handled by casting from the CompilerVariable to
-        // ConcreteCOmpilerVariable, but this way feels a little more robust to me.
+        // ConcreteCompilerVariable, but this way feels a little more robust to me.
         ConcreteCompilerType* opt_rtn_type = irstate->getReturnType();
         if (irstate->getReturnType()->llvmType() == val->getConcreteType()->llvmType())
             opt_rtn_type = val->getConcreteType();
@@ -1976,18 +1992,12 @@ private:
             case AST_TYPE::Assign:
                 doAssign(ast_cast<AST_Assign>(node), unw_info);
                 break;
-            case AST_TYPE::ClassDef:
-                doClassDef(ast_cast<AST_ClassDef>(node), unw_info);
-                break;
             case AST_TYPE::Delete:
                 doDelete(ast_cast<AST_Delete>(node), unw_info);
                 break;
             case AST_TYPE::Expr:
                 doExpr(ast_cast<AST_Expr>(node), unw_info);
                 break;
-            case AST_TYPE::FunctionDef:
-                doFunctionDef(ast_cast<AST_FunctionDef>(node), unw_info);
-                break;
             // case AST_TYPE::If:
             // doIf(ast_cast<AST_If>(node));
             // break;
@@ -2065,9 +2075,9 @@ private:
         SourceInfo* source = irstate->getSourceInfo();
         ScopeInfo* scope_info = irstate->getScopeInfo();
 
-        // Additional names to remove; remove them after iteration is done to new mess up the iterators
+        // Additional names to remove; remove them after iteration is done to not mess up the iterators
         std::vector<InternedString> also_remove;
-        for (SymbolTable::iterator it = symbol_table.begin(); it != symbol_table.end();) {
+        for (auto it = symbol_table.begin(); it != symbol_table.end();) {
             if (allowableFakeEndingSymbol(it->first)) {
                 ++it;
                 continue;
@@ -2078,7 +2088,7 @@ private:
 
             if (!source->liveness->isLiveAtEnd(it->first, myblock)) {
                 // printf("%s dead at end of %d; grabbed = %d, %d vrefs\n", it->first.c_str(), myblock->idx,
-                // it->second->isGrabbed(), it->second->getVrefs());
+                //        it->second->isGrabbed(), it->second->getVrefs());
                 also_remove.push_back(getIsDefinedName(it->first));
 
                 it->second->decvref(emitter);
@@ -2213,6 +2223,8 @@ public:
             return EndingState(st, phi_st, curblock);
         }
 
+        // We have one successor, but they have more than one predecessor.
+        // We're going to sort out which symbols need to go in phi_st and which belong inst.
         for (SymbolTable::iterator it = st->begin(); it != st->end();) {
             if (allowableFakeEndingSymbol(it->first) || source->phis->isRequiredAfter(it->first, myblock)) {
                 ASSERT(it->second->isGrabbed(), "%s", it->first.c_str());
@@ -2350,12 +2362,24 @@ public:
     }
 
     void run(const CFGBlock* block) override {
+        if (VERBOSITY("irgenerator") >= 1) { // print starting symbol table
+            printf("  %d init:", block->idx);
+            for (auto it = symbol_table.begin(); it != symbol_table.end(); ++it)
+                printf(" %s", it->first.c_str());
+            printf("\n");
+        }
         for (int i = 0; i < block->body.size(); i++) {
             if (state == DEAD)
                 break;
             assert(state != FINISHED);
             doStmt(block->body[i], UnwindInfo(block->body[i], NULL));
         }
+        if (VERBOSITY("irgenerator") >= 1) { // print ending symbol table
+            printf("  %d fini:", block->idx);
+            for (auto it = symbol_table.begin(); it != symbol_table.end(); ++it)
+                printf(" %s", it->first.c_str());
+            printf("\n");
+        }
     }
 
     void doSafePoint() override { emitter.getBuilder()->CreateCall(g.funcs.allowGLReadPreemption); }

src/codegen/irgen/irgenerator.h

@@ -95,10 +95,14 @@ public:
     ParamNames* getParamNames() { return param_names; }
 };
 
+// turns CFGBlocks into LLVM IR
 class IRGenerator {
 private:
 public:
     struct EndingState {
+        // symbol_table records which Python variables are bound to what CompilerVariables at the end of this block.
+        // phi_symbol_table records the ones that will need to be `phi'd.
+        // both only record non-globals.
         SymbolTable* symbol_table;
         ConcreteSymbolTable* phi_symbol_table;
         llvm::BasicBlock* ending_block;
@@ -113,7 +117,7 @@ public:
 
     virtual void giveLocalSymbol(InternedString name, CompilerVariable* var) = 0;
     virtual void copySymbolsFrom(SymbolTable* st) = 0;
-    virtual void run(const CFGBlock* block) = 0;
+    virtual void run(const CFGBlock* block) = 0; // primary entry point
     virtual EndingState getEndingSymbolTable() = 0;
     virtual void doSafePoint() = 0;
     virtual void addFrameStackmapArgs(PatchpointInfo* pp, AST_stmt* current_stmt,

src/core/ast.cpp

@@ -953,7 +953,6 @@ void AST_Branch::accept_stmt(StmtVisitor* v) {
     v->visit_branch(this);
 }
 
-
 void AST_Jump::accept(ASTVisitor* v) {
     bool skip = v->visit_jump(this);
     if (skip)
@@ -976,7 +975,29 @@ void* AST_ClsAttribute::accept_expr(ExprVisitor* v) {
     return v->visit_clsattribute(this);
 }
 
+void AST_MakeFunction::accept(ASTVisitor* v) {
+    bool skip = v->visit_makefunction(this);
+    if (skip)
+        return;
+
+    function_def->accept(v);
+}
+
+void* AST_MakeFunction::accept_expr(ExprVisitor* v) {
+    return v->visit_makefunction(this);
+}
 
+void AST_MakeClass::accept(ASTVisitor* v) {
+    bool skip = v->visit_makeclass(this);
+    if (skip)
+        return;
+
+    class_def->accept(v);
+}
+
+void* AST_MakeClass::accept_expr(ExprVisitor* v) {
+    return v->visit_makeclass(this);
+}
 
 void print_ast(AST* ast) {
     PrintVisitor v;
@@ -1851,6 +1872,16 @@ bool PrintVisitor::visit_clsattribute(AST_ClsAttribute* node) {
     return true;
 }
 
+bool PrintVisitor::visit_makefunction(AST_MakeFunction* node) {
+    printf("make_");
+    return false;
+}
+
+bool PrintVisitor::visit_makeclass(AST_MakeClass* node) {
+    printf("make_");
+    return false;
+}
+
 class FlattenVisitor : public ASTVisitor {
 private:
     std::vector<AST*>* output;
@@ -2098,6 +2129,15 @@ public:
         output->push_back(node);
         return false;
     }
+
+    virtual bool visit_makeclass(AST_MakeClass* node) {
+        output->push_back(node);
+        return false;
+    }
+    virtual bool visit_makefunction(AST_MakeFunction* node) {
+        output->push_back(node);
+        return false;
+    }
 };
 
 void flatten(const std::vector<AST_stmt*>& roots, std::vector<AST*>& output, bool expand_scopes) {

src/core/ast.h

@@ -118,7 +118,7 @@ enum AST_TYPE {
     DictComp = 15,
     Set = 43,
     Ellipsis = 87,
-    Expression = 88,
+    Expression = 88, // like Module, but used for eval.
     SetComp = 89,
 
     // Pseudo-nodes that are specific to this compiler:
@@ -128,6 +128,8 @@ enum AST_TYPE {
     AugBinOp = 203,
     Invoke = 204,
     LangPrimitive = 205,
+    MakeClass = 206,    // wraps a ClassDef to make it an expr
+    MakeFunction = 207, // wraps a FunctionDef to make it an expr
 
     // These aren't real AST types, but since we use AST types to represent binexp types
     // and divmod+truediv are essentially types of binops, we add them here (at least for now):
@@ -951,6 +953,31 @@ public:
     static const AST_TYPE::AST_TYPE TYPE = AST_TYPE::Yield;
 };
 
+class AST_MakeFunction : public AST_expr {
+public:
+    AST_FunctionDef* function_def;
+
+    virtual void accept(ASTVisitor* v);
+    virtual void* accept_expr(ExprVisitor* v);
+
+    AST_MakeFunction(AST_FunctionDef* fd)
+        : AST_expr(AST_TYPE::MakeFunction, fd->lineno, fd->col_offset), function_def(fd) {}
+
+    static const AST_TYPE::AST_TYPE TYPE = AST_TYPE::MakeFunction;
+};
+
+class AST_MakeClass : public AST_expr {
+public:
+    AST_ClassDef* class_def;
+
+    virtual void accept(ASTVisitor* v);
+    virtual void* accept_expr(ExprVisitor* v);
+
+    AST_MakeClass(AST_ClassDef* cd) : AST_expr(AST_TYPE::MakeClass, cd->lineno, cd->col_offset), class_def(cd) {}
+
+    static const AST_TYPE::AST_TYPE TYPE = AST_TYPE::MakeClass;
+};
+
 
 // AST pseudo-nodes that will get added during CFG-construction.  These don't exist in the input AST, but adding them in
 // lets us avoid creating a completely new IR for this phase
@@ -1019,14 +1046,14 @@ public:
 class AST_LangPrimitive : public AST_expr {
 public:
     enum Opcodes {
-        LANDINGPAD,
+        LANDINGPAD, // grabs the info about the last raised exception
         LOCALS,
         GET_ITER,
         IMPORT_FROM,
         IMPORT_NAME,
         IMPORT_STAR,
         NONE,
-        NONZERO,
+        NONZERO, // determines whether something is "true" for purposes of `if' and so forth
         CHECK_EXC_MATCH,
         SET_EXC_INFO,
         UNCACHE_EXC_INFO,
@@ -1115,6 +1142,8 @@ public:
     virtual bool visit_with(AST_With* node) { RELEASE_ASSERT(0, ""); }
     virtual bool visit_yield(AST_Yield* node) { RELEASE_ASSERT(0, ""); }
 
+    virtual bool visit_makeclass(AST_MakeClass* node) { RELEASE_ASSERT(0, ""); }
+    virtual bool visit_makefunction(AST_MakeFunction* node) { RELEASE_ASSERT(0, ""); }
     virtual bool visit_branch(AST_Branch* node) { RELEASE_ASSERT(0, ""); }
     virtual bool visit_jump(AST_Jump* node) { RELEASE_ASSERT(0, ""); }
 };
@@ -1187,6 +1216,8 @@ public:
 
     virtual bool visit_branch(AST_Branch* node) { return false; }
     virtual bool visit_jump(AST_Jump* node) { return false; }
+    virtual bool visit_makeclass(AST_MakeClass* node) { return false; }
+    virtual bool visit_makefunction(AST_MakeFunction* node) { return false; }
 };
 
 class ExprVisitor {
@@ -1223,6 +1254,8 @@ public:
     virtual void* visit_tuple(AST_Tuple* node) { RELEASE_ASSERT(0, ""); }
     virtual void* visit_unaryop(AST_UnaryOp* node) { RELEASE_ASSERT(0, ""); }
     virtual void* visit_yield(AST_Yield* node) { RELEASE_ASSERT(0, ""); }
+    virtual void* visit_makeclass(AST_MakeClass* node) { RELEASE_ASSERT(0, ""); }
+    virtual void* visit_makefunction(AST_MakeFunction* node) { RELEASE_ASSERT(0, ""); }
 };
 
 class StmtVisitor {
@@ -1332,6 +1365,8 @@ public:
 
     virtual bool visit_branch(AST_Branch* node);
     virtual bool visit_jump(AST_Jump* node);
+    virtual bool visit_makefunction(AST_MakeFunction* node);
+    virtual bool visit_makeclass(AST_MakeClass* node);
 };
 
 // Given an AST node, return a vector of the node plus all its descendents.

src/core/cfg.cpp

@@ -36,7 +36,8 @@ void CFGBlock::connectTo(CFGBlock* successor, bool allow_backedge) {
 
     if (!allow_backedge) {
         assert(this->idx >= 0);
-        ASSERT(successor->idx == -1 || successor->idx > this->idx, "edge from %d to %d", this->idx, successor->idx);
+        ASSERT(successor->idx == -1 || successor->idx > this->idx, "edge from %d (%s) to %d (%s)", this->idx,
+               this->info, successor->idx, successor->info);
     }
     // assert(successors.count(successor) == 0);
     // assert(successor->predecessors.count(this) == 0);
@@ -53,16 +54,123 @@ void CFGBlock::unconnectFrom(CFGBlock* successor) {
                                   successor->predecessors.end());
 }
 
+void CFGBlock::print() {
+    printf("Block %d", idx);
+    if (info)
+        printf(" '%s'", info);
+
+    printf("; Predecessors:");
+    for (int j = 0; j < predecessors.size(); j++) {
+        printf(" %d", predecessors[j]->idx);
+    }
+    printf(" Successors:");
+    for (int j = 0; j < successors.size(); j++) {
+        printf(" %d", successors[j]->idx);
+    }
+    printf("\n");
+
+    PrintVisitor pv(4);
+    for (int j = 0; j < body.size(); j++) {
+        printf("    ");
+        body[j]->accept(&pv);
+        printf("\n");
+    }
+}
+
 static const std::string RETURN_NAME("#rtnval");
 
+// The various reasons why a `finally' block (or similar, eg. a `with' exit block) might get entered.
+// this has to go outside CFGVisitor b/c why_values can't go inside it.
+enum Why : int8_t {
+    FALLTHROUGH, // i.e. normal control flow
+    CONTINUE,
+    BREAK,
+    RETURN,
+    EXCEPTION,
+};
+
+static const Why why_values[] = { FALLTHROUGH, CONTINUE, BREAK, RETURN, EXCEPTION };
+
 class CFGVisitor : public ASTVisitor {
+    // ---------- Types ----------
+private:
+    /* Explanation of ContInfo and ExcBlockInfo:
+     *
+     * While generating the CFG, we need to know what to do if we:
+     * 1. hit a `continue'
+     * 2. hit a `break'
+     * 3. hit a `return'
+     * 4. raise an exception
+     *
+     * We call these "continuations", because they're what we "continue on to" after these conditions occur.
+     *
+     * Various control flow constructs affect each of these:
+     * - `for' and `while' affect (1-2).
+     * - `try/except' affects (4).
+     * - `try/finally' and `with' affect all four.
+     *
+     * Each of these take effect only within some chunk of code. So, notionally, we keep a stack for each of (1-4) whose
+     * _top_ value says what to do if that condition occurs. The top of the continue-stack points to the block to jump
+     * to if we hit a `continue', etc.
+     *
+     * For example, when we enter a loop, we push a pointer to the head of the loop onto the continue-stack, and a
+     * pointer to the code after the loop onto the break-stack. When we visit a `break' in the loop body, we emit a jump
+     * to the top of the break-stack, which is the end of the loop. After we finish visiting the loop body, we pop the
+     * break- & continue-stacks, restoring our old state (maybe we were inside another loop, for example).
+     *
+     * It's more complicated in practice, because:
+     *
+     * 1. When we jump to a `finally' block, we must tell it *why* we jumped to it. After the `finally' block finishes,
+     *    it uses this info to resume what we were doing before we entered it (returning, raising an exception, etc).
+     *
+     * 2. When we jump to a `except' block, we must record three pieces of information about the exception (its type,
+     *    value, and traceback).
+     *
+     * So instead of four stacks of block pointers, instead we have two stacks:
+     * - `continuations', a stack of ContInfos, for `continue', `break', and `return'
+     * - `exc_handlers', a stack of ExcBlockInfos, for exceptions
+     *
+     * Read the comments in ContInfo & ExcBlockInfo for more information.
+     */
+    struct ContInfo {
+        // where to jump to if a continue, break, or return happens respectively
+        CFGBlock* continue_dest, *break_dest, *return_dest;
+        // true if this continuation needs to know the reason why we entered it. `finally' blocks use this info to
+        // determine how to resume execution after they finish.
+        bool say_why;
+        // bit-vector tracking all reasons Why we ever might enter this continuation. is only updated/used if `say_why'
+        // is true. when we emit a jump to this continuation for reason w, we set the bit (did_why & (1 << w)). this is
+        // used when emitting `finally' blocks to determine which continuation-cases to emit.
+        int did_why;
+        // name of the variable to store the reason Why we jumped in.
+        InternedString why_name;
+
+        ContInfo(CFGBlock* continue_dest, CFGBlock* break_dest, CFGBlock* return_dest, bool say_why,
+                 InternedString why_name)
+            : continue_dest(continue_dest), break_dest(break_dest), return_dest(return_dest), say_why(say_why),
+              did_why(0), why_name(why_name) {}
+    };
+
+    struct ExcBlockInfo {
+        // where to jump in case of an exception
+        CFGBlock* exc_dest;
+        // variable names to store the exception (type, value, traceback) in
+        InternedString exc_type_name, exc_value_name, exc_traceback_name;
+    };
+
+    // ---------- Member fields ----------
 private:
     SourceInfo* source;
+    // `root_type' is the type of the root of the AST tree that we are turning
+    // into a CFG. Used when we find a "return" to check that we're inside a
+    // function (otherwise we SyntaxError).
     AST_TYPE::AST_TYPE root_type;
     FutureFlags future_flags;
     CFG* cfg;
     CFGBlock* curblock;
     ScopingAnalysis* scoping_analysis;
+    std::vector<ContInfo> continuations;
+    std::vector<ExcBlockInfo> exc_handlers;
 
     friend CFG* computeCFG(SourceInfo* source, std::vector<AST_stmt*> body);
 
@@ -78,32 +186,12 @@ public:
     ~CFGVisitor() {
         // if we're being destroyed due to an exception, our internal invariants may be violated, but that's okay; the
         // CFG isn't going to get used anyway. (Maybe we should check that it won't be used somehow?)
-        assert(regions.size() == 0 || std::uncaught_exception());
+        assert(continuations.size() == 0 || std::uncaught_exception());
         assert(exc_handlers.size() == 0 || std::uncaught_exception());
     }
 
+    // ---------- private methods ----------
 private:
-    enum Why : int8_t {
-        FALLTHROUGH,
-        CONTINUE,
-        BREAK,
-        RETURN,
-        EXCEPTION,
-    };
-
-    // My first thought is to call this BlockInfo, but this is separate from the idea of cfg blocks.
-    struct RegionInfo {
-        CFGBlock* continue_dest, *break_dest, *return_dest;
-        bool say_why;
-        int did_why;
-        InternedString why_name;
-
-        RegionInfo(CFGBlock* continue_dest, CFGBlock* break_dest, CFGBlock* return_dest, bool say_why,
-                   InternedString why_name)
-            : continue_dest(continue_dest), break_dest(break_dest), return_dest(return_dest), say_why(say_why),
-              did_why(0), why_name(why_name) {}
-    };
-
     template <typename T> InternedString internString(T&& s) {
         return source->getInternedStrings().get(std::forward<T>(s));
     }
@@ -113,49 +201,33 @@ private:
         return name;
     }
 
-    std::vector<RegionInfo> regions;
-
-    struct ExcBlockInfo {
-        CFGBlock* exc_dest;
-        InternedString exc_type_name, exc_value_name, exc_traceback_name;
-    };
-    std::vector<ExcBlockInfo> exc_handlers;
+    AST_Name* makeLoad(InternedString id, AST* node) { return makeName(id, AST_TYPE::Load, node->lineno); }
 
-    void pushLoopRegion(CFGBlock* continue_dest, CFGBlock* break_dest) {
+    void pushLoopContinuation(CFGBlock* continue_dest, CFGBlock* break_dest) {
         assert(continue_dest
                != break_dest); // I guess this doesn't have to be true, but validates passing say_why=false
-        regions.emplace_back(continue_dest, break_dest, nullptr, false, internString(""));
+        continuations.emplace_back(continue_dest, break_dest, nullptr, false, internString(""));
     }
 
-    void pushFinallyRegion(CFGBlock* finally_block, InternedString why_name) {
-        regions.emplace_back(finally_block, finally_block, finally_block, true, why_name);
+    void pushFinallyContinuation(CFGBlock* finally_block, InternedString why_name) {
+        continuations.emplace_back(finally_block, finally_block, finally_block, true, why_name);
     }
 
-    void popRegion() { regions.pop_back(); }
-
-    // XXX get rid of this
-    void pushReturnRegion(CFGBlock* return_dest) {
-        regions.emplace_back(nullptr, nullptr, return_dest, false, internString(""));
-    }
+    void popContinuation() { continuations.pop_back(); }
 
     void doReturn(AST_expr* value) {
         assert(value);
         assert(curblock);
 
-        for (auto& region : llvm::make_range(regions.rbegin(), regions.rend())) {
-            if (region.return_dest) {
-                if (region.say_why) {
-                    pushAssign(region.why_name, makeNum(Why::RETURN));
-                    region.did_why |= (1 << Why::RETURN);
+        for (auto& cont : llvm::make_range(continuations.rbegin(), continuations.rend())) {
+            if (cont.return_dest) {
+                if (cont.say_why) {
+                    pushAssign(cont.why_name, makeNum(Why::RETURN));
+                    cont.did_why |= (1 << Why::RETURN);
                 }
 
                 pushAssign(internString(RETURN_NAME), value);
-
-                AST_Jump* j = makeJump();
-                j->target = region.return_dest;
-                curblock->connectTo(region.return_dest);
-                push_back(j);
-                curblock = NULL;
+                pushJump(cont.return_dest);
                 return;
             }
         }
@@ -170,18 +242,14 @@ private:
 
     void doContinue() {
         assert(curblock);
-        for (auto& region : llvm::make_range(regions.rbegin(), regions.rend())) {
-            if (region.continue_dest) {
-                if (region.say_why) {
-                    pushAssign(region.why_name, makeNum(Why::CONTINUE));
-                    region.did_why |= (1 << Why::CONTINUE);
+        for (auto& cont : llvm::make_range(continuations.rbegin(), continuations.rend())) {
+            if (cont.continue_dest) {
+                if (cont.say_why) {
+                    pushAssign(cont.why_name, makeNum(Why::CONTINUE));
+                    cont.did_why |= (1 << Why::CONTINUE);
                 }
 
-                AST_Jump* j = makeJump();
-                j->target = region.continue_dest;
-                curblock->connectTo(region.continue_dest, true);
-                push_back(j);
-                curblock = NULL;
+                pushJump(cont.continue_dest, true);
                 return;
             }
         }
@@ -191,18 +259,14 @@ private:
 
     void doBreak() {
         assert(curblock);
-        for (auto& region : llvm::make_range(regions.rbegin(), regions.rend())) {
-            if (region.break_dest) {
-                if (region.say_why) {
-                    pushAssign(region.why_name, makeNum(Why::BREAK));
-                    region.did_why |= (1 << Why::BREAK);
+        for (auto& cont : llvm::make_range(continuations.rbegin(), continuations.rend())) {
+            if (cont.break_dest) {
+                if (cont.say_why) {
+                    pushAssign(cont.why_name, makeNum(Why::BREAK));
+                    cont.did_why |= (1 << Why::BREAK);
                 }
 
-                AST_Jump* j = makeJump();
-                j->target = region.break_dest;
-                curblock->connectTo(region.break_dest, true);
-                push_back(j);
-                curblock = NULL;
+                pushJump(cont.break_dest, true);
                 return;
             }
         }
@@ -223,7 +287,7 @@ private:
 
         auto name = nodeName(e);
         pushAssign(name, call);
-        return makeName(name, AST_TYPE::Load, e->lineno);
+        return makeLoad(name, e);
     }
 
     AST_Name* remapName(AST_Name* name) { return name; }
@@ -256,19 +320,12 @@ private:
             InternedString iter_name = nodeName(node, "lc_iter", i);
             pushAssign(iter_name, iter_call);
 
-            AST_expr* next_attr
-                = makeLoadAttribute(makeName(iter_name, AST_TYPE::Load, node->lineno), internString("next"), true);
-
-            AST_Jump* j;
+            AST_expr* next_attr = makeLoadAttribute(makeLoad(iter_name, node), internString("next"), true);
 
             CFGBlock* test_block = cfg->addBlock();
             test_block->info = "comprehension_test";
             // printf("Test block for comp %d is %d\n", i, test_block->idx);
-
-            j = new AST_Jump();
-            j->target = test_block;
-            curblock->connectTo(test_block);
-            push_back(j);
+            pushJump(test_block);
 
             curblock = test_block;
             AST_LangPrimitive* test_call = new AST_LangPrimitive(AST_LangPrimitive::HASNEXT);
@@ -295,7 +352,7 @@ private:
             curblock = body_block;
             InternedString next_name(nodeName(next_attr));
             pushAssign(next_name, makeCall(next_attr));
-            pushAssign(c->target, makeName(next_name, AST_TYPE::Load, node->lineno));
+            pushAssign(c->target, makeLoad(next_name, node));
 
             for (AST_expr* if_condition : c->ifs) {
                 AST_expr* remapped = callNonzero(remapExpr(if_condition));
@@ -317,10 +374,7 @@ private:
                 curblock->connectTo(body_continue);
 
                 curblock = body_tramp;
-                j = new AST_Jump();
-                j->target = test_block;
-                push_back(j);
-                curblock->connectTo(test_block, true);
+                pushJump(test_block, true);
 
                 curblock = body_continue;
             }
@@ -330,21 +384,15 @@ private:
             assert((finished_block != NULL) == (i != 0));
             if (finished_block) {
                 curblock = exit_block;
-                j = new AST_Jump();
-                j->target = finished_block;
-                curblock->connectTo(finished_block, true);
-                push_back(j);
+                pushJump(finished_block, true);
             }
             finished_block = test_block;
 
             curblock = body_end;
             if (is_innermost) {
-                push_back(makeExpr(applyComprehensionCall(node, makeName(rtn_name, AST_TYPE::Load, node->lineno))));
+                push_back(makeExpr(applyComprehensionCall(node, makeLoad(rtn_name, node))));
 
-                j = new AST_Jump();
-                j->target = test_block;
-                curblock->connectTo(test_block, true);
-                push_back(j);
+                pushJump(test_block, true);
 
                 assert(exit_blocks.size());
                 curblock = exit_blocks[0];
@@ -361,7 +409,7 @@ private:
             // printf("Exit block for comp %d is %d\n", i, exit_blocks[i]->idx);
         }
 
-        return makeName(rtn_name, AST_TYPE::Load, node->lineno);
+        return makeLoad(rtn_name, node);
     }
 
     AST_expr* makeNum(int n) {
@@ -371,11 +419,15 @@ private:
         return node;
     }
 
-    AST_Jump* makeJump() {
+    void pushJump(CFGBlock* target, bool allow_backedge = false) {
         AST_Jump* rtn = new AST_Jump();
-        return rtn;
+        rtn->target = target;
+        push_back(rtn);
+        curblock->connectTo(target, allow_backedge);
+        curblock = nullptr;
     }
 
+    // NB. can generate blocks, because callNonzero can
     AST_Branch* makeBranch(AST_expr* test) {
         AST_Branch* rtn = new AST_Branch();
         rtn->test = callNonzero(test);
@@ -384,6 +436,30 @@ private:
         return rtn;
     }
 
+    // NB. this can (but usually doesn't) generate new blocks, which is why we require `iftrue' and `iffalse' to be
+    // deferred, to avoid heisenbugs. of course, this doesn't allow these branches to be backedges, but that hasn't yet
+    // been necessary.
+    void pushBranch(AST_expr* test, CFGBlock* iftrue, CFGBlock* iffalse) {
+        assert(iftrue->idx == -1 && iffalse->idx == -1);
+        AST_Branch* branch = makeBranch(test);
+        branch->iftrue = iftrue;
+        branch->iffalse = iffalse;
+        curblock->connectTo(iftrue);
+        curblock->connectTo(iffalse);
+        push_back(branch);
+        curblock = nullptr;
+    }
+
+    void pushReraise(AST* node, InternedString exc_type_name, InternedString exc_value_name,
+                     InternedString exc_traceback_name) {
+        auto raise = new AST_Raise();
+        raise->arg0 = makeLoad(exc_type_name, node);
+        raise->arg1 = makeLoad(exc_value_name, node);
+        raise->arg2 = makeLoad(exc_traceback_name, node);
+        push_back(raise);
+        curblock = nullptr;
+    }
+
     AST_expr* makeLoadAttribute(AST_expr* base, InternedString name, bool clsonly) {
         AST_expr* rtn;
         if (clsonly) {
@@ -414,28 +490,34 @@ private:
     }
 
     AST_Call* makeCall(AST_expr* func, AST_expr* arg0) {
-        AST_Call* call = new AST_Call();
+        auto call = makeCall(func);
         call->args.push_back(arg0);
-        call->starargs = NULL;
-        call->kwargs = NULL;
-        call->func = func;
-        call->col_offset = func->col_offset;
-        call->lineno = func->lineno;
         return call;
     }
 
     AST_Call* makeCall(AST_expr* func, AST_expr* arg0, AST_expr* arg1) {
-        AST_Call* call = new AST_Call();
+        auto call = makeCall(func);
         call->args.push_back(arg0);
         call->args.push_back(arg1);
-        call->starargs = NULL;
-        call->kwargs = NULL;
-        call->func = func;
-        call->col_offset = func->col_offset;
-        call->lineno = func->lineno;
         return call;
     }
 
+    AST_Call* makeCall(AST_expr* func, AST_expr* arg0, AST_expr* arg1, AST_expr* arg2) {
+        auto call = makeCall(func);
+        call->args.push_back(arg0);
+        call->args.push_back(arg1);
+        call->args.push_back(arg2);
+        return call;
+    }
+
+    AST_Compare* makeCompare(AST_TYPE::AST_TYPE oper, AST_expr* left, AST_expr* right) {
+        auto compare = new AST_Compare();
+        compare->ops.push_back(AST_TYPE::Eq);
+        compare->left = left;
+        compare->comparators.push_back(right);
+        return compare;
+    }
+
     void pushAssign(AST_expr* target, AST_expr* val) {
         AST_Assign* assign = new AST_Assign();
         assign->value = val;
@@ -497,7 +579,7 @@ private:
                 InternedString tmp_name = nodeName(target, "", i);
                 new_target->elts.push_back(makeName(tmp_name, AST_TYPE::Store, target->lineno));
 
-                pushAssign((*elts)[i], makeName(tmp_name, AST_TYPE::Load, target->lineno));
+                pushAssign((*elts)[i], makeLoad(tmp_name, target));
             }
         } else {
             RELEASE_ASSERT(0, "%d", target->type);
@@ -518,11 +600,10 @@ private:
         return stmt;
     }
 
-
-
     InternedString nodeName(AST* node) {
         char buf[40];
-        snprintf(buf, 40, "#%p", node);
+        int bytes = snprintf(buf, 40, "#%p", node);
+        assert(bytes < 40); // double-check
 // Uncomment this line to check to make sure we never reuse the same nodeName() accidentally.
 // This check is potentially too expensive for even debug mode, since it never frees any memory.
 // #define VALIDATE_FAKE_NAMES
@@ -539,13 +620,15 @@ private:
 
     InternedString nodeName(AST* node, const std::string& suffix) {
         char buf[50];
-        snprintf(buf, 50, "#%p_%s", node, suffix.c_str());
+        int bytes = snprintf(buf, 50, "#%p_%s", node, suffix.c_str());
+        assert(bytes < 50); // double-check
         return internString(std::string(buf));
     }
 
     InternedString nodeName(AST* node, const std::string& suffix, int idx) {
         char buf[50];
-        snprintf(buf, 50, "#%p_%s_%d", node, suffix.c_str(), idx);
+        int bytes = snprintf(buf, 50, "#%p_%s_%d", node, suffix.c_str(), idx);
+        assert(bytes < 50); // double-check
         return internString(std::string(buf));
     }
 
@@ -643,26 +726,19 @@ private:
             }
 
             curblock = crit_break_block;
-            AST_Jump* j = new AST_Jump();
-            j->target = exit_block;
-            push_back(j);
-            crit_break_block->connectTo(exit_block);
+            pushJump(exit_block);
 
             curblock = next_block;
         }
 
         AST_expr* final_val = remapExpr(node->values[node->values.size() - 1]);
         pushAssign(name, final_val);
-
-        AST_Jump* j = new AST_Jump();
-        push_back(j);
-        j->target = exit_block;
-        curblock->connectTo(exit_block);
+        pushJump(exit_block);
 
         cfg->placeBlock(exit_block);
         curblock = exit_block;
 
-        return makeName(name, AST_TYPE::Load, node->lineno);
+        return makeLoad(name, node);
     }
 
     AST_expr* remapCall(AST_Call* node) {
@@ -744,7 +820,7 @@ private:
                 pushAssign(name, val);
 
                 AST_Branch* br = new AST_Branch();
-                br->test = callNonzero(makeName(name, AST_TYPE::Load, node->lineno));
+                br->test = callNonzero(makeLoad(name, node));
                 push_back(br);
 
                 CFGBlock* was_block = curblock;
@@ -757,25 +833,18 @@ private:
                 br->iftrue = next_block;
 
                 curblock = crit_break_block;
-                AST_Jump* j = new AST_Jump();
-                j->target = exit_block;
-                push_back(j);
-                crit_break_block->connectTo(exit_block);
+                pushJump(exit_block);
 
                 curblock = next_block;
 
                 left = _dup(right);
             }
 
-            AST_Jump* j = new AST_Jump();
-            push_back(j);
-            j->target = exit_block;
-            curblock->connectTo(exit_block);
-
+            pushJump(exit_block);
             cfg->placeBlock(exit_block);
             curblock = exit_block;
 
-            return makeName(name, AST_TYPE::Load, node->lineno);
+            return makeLoad(name, node);
         }
     }
 
@@ -799,7 +868,7 @@ private:
     // Generates a FunctionDef which produces scope for `node'. The function produced is empty, so you'd better fill it.
     // `node' had better be a kind of node that scoping_analysis thinks can carry scope (see the switch (node->type)
     // block in ScopingAnalysis::processNameUsages in analysis/scoping_analysis.cpp); e.g. a Lambda or GeneratorExp.
-    AST_FunctionDef* makeFunctionForScope(AST* node) {
+    AST_MakeFunction* makeFunctionForScope(AST* node) {
         AST_FunctionDef* func = new AST_FunctionDef();
         func->lineno = node->lineno;
         func->col_offset = node->col_offset;
@@ -809,7 +878,7 @@ private:
         func->args->vararg = internString("");
         func->args->kwarg = internString("");
         scoping_analysis->registerScopeReplacement(node, func); // critical bit
-        return func;
+        return new AST_MakeFunction(func);
     }
 
     // This is a helper function used for generator expressions and comprehensions.
@@ -851,18 +920,18 @@ private:
         AST_expr* first = remapExpr(node->generators[0]->iter);
         InternedString first_generator_name = nodeName(node->generators[0]);
 
-        AST_FunctionDef* func = makeFunctionForScope(node);
-        func->args->args.push_back(makeName(first_generator_name, AST_TYPE::Param, node->lineno));
-        emitComprehensionLoops(&func->body, node->generators,
+        AST_MakeFunction* func = makeFunctionForScope(node);
+        func->function_def->args->args.push_back(makeName(first_generator_name, AST_TYPE::Param, node->lineno));
+        emitComprehensionLoops(&func->function_def->body, node->generators,
                                makeName(first_generator_name, AST_TYPE::Load, node->lineno),
                                [this, node](std::vector<AST_stmt*>* insert_point) {
                                    auto y = new AST_Yield();
                                    y->value = node->elt;
                                    insert_point->push_back(makeExpr(y));
                                });
-        push_back(func);
+        pushAssign(func->function_def->name, func);
 
-        return makeCall(makeName(func->name, AST_TYPE::Load, node->lineno), first);
+        return makeCall(makeLoad(func->function_def->name, node), first);
     }
 
     void emitComprehensionYield(AST_DictComp* node, InternedString dict_name, std::vector<AST_stmt*>* insert_point) {
@@ -883,70 +952,58 @@ private:
         AST_expr* first = remapExpr(node->generators[0]->iter);
         InternedString first_generator_name = nodeName(node->generators[0]);
 
-        AST_FunctionDef* func = makeFunctionForScope(node);
-        func->args->args.push_back(makeName(first_generator_name, AST_TYPE::Param, node->lineno));
+        AST_MakeFunction* func = makeFunctionForScope(node);
+        func->function_def->args->args.push_back(makeName(first_generator_name, AST_TYPE::Param, node->lineno));
 
         InternedString rtn_name = nodeName(node);
         auto asgn = new AST_Assign();
         asgn->targets.push_back(makeName(rtn_name, AST_TYPE::Store, node->lineno));
         asgn->value = new ResultType();
-        func->body.push_back(asgn);
+        func->function_def->body.push_back(asgn);
 
         auto lambda =
             [&](std::vector<AST_stmt*>* insert_point) { emitComprehensionYield(node, rtn_name, insert_point); };
         AST_Name* first_name = makeName(first_generator_name, AST_TYPE::Load, node->lineno);
-        emitComprehensionLoops(&func->body, node->generators, first_name, lambda);
+        emitComprehensionLoops(&func->function_def->body, node->generators, first_name, lambda);
 
         auto rtn = new AST_Return();
         rtn->value = makeName(rtn_name, AST_TYPE::Load, node->lineno);
-        func->body.push_back(rtn);
+        func->function_def->body.push_back(rtn);
 
-        push_back(func);
+        pushAssign(func->function_def->name, func);
 
-        return makeCall(makeName(func->name, AST_TYPE::Load, node->lineno), first);
+        return makeCall(makeName(func->function_def->name, AST_TYPE::Load, node->lineno), first);
     }
 
     AST_expr* remapIfExp(AST_IfExp* node) {
         assert(curblock);
 
         InternedString rtn_name = nodeName(node);
+        CFGBlock* iftrue = cfg->addDeferredBlock();
+        CFGBlock* iffalse = cfg->addDeferredBlock();
+        CFGBlock* exit_block = cfg->addDeferredBlock();
 
-        AST_Branch* br = new AST_Branch();
-        br->col_offset = node->col_offset;
-        br->lineno = node->lineno;
-        br->test = callNonzero(remapExpr(node->test));
-        push_back(br);
-
-        CFGBlock* starting_block = curblock;
+        pushBranch(remapExpr(node->test), iftrue, iffalse);
 
-        CFGBlock* iftrue = cfg->addBlock();
-        iftrue->info = "iftrue";
-        br->iftrue = iftrue;
-        starting_block->connectTo(iftrue);
+        // if true block
+        cfg->placeBlock(iftrue);
         curblock = iftrue;
+        iftrue->info = "iftrue";
         pushAssign(rtn_name, remapExpr(node->body));
-        AST_Jump* jtrue = new AST_Jump();
-        push_back(jtrue);
-        CFGBlock* endtrue = curblock;
+        pushJump(exit_block);
 
-        CFGBlock* iffalse = cfg->addBlock();
-        iffalse->info = "iffalse";
-        br->iffalse = iffalse;
-        starting_block->connectTo(iffalse);
+        // if false block
+        cfg->placeBlock(iffalse);
         curblock = iffalse;
+        iffalse->info = "iffalse";
         pushAssign(rtn_name, remapExpr(node->orelse));
-        AST_Jump* jfalse = new AST_Jump();
-        push_back(jfalse);
-        CFGBlock* endfalse = curblock;
-
-        CFGBlock* exit_block = cfg->addBlock();
-        jtrue->target = exit_block;
-        endtrue->connectTo(exit_block);
-        jfalse->target = exit_block;
-        endfalse->connectTo(exit_block);
+        pushJump(exit_block);
+
+        // exit block
+        cfg->placeBlock(exit_block);
         curblock = exit_block;
 
-        return makeName(rtn_name, AST_TYPE::Load, node->lineno);
+        return makeLoad(rtn_name, node);
     }
 
     AST_expr* remapIndex(AST_Index* node) {
@@ -957,14 +1014,25 @@ private:
         return rtn;
     }
 
-    AST_expr* remapLambda(AST_Lambda* node) {
-        // Remap in place: see note in visit_functiondef for why.
-
-        for (int i = 0; i < node->args->defaults.size(); i++) {
-            node->args->defaults[i] = remapExpr(node->args->defaults[i]);
-        }
+    AST_arguments* remapArguments(AST_arguments* args) {
+        auto rtn = new AST_arguments();
+        rtn = new AST_arguments();
+        // don't remap args, they're not evaluated. NB. expensive vector copy.
+        rtn->args = args->args;
+        rtn->kwarg = args->kwarg;
+        rtn->vararg = args->vararg;
+        for (auto expr : args->defaults)
+            rtn->defaults.push_back(remapExpr(expr));
+        return rtn;
+    }
 
-        return node;
+    AST_expr* remapLambda(AST_Lambda* node) {
+        auto rtn = new AST_Lambda();
+        rtn->body = node->body; // don't remap now; will be CFG'ed later
+        rtn->args = remapArguments(node->args);
+        // lambdas create scope, need to register as replacement
+        scoping_analysis->registerScopeReplacement(node, rtn);
+        return rtn;
     }
 
     AST_expr* remapLangPrimitive(AST_LangPrimitive* node) {
@@ -1068,9 +1136,14 @@ private:
 
         push_back(makeExpr(new AST_LangPrimitive(AST_LangPrimitive::UNCACHE_EXC_INFO)));
 
-        return makeName(node_name, AST_TYPE::Load, node->lineno);
+        return makeLoad(node_name, node);
     }
 
+    // Flattens a nested expression into a flat one, emitting instructions &
+    // generating temporary variables as needed.
+    //
+    // If `wrap_with_assign` is true, it will always return a temporary
+    // variable.
     AST_expr* remapExpr(AST_expr* node, bool wrap_with_assign = true) {
         if (node == NULL)
             return NULL;
@@ -1159,15 +1232,65 @@ private:
                 RELEASE_ASSERT(0, "%d", node->type);
         }
 
+        // this is the part that actually generates temporaries & assigns to them.
         if (wrap_with_assign && (rtn->type != AST_TYPE::Name || ast_cast<AST_Name>(rtn)->id.str()[0] != '#')) {
             InternedString name = nodeName(node);
             pushAssign(name, rtn);
-            return makeName(name, AST_TYPE::Load, node->lineno);
+            return makeLoad(name, node);
         } else {
             return rtn;
         }
     }
 
+    // helper for visit_{tryfinally,with}
+    CFGBlock* makeFinallyCont(Why reason, AST_expr* whyexpr, CFGBlock* then_block) {
+        CFGBlock* otherwise = cfg->addDeferredBlock();
+        otherwise->info = "finally_otherwise";
+        pushBranch(makeCompare(AST_TYPE::Eq, whyexpr, makeNum(reason)), then_block, otherwise);
+        cfg->placeBlock(otherwise);
+        return otherwise;
+    }
+
+    // Helper for visit_with. Performs the appropriate exit from a with-block, according to the value of `why'.
+    // NB. `exit_block' is only used if `why' is FALLTHROUGH.
+    void exitFinally(AST* node, Why why, CFGBlock* exit_block = nullptr) {
+        switch (why) {
+            case Why::RETURN:
+                doReturn(makeLoad(internString(RETURN_NAME), node));
+                break;
+            case Why::BREAK:
+                doBreak();
+                break;
+            case Why::CONTINUE:
+                doContinue();
+                break;
+            case Why::FALLTHROUGH:
+                assert(exit_block);
+                pushJump(exit_block);
+                break;
+            case Why::EXCEPTION:
+                assert(why != Why::EXCEPTION); // not handled here
+                break;
+        }
+        assert(curblock == nullptr);
+    }
+
+    // helper for visit_{with,tryfinally}. Generates a branch testing the value of `whyexpr' against `why', and
+    // performing the appropriate exit from the with-block if they are equal.
+    // NB. `exit_block' is only used if `why' is FALLTHROUGH.
+    void exitFinallyIf(AST* node, Why why, InternedString whyname, CFGBlock* exit_block = nullptr) {
+        CFGBlock* do_exit = cfg->addDeferredBlock();
+        do_exit->info = "with_exit_if";
+        CFGBlock* otherwise = makeFinallyCont(why, makeLoad(whyname, node), do_exit);
+
+        cfg->placeBlock(do_exit);
+        curblock = do_exit;
+        exitFinally(node, why, exit_block);
+
+        curblock = otherwise;
+    }
+
+    // ---------- public methods ----------
 public:
     void push_back(AST_stmt* node) {
         assert(node->type != AST_TYPE::Invoke);
@@ -1204,6 +1327,7 @@ public:
             if (asgn->targets[0]->type == AST_TYPE::Name) {
                 AST_Name* target = ast_cast<AST_Name>(asgn->targets[0]);
                 if (target->id.str()[0] != '#') {
+// assigning to a non-temporary
 #ifndef NDEBUG
                     if (!(asgn->value->type == AST_TYPE::Name && ast_cast<AST_Name>(asgn->value)->id.str()[0] == '#')
                         && asgn->value->type != AST_TYPE::Str && asgn->value->type != AST_TYPE::Num) {
@@ -1219,8 +1343,12 @@ public:
                     // Assigning from one temporary name to another:
                     curblock->push_back(node);
                     return;
-                } else if (asgn->value->type == AST_TYPE::Num || asgn->value->type == AST_TYPE::Str) {
+                } else if (asgn->value->type == AST_TYPE::Num || asgn->value->type == AST_TYPE::Str
+                           || (asgn->value->type == AST_TYPE::Name
+                               && ast_cast<AST_Name>(asgn->value)->id.str().compare("None") == 0)) {
                     // Assigning to a temporary name from an expression that can't throw:
+                    // NB. `None' can't throw in Python, because it's hardcoded
+                    // (seriously, try reassigning "None" in CPython).
                     curblock->push_back(node);
                     return;
                 }
@@ -1265,10 +1393,7 @@ public:
         exc_asgn->value = new AST_LangPrimitive(AST_LangPrimitive::LANDINGPAD);
         curblock->push_back(exc_asgn);
 
-        AST_Jump* j = new AST_Jump();
-        j->target = exc_info.exc_dest;
-        curblock->push_back(j);
-        curblock->connectTo(exc_info.exc_dest);
+        pushJump(exc_info.exc_dest);
 
         if (is_raise)
             curblock = NULL;
@@ -1277,42 +1402,46 @@ public:
     }
 
     bool visit_classdef(AST_ClassDef* node) override {
-        // 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]);
-        }
+        // waitaminute, who deallocates `node'?
+        auto def = new AST_ClassDef();
+        def->lineno = node->lineno;
+        def->col_offset = node->col_offset;
+        def->name = node->name;
+        def->body = node->body; // expensive vector copy
+
+        // Decorators are evaluated before bases:
+        for (auto expr : node->decorator_list)
+            def->decorator_list.push_back(remapExpr(expr));
+        for (auto expr : node->bases)
+            def->bases.push_back(remapExpr(expr));
+
+        scoping_analysis->registerScopeReplacement(node, def);
+
+        auto tmp = nodeName(node);
+        pushAssign(tmp, new AST_MakeClass(def));
+        // is this name mangling correct?
+        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno));
 
-        push_back(node);
         return true;
     }
 
     bool visit_functiondef(AST_FunctionDef* node) override {
-        // 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?
-
-        // 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]);
-        }
+        auto def = new AST_FunctionDef();
+        def->name = node->name;
+        def->body = node->body; // expensive vector copy
+        // Decorators are evaluated before the defaults, so this *must* go before remapArguments().
+        // TODO(rntz): do we have a test for this
+        for (auto expr : node->decorator_list)
+            def->decorator_list.push_back(remapExpr(expr));
+        def->args = remapArguments(node->args);
+
+        scoping_analysis->registerScopeReplacement(node, def);
+
+        auto tmp = nodeName(node);
+        pushAssign(tmp, new AST_MakeFunction(def));
+        // is this name mangling correct?
+        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno));
 
-        for (int i = 0; i < node->args->defaults.size(); i++) {
-            node->args->defaults[i] = remapExpr(node->args->defaults[i]);
-        }
-
-        push_back(node);
         return true;
     }
 
@@ -1357,7 +1486,7 @@ public:
             if (a->asname.str().size() == 0) {
                 // No asname, so load the top-level module into the name
                 // (e.g., for `import os.path`, loads the os module into `os`)
-                pushAssign(internString(getTopModule(a->name.str())), makeName(tmpname, AST_TYPE::Load, node->lineno));
+                pushAssign(internString(getTopModule(a->name.str())), makeLoad(tmpname, node));
             } else {
                 // If there is an asname, get the bottom-level module by
                 // getting the attributes and load it into asname.
@@ -1371,12 +1500,11 @@ public:
                         l = r + 1;
                         continue;
                     }
-                    pushAssign(tmpname,
-                               new AST_Attribute(makeName(tmpname, AST_TYPE::Load, node->lineno), AST_TYPE::Load,
-                                                 internString(a->name.str().substr(l, r - l))));
+                    pushAssign(tmpname, new AST_Attribute(makeLoad(tmpname, node), AST_TYPE::Load,
+                                                          internString(a->name.str().substr(l, r - l))));
                     l = r + 1;
                 } while (l < a->name.str().size());
-                pushAssign(a->asname, makeName(tmpname, AST_TYPE::Load, node->lineno));
+                pushAssign(a->asname, makeLoad(tmpname, node));
             }
         }
 
@@ -1417,7 +1545,7 @@ public:
                 AST_LangPrimitive* import_star = new AST_LangPrimitive(AST_LangPrimitive::IMPORT_STAR);
                 import_star->lineno = node->lineno;
                 import_star->col_offset = node->col_offset;
-                import_star->args.push_back(makeName(tmp_module_name, AST_TYPE::Load, node->lineno));
+                import_star->args.push_back(makeLoad(tmp_module_name, node));
 
                 AST_Expr* import_star_expr = new AST_Expr();
                 import_star_expr->value = import_star;
@@ -1429,13 +1557,12 @@ public:
                 AST_LangPrimitive* import_from = new AST_LangPrimitive(AST_LangPrimitive::IMPORT_FROM);
                 import_from->lineno = node->lineno;
                 import_from->col_offset = node->col_offset;
-                import_from->args.push_back(makeName(tmp_module_name, AST_TYPE::Load, node->lineno));
+                import_from->args.push_back(makeLoad(tmp_module_name, node));
                 import_from->args.push_back(new AST_Str(a->name.str()));
 
                 InternedString tmp_import_name = nodeName(a);
                 pushAssign(tmp_import_name, import_from);
-                pushAssign(a->asname.str().size() ? a->asname : a->name,
-                           makeName(tmp_import_name, AST_TYPE::Load, node->lineno));
+                pushAssign(a->asname.str().size() ? a->asname : a->name, makeLoad(tmp_import_name, node));
             }
         }
 
@@ -1480,17 +1607,10 @@ public:
 
         CFGBlock* unreachable = cfg->addBlock();
         unreachable->info = "unreachable";
-        curblock->connectTo(unreachable);
-
-        AST_Jump* j = new AST_Jump();
-        j->target = unreachable;
-        push_back(j);
+        pushJump(unreachable);
 
         curblock = unreachable;
-        AST_Jump* j2 = new AST_Jump();
-        j2->target = unreachable;
-        push_back(j2);
-        curblock->connectTo(unreachable, true);
+        pushJump(unreachable, true);
 
         curblock = iftrue;
 
@@ -1533,9 +1653,9 @@ public:
                 AST_Name* n = ast_cast<AST_Name>(node->target);
                 assert(n->ctx_type == AST_TYPE::Store);
                 InternedString n_name(nodeName(n));
-                pushAssign(n_name, makeName(n->id, AST_TYPE::Load, node->lineno));
+                pushAssign(n_name, makeLoad(n->id, node));
                 remapped_target = n;
-                remapped_lhs = makeName(n_name, AST_TYPE::Load, node->lineno);
+                remapped_lhs = makeLoad(n_name, node);
                 break;
             }
             case AST_TYPE::Subscript: {
@@ -1595,7 +1715,7 @@ public:
 
         InternedString node_name(nodeName(node));
         pushAssign(node_name, binop);
-        pushAssign(remapped_target, makeName(node_name, AST_TYPE::Load, node->lineno));
+        pushAssign(remapped_target, makeLoad(node_name, node));
         return true;
     }
 
@@ -1710,6 +1830,8 @@ public:
     }
 
     bool visit_return(AST_Return* node) override {
+        // returns are allowed in functions (of course), and also in eval("...") strings - basically, eval strings get
+        // an implicit `return'. root_type is AST_TYPE::Expression when we're compiling an eval string.
         assert(curblock);
 
         if (root_type != AST_TYPE::FunctionDef && root_type != AST_TYPE::Lambda && root_type != AST_TYPE::Expression) {
@@ -1719,10 +1841,7 @@ public:
         if (!curblock)
             return true;
 
-        AST_expr* value = remapExpr(node->value);
-        if (value == NULL)
-            value = makeName(internString("None"), AST_TYPE::Load, node->lineno);
-        doReturn(value);
+        doReturn(node->value ? remapExpr(node->value) : makeLoad(internString("None"), node));
         return true;
     }
 
@@ -1750,10 +1869,7 @@ public:
                 break;
         }
         if (curblock) {
-            AST_Jump* jtrue = new AST_Jump();
-            push_back(jtrue);
-            jtrue->target = exit;
-            curblock->connectTo(exit);
+            pushJump(exit);
         }
 
         CFGBlock* iffalse = cfg->addBlock();
@@ -1768,10 +1884,7 @@ public:
                 break;
         }
         if (curblock) {
-            AST_Jump* jfalse = new AST_Jump();
-            push_back(jfalse);
-            jfalse->target = exit;
-            curblock->connectTo(exit);
+            pushJump(exit);
         }
 
         if (exit->predecessors.size() == 0) {
@@ -1807,11 +1920,7 @@ public:
 
         CFGBlock* test_block = cfg->addBlock();
         test_block->info = "while_test";
-
-        AST_Jump* j = makeJump();
-        push_back(j);
-        j->target = test_block;
-        curblock->connectTo(test_block);
+        pushJump(test_block);
 
         curblock = test_block;
         AST_Branch* br = makeBranch(remapExpr(node->test));
@@ -1822,7 +1931,7 @@ public:
         // but we don't want it to be placed until after the orelse.
         CFGBlock* end = cfg->addDeferredBlock();
         end->info = "while_exit";
-        pushLoopRegion(test_block, end);
+        pushLoopContinuation(test_block, end);
 
         CFGBlock* body = cfg->addBlock();
         body->info = "while_body_start";
@@ -1835,13 +1944,9 @@ public:
             if (!curblock)
                 break;
         }
-        if (curblock) {
-            AST_Jump* jbody = makeJump();
-            push_back(jbody);
-            jbody->target = test_block;
-            curblock->connectTo(test_block, true);
-        }
-        popRegion();
+        if (curblock)
+            pushJump(test_block, true);
+        popContinuation();
 
         CFGBlock* orelse = cfg->addBlock();
         orelse->info = "while_orelse_start";
@@ -1853,12 +1958,8 @@ public:
             if (!curblock)
                 break;
         }
-        if (curblock) {
-            AST_Jump* jend = makeJump();
-            push_back(jend);
-            jend->target = end;
-            curblock->connectTo(end);
-        }
+        if (curblock)
+            pushJump(end);
 
         if (end->predecessors.size() == 0) {
             delete end;
@@ -1887,14 +1988,10 @@ public:
         InternedString itername = internString(itername_buf);
         pushAssign(itername, iter_call);
 
-        AST_expr* next_attr
-            = makeLoadAttribute(makeName(itername, AST_TYPE::Load, node->lineno), internString("next"), true);
+        AST_expr* next_attr = makeLoadAttribute(makeLoad(itername, node), internString("next"), true);
 
         CFGBlock* test_block = cfg->addBlock();
-        AST_Jump* jump_to_test = makeJump();
-        jump_to_test->target = test_block;
-        push_back(jump_to_test);
-        curblock->connectTo(test_block);
+        pushJump(test_block);
         curblock = test_block;
 
         AST_LangPrimitive* test_call = new AST_LangPrimitive(AST_LangPrimitive::HASNEXT);
@@ -1914,32 +2011,25 @@ public:
         CFGBlock* else_block = cfg->addDeferredBlock();
 
         curblock = test_true;
-
         // TODO simplify the breaking of these crit edges?
-        AST_Jump* test_true_jump = makeJump();
-        test_true_jump->target = loop_block;
-        push_back(test_true_jump);
-        test_true->connectTo(loop_block);
+        pushJump(loop_block);
 
         curblock = test_false;
-        AST_Jump* test_false_jump = makeJump();
-        test_false_jump->target = else_block;
-        push_back(test_false_jump);
-        test_false->connectTo(else_block);
+        pushJump(else_block);
 
-        pushLoopRegion(test_block, end_block);
+        pushLoopContinuation(test_block, end_block);
 
         curblock = loop_block;
         InternedString next_name(nodeName(next_attr));
         pushAssign(next_name, makeCall(next_attr));
-        pushAssign(node->target, makeName(next_name, AST_TYPE::Load, node->lineno));
+        pushAssign(node->target, makeLoad(next_name, node));
 
         for (int i = 0; i < node->body.size(); i++) {
             node->body[i]->accept(this);
             if (!curblock)
                 break;
         }
-        popRegion();
+        popContinuation();
 
         if (curblock) {
             AST_LangPrimitive* end_call = new AST_LangPrimitive(AST_LangPrimitive::HASNEXT);
@@ -1955,16 +2045,10 @@ public:
             curblock->connectTo(end_false);
 
             curblock = end_true;
-            AST_Jump* end_true_jump = makeJump();
-            end_true_jump->target = loop_block;
-            push_back(end_true_jump);
-            end_true->connectTo(loop_block, true);
+            pushJump(loop_block, true);
 
             curblock = end_false;
-            AST_Jump* end_false_jump = makeJump();
-            end_false_jump->target = else_block;
-            push_back(end_false_jump);
-            end_false->connectTo(else_block);
+            pushJump(else_block);
         }
 
         cfg->placeBlock(else_block);
@@ -1975,12 +2059,8 @@ public:
             if (!curblock)
                 break;
         }
-        if (curblock) {
-            AST_Jump* else_jump = makeJump();
-            push_back(else_jump);
-            else_jump->target = end_block;
-            curblock->connectTo(end_block);
-        }
+        if (curblock)
+            pushJump(end_block);
 
         if (end_block->predecessors.size() == 0) {
             delete end_block;
@@ -2058,12 +2138,8 @@ public:
         }
 
         CFGBlock* join_block = cfg->addDeferredBlock();
-        if (curblock) {
-            AST_Jump* j = new AST_Jump();
-            j->target = join_block;
-            push_back(j);
-            curblock->connectTo(join_block);
-        }
+        if (curblock)
+            pushJump(join_block);
 
         if (exc_handler_block->predecessors.size() == 0) {
             delete exc_handler_block;
@@ -2072,7 +2148,7 @@ public:
             curblock = exc_handler_block;
 
             // TODO This is supposed to be exc_type_name (value doesn't matter for checking matches)
-            AST_expr* exc_obj = makeName(exc_value_name, AST_TYPE::Load, node->lineno);
+            AST_expr* exc_obj = makeLoad(exc_value_name, node);
 
             bool caught_all = false;
             for (AST_ExceptHandler* exc_handler : node->handlers) {
@@ -2103,9 +2179,9 @@ public:
                 }
 
                 AST_LangPrimitive* set_exc_info = new AST_LangPrimitive(AST_LangPrimitive::SET_EXC_INFO);
-                set_exc_info->args.push_back(makeName(exc_type_name, AST_TYPE::Load, node->lineno));
-                set_exc_info->args.push_back(makeName(exc_value_name, AST_TYPE::Load, node->lineno));
-                set_exc_info->args.push_back(makeName(exc_traceback_name, AST_TYPE::Load, node->lineno));
+                set_exc_info->args.push_back(makeLoad(exc_type_name, node));
+                set_exc_info->args.push_back(makeLoad(exc_value_name, node));
+                set_exc_info->args.push_back(makeLoad(exc_traceback_name, node));
                 push_back(makeExpr(set_exc_info));
 
                 if (exc_handler->name) {
@@ -2119,10 +2195,7 @@ public:
                 }
 
                 if (curblock) {
-                    AST_Jump* j = new AST_Jump();
-                    j->target = join_block;
-                    push_back(j);
-                    curblock->connectTo(join_block);
+                    pushJump(join_block);
                 }
 
                 if (exc_next) {
@@ -2135,9 +2208,9 @@ public:
 
             if (!caught_all) {
                 AST_Raise* raise = new AST_Raise();
-                raise->arg0 = makeName(exc_type_name, AST_TYPE::Load, node->lineno);
-                raise->arg1 = makeName(exc_value_name, AST_TYPE::Load, node->lineno);
-                raise->arg2 = makeName(exc_traceback_name, AST_TYPE::Load, node->lineno);
+                raise->arg0 = makeLoad(exc_type_name, node);
+                raise->arg1 = makeLoad(exc_value_name, node);
+                raise->arg2 = makeLoad(exc_traceback_name, node);
                 push_back(raise);
                 curblock = NULL;
             }
@@ -2165,7 +2238,7 @@ public:
         exc_handlers.push_back({ exc_handler_block, exc_type_name, exc_value_name, exc_traceback_name });
 
         CFGBlock* finally_block = cfg->addDeferredBlock();
-        pushFinallyRegion(finally_block, exc_why_name);
+        pushFinallyContinuation(finally_block, exc_why_name);
 
         for (AST_stmt* subnode : node->body) {
             subnode->accept(this);
@@ -2175,17 +2248,14 @@ public:
 
         exc_handlers.pop_back();
 
-        int did_why = regions.back().did_why; // bad to just reach in like this
-        popRegion();                          // finally region
+        int did_why = continuations.back().did_why; // bad to just reach in like this
+        popContinuation();                          // finally continuation
 
         if (curblock) {
             // assign the exc_*_name variables to tell irgen that they won't be undefined?
             // have an :UNDEF() langprimitive to not have to do any loading there?
             pushAssign(exc_why_name, makeNum(Why::FALLTHROUGH));
-            AST_Jump* j = new AST_Jump();
-            j->target = finally_block;
-            push_back(j);
-            curblock->connectTo(finally_block);
+            pushJump(finally_block);
         }
 
         if (exc_handler_block->predecessors.size() == 0) {
@@ -2193,13 +2263,8 @@ public:
         } else {
             cfg->placeBlock(exc_handler_block);
             curblock = exc_handler_block;
-
             pushAssign(exc_why_name, makeNum(Why::EXCEPTION));
-
-            AST_Jump* j = new AST_Jump();
-            j->target = finally_block;
-            push_back(j);
-            curblock->connectTo(finally_block);
+            pushJump(finally_block);
         }
 
         cfg->placeBlock(finally_block);
@@ -2212,99 +2277,19 @@ public:
         }
 
         if (curblock) {
-            // TODO: these 4 cases are pretty copy-pasted from each other:
-            if (did_why & (1 << Why::RETURN)) {
-                CFGBlock* doreturn = cfg->addBlock();
-                CFGBlock* otherwise = cfg->addBlock();
-
-                AST_Compare* compare = new AST_Compare();
-                compare->ops.push_back(AST_TYPE::Eq);
-                compare->left = makeName(exc_why_name, AST_TYPE::Load, node->lineno);
-                compare->comparators.push_back(makeNum(Why::RETURN));
-
-                AST_Branch* br = new AST_Branch();
-                br->test = callNonzero(compare);
-                br->iftrue = doreturn;
-                br->iffalse = otherwise;
-                curblock->connectTo(doreturn);
-                curblock->connectTo(otherwise);
-                push_back(br);
-
-                curblock = doreturn;
-                doReturn(makeName(internString(RETURN_NAME), AST_TYPE::Load, node->lineno));
-
-                curblock = otherwise;
-            }
-
-            if (did_why & (1 << Why::BREAK)) {
-                CFGBlock* doreturn = cfg->addBlock();
-                CFGBlock* otherwise = cfg->addBlock();
-
-                AST_Compare* compare = new AST_Compare();
-                compare->ops.push_back(AST_TYPE::Eq);
-                compare->left = makeName(exc_why_name, AST_TYPE::Load, node->lineno);
-                compare->comparators.push_back(makeNum(Why::BREAK));
-
-                AST_Branch* br = new AST_Branch();
-                br->test = callNonzero(compare);
-                br->iftrue = doreturn;
-                br->iffalse = otherwise;
-                curblock->connectTo(doreturn);
-                curblock->connectTo(otherwise);
-                push_back(br);
-
-                curblock = doreturn;
-                doBreak();
-
-                curblock = otherwise;
-            }
+            if (did_why & (1 << Why::RETURN))
+                exitFinallyIf(node, Why::RETURN, exc_why_name);
+            if (did_why & (1 << Why::BREAK))
+                exitFinallyIf(node, Why::BREAK, exc_why_name);
+            if (did_why & (1 << Why::CONTINUE))
+                exitFinallyIf(node, Why::CONTINUE, exc_why_name);
 
-            if (did_why & (1 << Why::CONTINUE)) {
-                CFGBlock* doreturn = cfg->addBlock();
-                CFGBlock* otherwise = cfg->addBlock();
-
-                AST_Compare* compare = new AST_Compare();
-                compare->ops.push_back(AST_TYPE::Eq);
-                compare->left = makeName(exc_why_name, AST_TYPE::Load, node->lineno);
-                compare->comparators.push_back(makeNum(Why::CONTINUE));
-
-                AST_Branch* br = new AST_Branch();
-                br->test = callNonzero(compare);
-                br->iftrue = doreturn;
-                br->iffalse = otherwise;
-                curblock->connectTo(doreturn);
-                curblock->connectTo(otherwise);
-                push_back(br);
-
-                curblock = doreturn;
-                doContinue();
-
-                curblock = otherwise;
-            }
-
-            AST_Compare* compare = new AST_Compare();
-            compare->ops.push_back(AST_TYPE::Eq);
-            compare->left = makeName(exc_why_name, AST_TYPE::Load, node->lineno);
-            compare->comparators.push_back(makeNum(Why::EXCEPTION));
-
-            AST_Branch* br = new AST_Branch();
-            br->test = callNonzero(compare);
-
-            CFGBlock* reraise = cfg->addBlock();
-            CFGBlock* noexc = cfg->addBlock();
-
-            br->iftrue = reraise;
-            br->iffalse = noexc;
-            curblock->connectTo(reraise);
-            curblock->connectTo(noexc);
-            push_back(br);
+            CFGBlock* reraise = cfg->addDeferredBlock();
+            CFGBlock* noexc = makeFinallyCont(Why::EXCEPTION, makeLoad(exc_why_name, node), reraise);
 
+            cfg->placeBlock(reraise);
             curblock = reraise;
-            AST_Raise* raise = new AST_Raise();
-            raise->arg0 = makeName(exc_type_name, AST_TYPE::Load, node->lineno);
-            raise->arg1 = makeName(exc_value_name, AST_TYPE::Load, node->lineno);
-            raise->arg2 = makeName(exc_traceback_name, AST_TYPE::Load, node->lineno);
-            push_back(raise);
+            pushReraise(node, exc_type_name, exc_value_name, exc_traceback_name);
 
             curblock = noexc;
         }
@@ -2313,44 +2298,65 @@ public:
     }
 
     bool visit_with(AST_With* node) override {
+        // see https://www.python.org/dev/peps/pep-0343/
+        // section "Specification: the 'with' Statement"
+        // which contains pseudocode for what this implements:
+        //
+        // mgr = (EXPR)
+        // exit = type(mgr).__exit__            # not calling it yet
+        // value = type(mgr).__enter__(mgr)
+        // exc = True
+        // try:
+        //     VAR = value
+        //     BLOCK
+        // except:
+        //     exc = False
+        //     if not exit(mgr, *sys.exc_info()):
+        //         raise
+        // finally:
+        //     if exc:
+        //         exit(mgr, None, None, None)
+        //
+        // Unfortunately, this pseudocode isn't *quite* correct. We don't actually call type(mgr).__exit__ and
+        // type(mgr).__enter__; rather, we use Python's "special method lookup rules" to find the appropriate method.
+        // See https://docs.python.org/2/reference/datamodel.html#new-style-special-lookup. This is one reason we can't
+        // just translate this into AST_Try{Except,Finally} nodes and recursively visit those. (If there are other
+        // reasons, I've forgotten them.)
         assert(curblock);
-
-        char ctxmgrname_buf[80];
-        snprintf(ctxmgrname_buf, 80, "#ctxmgr_%p", node);
-        InternedString ctxmgrname = internString(ctxmgrname_buf);
-        char exitname_buf[80];
-        snprintf(exitname_buf, 80, "#exit_%p", node);
-        InternedString exitname = internString(exitname_buf);
+        InternedString ctxmgrname = nodeName(node, "ctxmgr");
+        InternedString exitname = nodeName(node, "exit");
+        InternedString whyname = nodeName(node, "why");
+        InternedString exc_type_name = nodeName(node, "exc_type");
+        InternedString exc_value_name = nodeName(node, "exc_value");
+        InternedString exc_traceback_name = nodeName(node, "exc_traceback");
         InternedString nonename = internString("None");
+        CFGBlock* exit_block = cfg->addDeferredBlock();
+        exit_block->info = "with_exit";
 
         pushAssign(ctxmgrname, remapExpr(node->context_expr));
 
-        AST_expr* enter
-            = makeLoadAttribute(makeName(ctxmgrname, AST_TYPE::Load, node->lineno), internString("__enter__"), true);
-        AST_expr* exit
-            = makeLoadAttribute(makeName(ctxmgrname, AST_TYPE::Load, node->lineno), internString("__exit__"), true);
+        // TODO(rntz): for some reason, in the interpreter (but not the JIT), this is looking up __exit__ on the
+        // instance rather than the class. See test/tests/with_ctxclass_instance_attrs.py.
+        AST_expr* exit = makeLoadAttribute(makeLoad(ctxmgrname, node), internString("__exit__"), true);
         pushAssign(exitname, exit);
-        enter = remapExpr(makeCall(enter));
 
-        if (node->optional_vars) {
+        // Oddly, this acces to __enter__ doesn't suffer from the same bug. Perhaps it has something to do with
+        // __enter__ being called immediately?
+        AST_expr* enter = makeLoadAttribute(makeLoad(ctxmgrname, node), internString("__enter__"), true);
+        enter = remapExpr(makeCall(enter));
+        if (node->optional_vars)
             pushAssign(node->optional_vars, enter);
-        } else {
+        else
             push_back(makeExpr(enter));
-        }
 
-        CFGBlock* continue_dest = NULL, * break_dest = NULL;
-        if (regions.size()) {
-            continue_dest = cfg->addDeferredBlock();
-            continue_dest->info = "with_continue";
-            break_dest = cfg->addDeferredBlock();
-            break_dest->info = "with_break";
-
-            pushLoopRegion(continue_dest, break_dest);
-        }
+        // push continuations
+        CFGBlock* finally_block = cfg->addDeferredBlock();
+        finally_block->info = "with_finally";
+        pushFinallyContinuation(finally_block, whyname);
 
-        CFGBlock* return_dest = cfg->addDeferredBlock();
-        return_dest->info = "with_return";
-        pushReturnRegion(return_dest);
+        CFGBlock* exc_block = cfg->addDeferredBlock();
+        exc_block->info = "with_exc";
+        exc_handlers.push_back({ exc_block, exc_type_name, exc_value_name, exc_traceback_name });
 
         for (int i = 0; i < node->body.size(); i++) {
             node->body[i]->accept(this);
@@ -2358,64 +2364,74 @@ public:
                 break;
         }
 
-        popRegion(); // for the retrun
-
-        AST_Call* exit_call = makeCall(makeName(exitname, AST_TYPE::Load, node->lineno));
-        exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-        exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-        exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-        push_back(makeExpr(exit_call));
-
-        CFGBlock* orig_ending_block = curblock;
-
-        if (continue_dest) {
-            popRegion(); // for the loop region
-            if (continue_dest->predecessors.size() == 0) {
-                delete continue_dest;
-            } else {
-                curblock = continue_dest;
-
-                AST_Call* exit_call = makeCall(makeName(exitname, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                push_back(makeExpr(exit_call));
-
-                cfg->placeBlock(continue_dest);
-                doContinue();
-            }
-
-            if (break_dest->predecessors.size() == 0) {
-                delete break_dest;
-            } else {
-                curblock = break_dest;
-
-                AST_Call* exit_call = makeCall(makeName(exitname, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-                push_back(makeExpr(exit_call));
+        exc_handlers.pop_back();
+        int finally_did_why = continuations.back().did_why;
+        popContinuation();
 
-                cfg->placeBlock(break_dest);
-                doBreak();
-            }
-            curblock = orig_ending_block;
+        if (curblock) {
+            // The try-suite finished as normal; jump to the finally block.
+            pushAssign(whyname, makeNum(Why::FALLTHROUGH));
+            pushJump(finally_block);
         }
 
-        if (return_dest->predecessors.size() == 0) {
-            delete return_dest;
+        // The exception-handling block
+        if (exc_block->predecessors.size() == 0) {
+            // TODO(rntz): test for this case
+            delete exc_block;
         } else {
-            cfg->placeBlock(return_dest);
-            curblock = return_dest;
-
-            AST_Call* exit_call = makeCall(makeName(exitname, AST_TYPE::Load, node->lineno));
-            exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-            exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-            exit_call->args.push_back(makeName(nonename, AST_TYPE::Load, node->lineno));
-            push_back(makeExpr(exit_call));
-
-            doReturn(makeName(internString(RETURN_NAME), AST_TYPE::Load, node->lineno));
-            curblock = orig_ending_block;
+            cfg->placeBlock(exc_block);
+            curblock = exc_block;
+
+            // call the context-manager's exit method
+            InternedString suppressname = nodeName(node, "suppress");
+            pushAssign(suppressname, makeCall(makeLoad(exitname, node), makeLoad(exc_type_name, node),
+                                              makeLoad(exc_value_name, node), makeLoad(exc_traceback_name, node)));
+
+            // if it returns true, suppress the error and go to our exit block
+            CFGBlock* reraise_block = cfg->addDeferredBlock();
+            reraise_block->info = "with_reraise";
+            // break potential critical edge
+            CFGBlock* exiter = cfg->addDeferredBlock();
+            exiter->info = "with_exiter";
+            pushBranch(makeLoad(suppressname, node), exiter, reraise_block);
+
+            cfg->placeBlock(exiter);
+            curblock = exiter;
+            pushJump(exit_block);
+
+            // otherwise, reraise the exception
+            cfg->placeBlock(reraise_block);
+            curblock = reraise_block;
+            pushReraise(node, exc_type_name, exc_value_name, exc_traceback_name);
+        }
+
+        // The finally block
+        if (finally_block->predecessors.size() == 0) {
+            // TODO(rntz): test for this case, "with foo: raise bar"
+            delete finally_block;
+        } else {
+            cfg->placeBlock(finally_block);
+            curblock = finally_block;
+            // call the context-manager's exit method, ignoring result
+            push_back(makeExpr(makeCall(makeLoad(exitname, node), makeLoad(nonename, node), makeLoad(nonename, node),
+                                        makeLoad(nonename, node))));
+
+            if (finally_did_why & (1 << Why::CONTINUE))
+                exitFinallyIf(node, Why::CONTINUE, whyname);
+            if (finally_did_why & (1 << Why::BREAK))
+                exitFinallyIf(node, Why::BREAK, whyname);
+            if (finally_did_why & (1 << Why::RETURN))
+                exitFinallyIf(node, Why::RETURN, whyname);
+            exitFinally(node, Why::FALLTHROUGH, exit_block);
+        }
+
+        if (exit_block->predecessors.size() == 0) {
+            // FIXME(rntz): does this ever happen?
+            // make a test for it!
+            delete exit_block;
+        } else {
+            cfg->placeBlock(exit_block);
+            curblock = exit_block;
         }
 
         return true;
@@ -2425,30 +2441,8 @@ public:
 void CFG::print() {
     printf("CFG:\n");
     printf("%ld blocks\n", blocks.size());
-    PrintVisitor* pv = new PrintVisitor(4);
-    for (int i = 0; i < blocks.size(); i++) {
-        CFGBlock* b = blocks[i];
-        printf("Block %d", b->idx);
-        if (b->info)
-            printf(" '%s'", b->info);
-
-        printf("; Predecessors:");
-        for (int j = 0; j < b->predecessors.size(); j++) {
-            printf(" %d", b->predecessors[j]->idx);
-        }
-        printf(" Successors:");
-        for (int j = 0; j < b->successors.size(); j++) {
-            printf(" %d", b->successors[j]->idx);
-        }
-        printf("\n");
-
-        for (int j = 0; j < b->body.size(); j++) {
-            printf("    ");
-            b->body[j]->accept(pv);
-            printf("\n");
-        }
-    }
-    delete pv;
+    for (int i = 0; i < blocks.size(); i++)
+        blocks[i]->print();
 }
 
 CFG* computeCFG(SourceInfo* source, std::vector<AST_stmt*> body) {

src/core/cfg.h

@@ -56,6 +56,7 @@ public:
     void unconnectFrom(CFGBlock* successor);
 
     void push_back(AST_stmt* node) { body.push_back(node); }
+    void print();
 };
 
 // Control Flow Graph
@@ -79,6 +80,9 @@ public:
         return block;
     }
 
+    // Creates a block which must be placed later, using placeBlock().
+    // Must be placed on same CFG it was created on.
+    // You can also safely delete it without placing it.
     CFGBlock* addDeferredBlock() {
         CFGBlock* block = new CFGBlock(this, -1);
         return block;

src/core/common.h

@@ -39,6 +39,8 @@
 #define _CAT(A, B) A##B
 #define CAT(A, B) _CAT(A, B)
 
+#define ARRAY_LEN(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 // GCC and clang handle always_inline very differently;
 // we mostly only care about it for the stdlib, so just remove the attributes
 // if we're not in clang

src/core/util.h

@@ -55,7 +55,9 @@ bool endswith(const std::string& s, const std::string& pattern);
 
 void removeDirectoryIfExists(const std::string& path);
 
-template <class T1, class T2> void compareKeyset(T1* lhs, T2* rhs) {
+// Checks that lhs and rhs, which are iterables of InternedStrings, have the
+// same set of names in them.
+template <class T1, class T2> bool sameKeyset(T1* lhs, T2* rhs) {
     std::vector<InternedString> lv, rv;
     for (typename T1::iterator it = lhs->begin(); it != lhs->end(); it++) {
         lv.push_back(it->first);
@@ -92,7 +94,7 @@ template <class T1, class T2> void compareKeyset(T1* lhs, T2* rhs) {
         }
         good = false;
     }
-    assert(good);
+    return good;
 }
 }
 

src/runtime/file.cpp

@@ -800,10 +800,6 @@ Box* fileEnter(BoxedFile* self) {
 Box* fileExit(BoxedFile* self, Box* exc_type, Box* exc_val, Box** args) {
     Box* exc_tb = args[0];
     assert(self->cls == file_cls);
-    assert(exc_type == None);
-    assert(exc_val == None);
-    assert(exc_tb == None);
-
     fileClose(self);
     return None;
 }

src/runtime/types.cpp

@@ -390,6 +390,9 @@ std::string BoxedModule::name() {
     }
 }
 
+// This mustn't throw; our IR generator generates calls to it without "invoke" even when there are exception handlers /
+// finally-blocks in scope.
+// TODO: should we use C++11 `noexcept' here?
 extern "C" Box* boxCLFunction(CLFunction* f, BoxedClosure* closure, bool isGenerator,
                               std::initializer_list<Box*> defaults) {
     if (closure)

test/tests/exceptions_test.py

-# expected: fail
-# - with statements
-
 class TestException(Exception):
     pass
 
@@ -361,7 +358,11 @@ def f12():
         except Exception as l[0]:
             print "shouldnt get here"
     except Exception as e2:
-        print e2
+        # print it to stderr, so that our tester's error message substituter can
+        # deal with differences between error messages between different Python
+        # versions.
+        import traceback
+        traceback.print_exc()
 
     l = []
     try:

test/tests/file_closed.py

+# expected: fail
+import sys
+print sys.stdout.closed

test/tests/finally_except.py

+# these may seem pointless, but they exercise a family of corner cases in our
+# CFG generation pass (cfg.cpp).
+def returner():
+    try:
+        try:
+            print '2 + 2'
+            return
+        finally:
+            print 'finally'
+    except:
+        print 'exception'
+
+returner()
+
+def continuer():
+    for x in [1]:
+        try:
+            try:
+                print '2 + 2'
+                continue
+            finally:
+                print 'finally'
+        except:
+            print 'exception'
+
+continuer()
+
+def breaker():
+    for x in [1]:
+        try:
+            try:
+                print '2 + 2'
+                break
+            finally:
+                print 'finally'
+        except:
+            print 'exception'
+
+breaker()
+
+def raiser():
+    try:
+        try:
+            print '2 + 2'
+            raise Exception('blaaargh')
+        finally:
+            print 'finally'
+    except:
+        print 'exception'
+
+raiser()
+
+def alltogethernow():
+    for x in [1,2,3,4]:
+        try:
+            try:
+                print '2 + 2'
+                if x == 1: break
+                if x == 2: continue
+                if x == 3: raise Exception('blaargh')
+                if x == 4: return
+            finally:
+                print 'finally'
+        except:
+            print 'exception'
+
+alltogethernow()

test/tests/try_class.py

+# we have one test at global scope, another at local.
+# they behave differently in codegen; there have been points at which either was bugged when the other was not.
+try:
+    class C(object):
+        print 'here'
+finally:
+    print 'finally'
+
+def f():
+    try:
+        class C(object):
+            print 'here'
+    finally:
+        print 'finally'
+f()
+
+try:
+    class D(object):
+        print 'here'
+except:
+    print 'impossible'
+    raise
+
+def f2():
+    try:
+        class D(object):
+            print 'here'
+    except:
+        print 'impossible'
+        print D
+        raise
+f2()

test/tests/try_continue.py

+# expected: fail
+# Syntax error to have a continue outside a loop.
+def foo():
+    try: continue
+    finally: pass

test/tests/try_def.py

+def f():
+    try:
+        def foo(): return 0
+    except:
+        print 'except'
+    finally:
+        print 'finally'
+f()
+
+def f2():
+    try:
+        def foo(): return 0
+    except:
+        print 'except'
+f2()
+
+def f3():
+    try:
+        def foo(): return 0
+    finally:
+        print 'finally'
+f3()

test/tests/try_return_finally.py

+def f():
+    try:
+        # Looks like this returns from the function, but it needs to go to the finally block
+        return
+    finally:
+        pass
+f()

test/tests/with_class.py

+def f():
+    # originally this exposed a bug in our irgen phase, so even `with None`
+    # failed here; the bug happened before actual execution. Just to test more
+    # things, though, we use an actual contextmanager here.
+    with open('/dev/null'):
+        class C(object):
+            print 'hello'
+
+f()

test/tests/with_class_redefine.py

+def f():
+    C = 23
+    try:
+        class C(object): pass
+    except:
+        print 'except: C = %s' % C
+        raise
+    finally:
+        print 'finally: C = %s' % C
+    print 'end: C = %s' % C
+
+f()

test/tests/with_continue.py

+# should_error
+class Mgr(object):
+    def __enter__(self):
+        print 'entered!'
+    def __exit__(self, *args):
+        print 'exited!'
+
+with Mgr() as m:
+    continue

test/tests/with_ctxclass_instance_attrs.py

+class Mgr(object):
+    def __init__(self): self.__enter__ = 'sucks to be you'
+    def __enter__(self): pass
+    def __exit__(self, typ, val, tback): pass
+
+with Mgr() as m:
+    print 'boom boom boom boom'
+
+class Mgr2(object):
+    def __init__(self): self.__exit__ = 'screwed!'
+    def __enter__(self): pass
+    def __exit__(self, typ, val, tback): pass
+
+with Mgr2() as m:
+    print 'bang bang bang bang'

test/tests/with_ctxclass_order_of_access.py

+class Mgr(object):
+    def __init__(self): print 'Mgr.__init__()'
+
+    @property
+    def __enter__(self):
+        print 'Mgr.__enter__ accessed'
+        def enterer(*args):
+            print 'Mgr.__enter__ called'
+            return 23
+        return enterer
+
+    @property
+    def __exit__(self):
+        print 'Mgr.__exit__ accessed'
+        def exiter(*args):
+            print 'Mgr.__exit__ called'
+            return False
+        return exiter
+
+with Mgr() as m:
+    print 'hello I am a with block'
+    print 'm: %s' % m
+
+try:
+    with Mgr() as m:
+        1/0
+    print "you can't get there from here"
+except ZeroDivisionError, e:
+    print e

test/tests/with_file.py

+f = open('/dev/null')
+try:
+    with f:
+        1/0
+except ZeroDivisionError as e:
+    print e
+
+try:
+    f.readline()
+except ValueError as e:
+    print e

test/tests/with_functiondef.py

+def f():
+    # this exposes a bug in our irgen phase, so even `with None` bugs out here;
+    # the bug happens before actual execution. Just to test more things, though,
+    # we use an actual contextmanager here.
+    with open('/dev/null'):
+        def foo():
+            pass
+
+f()

tools/tester.py

@@ -115,6 +115,7 @@ def canonicalize_stderr(stderr):
             ("NameError: global name '", "NameError: name '"),
             ("AttributeError: '(\w+)' object attribute '(\w+)' is read-only", "AttributeError: \\2"),
             (r"TypeError: object.__new__\(\) takes no parameters", "TypeError: object() takes no parameters"),
+            ("IndexError: list assignment index out of range", "IndexError: list index out of range"),
             ]
 
     for pattern, subst_with in substitutions: