Fix returning from constructors (#845)

* Fix returning from constructors

 1. Do not allow returning with a value
 2. Return the instance, correctly, even when the user returned explicitly

* revise error message for consistency, revise implementation details a bit, fix extra args to finishBody

* clarify tests a bit

* document constructor return

Co-authored-by: ruby0x1 <ruby0x1@pm.me>

doc/site/classes.markdown

@@ -347,6 +347,20 @@ overloaded by [arity](#signature). A constructor *must* be a named method with
 a (possibly empty) argument list. Operators, getters, and setters cannot be
 constructors.
 
+A constructor returns the instance of the class being created, even if you 
+don't explicitly use `return`. It is valid to use `return` inside of a 
+constructor, but it is an error to have an expression after the return.
+That rule applies to `return this` as well, return handles that implicitly inside
+a constructor, so just `return` is enough.
+
+<pre class="snippet">
+return          //> valid, returns 'this'
+
+return variable //> invalid
+return null     //> invalid
+return this     //> also invalid
+</pre>
+
 A constructor is actually a pair of methods. You get a method on the class:
 
 <pre class="snippet">

src/vm/wren_compiler.c

@@ -355,7 +355,11 @@ struct sCompiler
   // The function being compiled.
   ObjFn* fn;
   
+  // The constants for the function being compiled.
   ObjMap* constants;
+
+  // Whether or not the compiler is for a constructor initializer
+  bool isInitializer;
 };
 
 // Describes where a variable is declared.
@@ -509,6 +513,7 @@ static void initCompiler(Compiler* compiler, Parser* parser, Compiler* parent,
   compiler->parent = parent;
   compiler->loop = NULL;
   compiler->enclosingClass = NULL;
+  compiler->isInitializer = false;
   
   // Initialize these to NULL before allocating in case a GC gets triggered in
   // the middle of initializing the compiler.
@@ -1749,13 +1754,13 @@ static bool finishBlock(Compiler* compiler)
 
 // Parses a method or function body, after the initial "{" has been consumed.
 //
-// It [isInitializer] is `true`, this is the body of a constructor initializer.
-// In that case, this adds the code to ensure it returns `this`.
-static void finishBody(Compiler* compiler, bool isInitializer)
+// If [Compiler->isInitializer] is `true`, this is the body of a constructor
+// initializer. In that case, this adds the code to ensure it returns `this`.
+static void finishBody(Compiler* compiler)
 {
   bool isExpressionBody = finishBlock(compiler);
 
-  if (isInitializer)
+  if (compiler->isInitializer)
   {
     // If the initializer body evaluates to a value, discard it.
     if (isExpressionBody) emitOp(compiler, CODE_POP);
@@ -1994,7 +1999,7 @@ static void methodCall(Compiler* compiler, Code instruction,
 
     fnCompiler.fn->arity = fnSignature.arity;
 
-    finishBody(&fnCompiler, false);
+    finishBody(&fnCompiler);
 
     // Name the function based on the method its passed to.
     char blockName[MAX_METHOD_SIGNATURE + 15];
@@ -3165,11 +3170,18 @@ void statement(Compiler* compiler)
     // Compile the return value.
     if (peek(compiler) == TOKEN_LINE)
     {
-      // Implicitly return null if there is no value.
-      emitOp(compiler, CODE_NULL);
+      // If there's no expression after return, initializers should 
+      // return 'this' and regular methods should return null
+      Code result = compiler->isInitializer ? CODE_LOAD_LOCAL_0 : CODE_NULL;
+      emitOp(compiler, result);
     }
     else
     {
+      if (compiler->isInitializer)
+      {
+        error(compiler, "A constructor cannot return a value.");
+      }
+
       expression(compiler);
     }
 
@@ -3306,6 +3318,8 @@ static bool method(Compiler* compiler, Variable classVariable)
 
   // Compile the method signature.
   signatureFn(&methodCompiler, &signature);
+
+  methodCompiler.isInitializer = signature.type == SIG_INITIALIZER;
   
   if (isStatic && signature.type == SIG_INITIALIZER)
   {
@@ -3335,7 +3349,7 @@ static bool method(Compiler* compiler, Variable classVariable)
   else
   {
     consume(compiler, TOKEN_LEFT_BRACE, "Expect '{' to begin method body.");
-    finishBody(&methodCompiler, signature.type == SIG_INITIALIZER);
+    finishBody(&methodCompiler);
     endCompiler(&methodCompiler, fullSignature, length);
   }
   

test/language/constructor/cannot_return_value.wren

@@ -0,0 +1,5 @@
+class Foo {
+  construct new() {
+    return 1 // expect error
+  }
+}

test/language/constructor/return_without_value.wren

@@ -0,0 +1,18 @@
+class Baz {
+  construct new() {}
+}
+
+class Bar {
+  construct new() {
+  }
+}
+
+class Foo {
+  construct new() {
+    return
+  }
+}
+System.print(Baz.new()) // expect: instance of Baz
+System.print(Bar.new()) // expect: instance of Bar
+System.print(Foo.new()) // expect: instance of Foo
+System.print(Foo.new() != null) // expect: true