feat: stop closure upvalue search at method boundaries and treat capitalized names as globals

Modify `findUpvalue` in the compiler to return -1 when the enclosing function is a method, preventing closures from capturing outer local variables. Add a new `nonlocal` variable resolution path that treats capitalized identifiers as global variables rather than method receivers. Update existing test files to use capitalized global names and add new test cases for nonlocal assignment, duplicate nonlocal declarations, block scoping, and method-local variable shadowing.

src/wren_compiler.c

@@ -1117,13 +1117,13 @@ static int addUpvalue(Compiler* compiler, bool isLocal, int index)
 // If the name is found outside of the immediately enclosing function, this
 // will flatten the closure and add upvalues to all of the intermediate
 // functions so that it gets walked down to this one.
+//
+// If it reaches a method boundary, this stops and returns -1 since methods do
+// not close over local variables.
 static int findUpvalue(Compiler* compiler, const char* name, int length)
 {
-  // If we are out of enclosing functions, it can't be an upvalue.
-  if (compiler->parent == NULL)
-  {
-    return -1;
-  }
+  // If we are at a method boundary or the top level, we didn't find it.
+  if (compiler->parent == NULL || compiler->enclosingClass != NULL) return -1;
 
   // See if it's a local variable in the immediately enclosing function.
   int local = resolveLocal(compiler->parent, name, length);
@@ -1137,11 +1137,11 @@ static int findUpvalue(Compiler* compiler, const char* name, int length)
   }
 
   // See if it's an upvalue in the immediately enclosing function. In other
-  // words, if its a local variable in a non-immediately enclosing function.
-  // This will "flatten" closures automatically: it will add upvalues to all
-  // of the intermediate functions to get from the function where a local is
-  // declared all the way into the possibly deeply nested function that is
-  // closing over it.
+  // words, if it's a local variable in a non-immediately enclosing function.
+  // This "flattens" closures automatically: it adds upvalues to all of the
+  // intermediate functions to get from the function where a local is declared
+  // all the way into the possibly deeply nested function that is closing over
+  // it.
   int upvalue = findUpvalue(compiler->parent, name, length);
   if (upvalue != -1)
   {
@@ -1154,13 +1154,13 @@ static int findUpvalue(Compiler* compiler, const char* name, int length)
 }
 
 // Look up [name] in the current scope to see what name it is bound to. Returns
-// the index of the name either in global scope, local scope, or the enclosing
-// function's upvalue list. Returns -1 if not found.
+// the index of the name either in local scope, or the enclosing function's
+// upvalue list. Does not search the global scope. Returns -1 if not found.
 //
 // Sets [loadInstruction] to the instruction needed to load the variable. Will
-// be one of [CODE_LOAD_LOCAL], [CODE_LOAD_UPVALUE], or [CODE_LOAD_GLOBAL].
-static int resolveName(Compiler* compiler, const char* name, int length,
-                       Code* loadInstruction)
+// be [CODE_LOAD_LOCAL] or [CODE_LOAD_UPVALUE].
+static int resolveNonglobal(Compiler* compiler, const char* name, int length,
+                            Code* loadInstruction)
 {
   // Look it up in the local scopes. Look in reverse order so that the most
   // nested variable is found first and shadows outer ones.
@@ -1171,13 +1171,23 @@ static int resolveName(Compiler* compiler, const char* name, int length,
   // If we got here, it's not a local, so lets see if we are closing over an
   // outer local.
   *loadInstruction = CODE_LOAD_UPVALUE;
-  int upvalue = findUpvalue(compiler, name, length);
-  if (upvalue != -1) return upvalue;
+  return findUpvalue(compiler, name, length);
+}
+
+// Look up [name] in the current scope to see what name it is bound to. Returns
+// the index of the name either in global scope, local scope, or the enclosing
+// function's upvalue list. Returns -1 if not found.
+//
+// Sets [loadInstruction] to the instruction needed to load the variable. Will
+// be one of [CODE_LOAD_LOCAL], [CODE_LOAD_UPVALUE], or [CODE_LOAD_GLOBAL].
+static int resolveName(Compiler* compiler, const char* name, int length,
+                       Code* loadInstruction)
+{
+  int nonglobal = resolveNonglobal(compiler, name, length, loadInstruction);
+  if (nonglobal != -1) return nonglobal;
 
-  // If we got here, it wasn't in a local scope, so try the global scope.
   *loadInstruction = CODE_LOAD_GLOBAL;
-  return wrenSymbolTableFind(
-      &compiler->parser->vm->globalNames, name, length);
+  return wrenSymbolTableFind(&compiler->parser->vm->globalNames, name, length);
 }
 
 static void loadLocal(Compiler* compiler, int slot)
@@ -1531,9 +1541,7 @@ static void namedCall(Compiler* compiler, bool allowAssignment,
 static void loadThis(Compiler* compiler)
 {
   Code loadInstruction;
-  int index = resolveName(compiler, "this", 4, &loadInstruction);
-  ASSERT(index == -1 || loadInstruction != CODE_LOAD_GLOBAL,
-         "'this' should not be global.");
+  int index = resolveNonglobal(compiler, "this", 4, &loadInstruction);
   if (loadInstruction == CODE_LOAD_LOCAL)
   {
     loadLocal(compiler, index);
@@ -1735,63 +1743,68 @@ static void staticField(Compiler* compiler, bool allowAssignment)
       // Implicitly initialize it to null.
       emit(classCompiler, CODE_NULL);
       defineVariable(classCompiler, symbol);
+    }
 
-      index = resolveName(compiler, token->start, token->length,
-                          &loadInstruction);
-    }
-    else
-    {
-      // It exists already, so resolve it properly. This is different from the
-      // above resolveLocal() call because we may have already closed over it
-      // as an upvalue.
-      index = resolveName(compiler, token->start, token->length,
-                          &loadInstruction);
-    }
+    // It definitely exists now, so resolve it properly. This is different from
+    // the above resolveLocal() call because we may have already closed over it
+    // as an upvalue.
+    index = resolveName(compiler, token->start, token->length,
+                        &loadInstruction);
   }
 
   variable(compiler, allowAssignment, index, loadInstruction);
 }
 
+// Returns `true` if [name] is a local variable name (starts with a lowercase
+// letter).
+static bool isLocalName(const char* name)
+{
+  return name[0] >= 'a' && name[0] <= 'z';
+}
+
+// Compiles a variable name or method call with an implicit receiver.
 static void name(Compiler* compiler, bool allowAssignment)
 {
-  // Look up the name in the scope chain.
+  // Look for the name in the scope chain up to the nearest enclosing method.
   Token* token = &compiler->parser->previous;
 
   Code loadInstruction;
-  int index = resolveName(compiler, token->start, token->length,
-                          &loadInstruction);
+  int index = resolveNonglobal(compiler, token->start, token->length,
+                               &loadInstruction);
   if (index != -1)
   {
     variable(compiler, allowAssignment, index, loadInstruction);
     return;
   }
-  // TODO: The fact that we return here if the variable is known and parse an
-  // optional argument list below if not means that the grammar is not
+
+  // TODO: The fact that we return above here if the variable is known and parse
+  // an optional argument list below if not means that the grammar is not
   // context-free. A line of code in a method like "someName(foo)" is a parse
   // error if "someName" is a defined variable in the surrounding scope and not
   // if it isn't. Fix this. One option is to have "someName(foo)" always
   // resolve to a self-call if there is an argument list, but that makes
   // getters a little confusing.
 
-  // TODO: The fact that we walk the entire scope chain up to global before
-  // interpreting a name as an implicit "this" call means that surrounding
-  // names shadow ones in the class. This is good for things like globals.
-  // (You wouldn't want `new Fiber` translating to `new this.Fiber`.) But it
-  // may not be what we want for other names. One option is to make capitalized
-  // names *always* global, and then a lowercase name will become on an
-  // implicit this if it's not a local in the nearest enclosing class.
-
-  // Otherwise, if we are inside a class, it's a call with an implicit "this"
-  // receiver.
-  ClassCompiler* classCompiler = getEnclosingClass(compiler);
-  if (classCompiler == NULL)
+  // If we're inside a method and the name is lowercase, treat it as a method
+  // on this.
+  if (isLocalName(token->start) && getEnclosingClass(compiler) != NULL)
   {
+    loadThis(compiler);
+    namedCall(compiler, allowAssignment, CODE_CALL_0);
+    return;
+  }
+
+  // Otherwise, look for a global variable with the name.
+  int global = wrenSymbolTableFind(&compiler->parser->vm->globalNames,
+                                   token->start, token->length);
+  if (global == -1)
+  {
+    // TODO: Implicitly declare it.
     error(compiler, "Undefined variable.");
     return;
   }
 
-  loadThis(compiler);
-  namedCall(compiler, allowAssignment, CODE_CALL_0);
+  variable(compiler, allowAssignment, global, CODE_LOAD_GLOBAL);
 }
 
 static void null(Compiler* compiler, bool allowAssignment)

test/closure/close_over_method_parameter.wren

@@ -1,12 +1,12 @@
-var f = null
+var F = null
 
 class Foo {
   method(param) {
-    f = new Fn {
+    F = new Fn {
       IO.print(param)
     }
   }
 }
 
 (new Foo).method("param")
-f.call // expect: param
+F.call // expect: param

test/nonlocal/assignment.wren

@@ -0,0 +1,18 @@
+var Nonlocal = "before"
+IO.print(Nonlocal) // expect: before
+Nonlocal = "after"
+IO.print(Nonlocal) // expect: after
+
+class Foo {
+  static method {
+    Nonlocal = "method"
+  }
+}
+
+Foo.method
+IO.print(Nonlocal) // expect: method
+
+new Fn {
+  Nonlocal = "fn"
+}.call
+IO.print(Nonlocal) // expect: fn

test/nonlocal/duplicate_nonlocal.wren

@@ -0,0 +1,6 @@
+class Foo {
+  bar {
+    var A = "value"
+    var A = "other" // expect error
+  }
+}

test/nonlocal/in_block_scope.wren

@@ -0,0 +1,8 @@
+var Nonlocal = "outer"
+
+{
+  var Nonlocal = "inner"
+  IO.print(Nonlocal) // expect: inner
+}
+
+IO.print(Nonlocal) // expect: outer

test/nonlocal/use_global_in_function.wren

@@ -1,6 +1,6 @@
-var global = "global"
+var Global = "global"
 // TODO: Forward reference to global declared after use.
 
 new Fn {
-  IO.print(global) // expect: global
+  IO.print(Global) // expect: global
 }.call

test/nonlocal/use_global_in_method.wren

@@ -1,13 +1,13 @@
-var global = "global"
+var Global = "global"
 // TODO: Forward reference to global declared after use.
 
 class Foo {
   method {
-    IO.print(global)
+    IO.print(Global)
   }
 
   static classMethod {
-    IO.print(global)
+    IO.print(Global)
   }
 }
 

test/variable/outside_method.wren

@@ -0,0 +1,18 @@
+var foo = "variable"
+
+class Foo {
+  foo { "method" }
+
+  method {
+    IO.print(foo)
+  }
+
+  static foo { "class method" }
+
+  static classMethod {
+    IO.print(foo)
+  }
+}
+
+(new Foo).method // expect: method
+Foo.classMethod  // expect: class method