Fix local variable declarations in the REPL.

A statement like:

  for (i in 1..2)

When run in the REPL declares a local variable ("i"), but not inside
a function or method body. This hit a corner case in the compiler
where it didn't have the correct slot indexes set up.

That corner case is because sometimes when you compile a chunk, local
slot zero is pre-allocated -- either to refer to "this" or to hold the
closure for a function so that it doesn't get GCed while running. But
if you're compiling top-level code, that slot isn't allocated. But top
level code for the REPL *should* be, because that gets invoked like a
function.

To simplify things, *every* compiled chunk now pre-allocates slot zero.
That way, there are fewer cases to keep in mind.

Also fixed an issue where a GC during an import could collected the
imported module body's closure.

Fix #456.

src/vm/wren_compiler.c

@@ -498,7 +498,7 @@ static int addConstant(Compiler* compiler, Value constant)
 
 // Initializes [compiler].
 static void initCompiler(Compiler* compiler, Parser* parser, Compiler* parent,
-                         bool isFunction)
+                         bool isMethod)
 {
   compiler->parser = parser;
   compiler->parent = parent;
@@ -512,41 +512,41 @@ static void initCompiler(Compiler* compiler, Parser* parser, Compiler* parent,
 
   parser->vm->compiler = compiler;
 
+  // Declare a local slot for either the closure or method receiver so that we
+  // don't try to reuse that slot for a user-defined local variable. For
+  // methods, we name it "this", so that we can resolve references to that like
+  // a normal variable. For functions, they have no explicit "this", so we use
+  // an empty name. That way references to "this" inside a function walks up
+  // the parent chain to find a method enclosing the function whose "this" we
+  // can close over.
+  compiler->numLocals = 1;
+  compiler->numSlots = compiler->numLocals;
+
+  if (isMethod)
+  {
+    compiler->locals[0].name = "this";
+    compiler->locals[0].length = 4;
+  }
+  else
+  {
+    compiler->locals[0].name = NULL;
+    compiler->locals[0].length = 0;
+  }
+  
+  compiler->locals[0].depth = -1;
+  compiler->locals[0].isUpvalue = false;
+
   if (parent == NULL)
   {
-    compiler->numLocals = 0;
-
     // Compiling top-level code, so the initial scope is module-level.
     compiler->scopeDepth = -1;
   }
   else
   {
-    // Declare a fake local variable for the receiver so that it's slot in the
-    // stack is taken. For methods, we call this "this", so that we can resolve
-    // references to that like a normal variable. For functions, they have no
-    // explicit "this". So we pick a bogus name. That way references to "this"
-    // inside a function will try to walk up the parent chain to find a method
-    // enclosing the function whose "this" we can close over.
-    compiler->numLocals = 1;
-    if (isFunction)
-    {
-      compiler->locals[0].name = NULL;
-      compiler->locals[0].length = 0;
-    }
-    else
-    {
-      compiler->locals[0].name = "this";
-      compiler->locals[0].length = 4;
-    }
-    compiler->locals[0].depth = -1;
-    compiler->locals[0].isUpvalue = false;
-
-    // The initial scope for function or method is a local scope.
+    // The initial scope for functions and methods is local scope.
     compiler->scopeDepth = 0;
   }
   
-  compiler->numSlots = compiler->numLocals;
-
   compiler->fn = wrenNewFunction(parser->vm, parser->module,
                                  compiler->numLocals);
 }
@@ -1867,7 +1867,7 @@ static void methodCall(Compiler* compiler, Code instruction,
     called.arity++;
 
     Compiler fnCompiler;
-    initCompiler(&fnCompiler, compiler->parser, compiler, true);
+    initCompiler(&fnCompiler, compiler->parser, compiler, false);
 
     // Make a dummy signature to track the arity.
     Signature fnSignature = { "", 0, SIG_METHOD, 0 };
@@ -3080,7 +3080,7 @@ static void createConstructor(Compiler* compiler, Signature* signature,
                               int initializerSymbol)
 {
   Compiler methodCompiler;
-  initCompiler(&methodCompiler, compiler->parser, compiler, false);
+  initCompiler(&methodCompiler, compiler->parser, compiler, true);
   
   // Allocate the instance.
   emitOp(&methodCompiler, compiler->enclosingClass->isForeign
@@ -3166,7 +3166,7 @@ static bool method(Compiler* compiler, Variable classVariable)
   compiler->enclosingClass->signature = &signature;
 
   Compiler methodCompiler;
-  initCompiler(&methodCompiler, compiler->parser, compiler, false);
+  initCompiler(&methodCompiler, compiler->parser, compiler, true);
 
   // Compile the method signature.
   signatureFn(&methodCompiler, &signature);
@@ -3445,7 +3445,7 @@ ObjFn* wrenCompile(WrenVM* vm, ObjModule* module, const char* source,
   int numExistingVariables = module->variables.count;
   
   Compiler compiler;
-  initCompiler(&compiler, &parser, NULL, true);
+  initCompiler(&compiler, &parser, NULL, false);
   ignoreNewlines(&compiler);
 
   if (isExpression)

src/vm/wren_core.c

@@ -59,15 +59,7 @@ DEF_PRIMITIVE(fiber_new)
     RETURN_ERROR("Function cannot take more than one parameter.");
   }
   
-  ObjFiber* newFiber = wrenNewFiber(vm, closure);
-
-  // The compiler expects the first slot of a function to hold the receiver.
-  // Since a fiber's stack is invoked directly, it doesn't have one, so put it
-  // in here.
-  newFiber->stack[0] = NULL_VAL;
-  newFiber->stackTop++;
-  
-  RETURN_OBJ(newFiber);
+  RETURN_OBJ(wrenNewFiber(vm, closure));
 }
 
 DEF_PRIMITIVE(fiber_abort)

src/vm/wren_value.c

@@ -159,27 +159,31 @@ ObjFiber* wrenNewFiber(WrenVM* vm, ObjClosure* closure)
   
   ObjFiber* fiber = ALLOCATE(vm, ObjFiber);
   initObj(vm, &fiber->obj, OBJ_FIBER, vm->fiberClass);
+
+  fiber->stack = stack;
+  fiber->stackTop = fiber->stack;
+  fiber->stackCapacity = stackCapacity;
+
   fiber->frames = frames;
   fiber->frameCapacity = INITIAL_CALL_FRAMES;
-  fiber->stack = stack;
-  fiber->stackCapacity = stackCapacity;
-  wrenResetFiber(vm, fiber, closure);
+  fiber->numFrames = 0;
 
-  return fiber;
-}
-
-void wrenResetFiber(WrenVM* vm, ObjFiber* fiber, ObjClosure* closure)
-{
-  // Reset everything.
-  fiber->stackTop = fiber->stack;
   fiber->openUpvalues = NULL;
   fiber->caller = NULL;
   fiber->error = NULL_VAL;
   fiber->callerIsTrying = false;
-  fiber->numFrames = 0;
+  
+  if (closure != NULL)
+  {
+    // Initialize the first call frame.
+    wrenAppendCallFrame(vm, fiber, closure, fiber->stack);
 
-  // Initialize the first call frame.
-  if (closure != NULL) wrenAppendCallFrame(vm, fiber, closure, fiber->stack);
+    // The first slot always holds the closure.
+    fiber->stackTop[0] = OBJ_VAL(closure);
+    fiber->stackTop++;
+  }
+  
+  return fiber;
 }
 
 void wrenEnsureStack(WrenVM* vm, ObjFiber* fiber, int needed)

src/vm/wren_value.h

@@ -628,10 +628,6 @@ ObjClosure* wrenNewClosure(WrenVM* vm, ObjFn* fn);
 // Creates a new fiber object that will invoke [closure].
 ObjFiber* wrenNewFiber(WrenVM* vm, ObjClosure* closure);
 
-// Resets [fiber] back to an initial state where it is ready to invoke
-// [closure].
-void wrenResetFiber(WrenVM* vm, ObjFiber* fiber, ObjClosure* closure);
-
 // Adds a new [CallFrame] to [fiber] invoking [closure] whose stack starts at
 // [stackStart].
 static inline void wrenAppendCallFrame(WrenVM* vm, ObjFiber* fiber,

src/vm/wren_vm.c

@@ -1249,17 +1249,18 @@ static WrenInterpretResult runInterpreter(WrenVM* vm, register ObjFiber* fiber)
       Value name = fn->constants.data[READ_SHORT()];
 
       // Make a slot on the stack for the module's fiber to place the return
-      // value. It will be popped after this fiber is resumed.
-      PUSH(NULL_VAL);
+      // value. It will be popped after this fiber is resumed. Store the
+      // imported module's closure in the slot in case a GC happens when
+      // invoking the closure.
+      PUSH(wrenImportModule(vm, name));
       
-      Value result = wrenImportModule(vm, name);
       if (!IS_NULL(fiber->error)) RUNTIME_ERROR();
 
       // If we get a closure, call it to execute the module body.
-      if (IS_CLOSURE(result))
+      if (IS_CLOSURE(PEEK()))
       {
         STORE_FRAME();
-        ObjClosure* closure = AS_CLOSURE(result);
+        ObjClosure* closure = AS_CLOSURE(PEEK());
         callFunction(vm, fiber, closure, 1);
         LOAD_FRAME();
       }
@@ -1417,7 +1418,7 @@ WrenInterpretResult wrenInterpretInModule(WrenVM* vm, const char* module,
   wrenPushRoot(vm, (Obj*)closure);
   ObjFiber* fiber = wrenNewFiber(vm, closure);
   wrenPopRoot(vm); // closure.
-
+  
   return runInterpreter(vm, fiber);
 }
 

test/language/variable/many_locals.wren

@@ -1,6 +1,6 @@
 {
-  var a0 = "value"
-  var a1 = a0
+  // Slot zero is always taken to hold the closure or receiver.
+  var a1 = "value"
   var a2 = a1
   var a3 = a2
   var a4 = a3

test/language/variable/many_nonsimultaneous_locals.wren

@@ -1,10 +1,10 @@
-// Can have more than 256 local variables in a local scope, as long as they
+// Can have more than 255 local variables in a local scope, as long as they
 // aren't all in scope at the same time.
 
 {
   {
-    var a0 = "value a"
-    var a1 = a0
+    // Slot zero is always taken to hold the closure or receiver.
+    var a1 = "value a"
     var a2 = a1
     var a3 = a2
     var a4 = a3
@@ -263,8 +263,8 @@
   }
 
   {
-    var b0 = "value b"
-    var b1 = b0
+    // Slot zero is always taken to hold the closure or receiver.
+    var b1 = "value b"
     var b2 = b1
     var b3 = b2
     var b4 = b3

test/language/variable/too_many_locals.wren

@@ -1,6 +1,6 @@
 {
-  var a0 = "value"
-  var a1 = a0
+  // Slot zero is always taken to hold the closure or receiver.
+  var a1 = "value"
   var a2 = a1
   var a3 = a2
   var a4 = a3

test/language/variable/too_many_locals_nested.wren

@@ -1,6 +1,6 @@
 {
-  var a0 = "value"
-  var a1 = a0
+  // Slot zero is always taken to hold the closure or receiver.
+  var a1 = "value"
   var a2 = a1
   var a3 = a2
   var a4 = a3