Merge pull request #138 from water111/w/compiler-fixes

[Compiler] Fix return-from, returning none, and inline function block scoping bugs

common/versions.h

@@ -13,7 +13,7 @@
 namespace versions {
 // language version
 constexpr s32 GOAL_VERSION_MAJOR = 0;
-constexpr s32 GOAL_VERSION_MINOR = 1;
+constexpr s32 GOAL_VERSION_MINOR = 2;
 constexpr u32 ART_FILE_VERSION = 6;
 constexpr u32 LEVEL_FILE_VERSION = 30;
 constexpr u32 DGO_FILE_VERSION = 1;

doc/changelog.md

@@ -46,4 +46,9 @@
 - Static fields can now contain floating point values
 - Fixed a bug where loading a float from an object and immediately using it math would cause a compiler crash
 
-- Arrays of value types can be created on the stack with `new`.
+- Arrays of value types can be created on the stack with `new`.
+
+## V0.2
+- Breaking change: return type of a function using `return-from #f` to return a value from the entire function is now the lowest common ancestor of all possible return values.
+- Fixed bug where `return-from` could reach outside of an inlined function.
+- Fixed bug where `return-from` might not behave correctly when returning from inside a let inside an inlined function.

doc/goal_doc.md

@@ -66,7 +66,7 @@ Exit a `block` or function early.
 (return-from block-name value)
 ```
 
-Looks up the block and exits from it with the value. You can exit out nested blocks. If you are enclosed in multiple blocks with the same name, exits from the inner-most one with a matching name. Everything in a function is wrapped in a block named `#f`, so you can use `(return-from #f x)` to return early from a function with `x`.  Unlike returning from a block, using `return-from` to exit a function currently does _not_ modify the return type of your function and does _not_ check the type of what you return. 
+Looks up the block and exits from it with the value. You can exit out nested blocks. If you are enclosed in multiple blocks with the same name, exits from the inner-most one with a matching name. Everything in a function is wrapped in a block named `#f`, so you can use `(return-from #f x)` to return early from a function with `x`.  When using this form, it may change the return type of the function or block. The return type will be the lowest common ancestor type of all written return paths. If there is an unreachable return path, it will still be considered.
 
 Example
 ```lisp
@@ -109,6 +109,9 @@ Example:
 
 The `goto` form used very rarely outside of macros and inline assembly. Try to avoid using `goto`.
 
+## `top-level`
+This form is reserved by the compiler. Internally all forms in a file are grouped under a `top-level` form, so you may see this in error messages. Do not name anything `top-level`.
+
 # Compiler Forms - Compiler Control
 These forms are used to control the GOAL compiler, and are usually entered at the GOAL REPL, or as part of a macro that's executed at the GOAL REPL. These shouldn't really be used in GOAL source code.
 

goal_src/kernel/gcommon.gc

@@ -477,7 +477,7 @@
   "Delete the first occurance of item from a list and return the list.
   Does nothing if the item isn't in the list."
   (if (eq? (car lst) item)
-      (return-from #f (cdr lst))
+      (return-from #f (the pair (cdr lst)))
       )
   
   (let ((iter (cdr lst))

goalc/compiler/Env.cpp

@@ -248,6 +248,11 @@ std::unordered_map<std::string, Label>& LabelEnv::get_label_map() {
   return m_labels;
 }
 
+BlockEnv* LabelEnv::find_block(const std::string& name) {
+  (void)name;
+  return nullptr;
+}
+
 RegVal* FunctionEnv::lexical_lookup(goos::Object sym) {
   if (!sym.is_symbol()) {
     throw std::runtime_error("invalid symbol in lexical_lookup");

goalc/compiler/Env.h

@@ -234,6 +234,7 @@ class LabelEnv : public Env {
   explicit LabelEnv(Env* parent) : Env(parent) {}
   std::string print() override { return "labelenv"; }
   std::unordered_map<std::string, Label>& get_label_map() override;
+  BlockEnv* find_block(const std::string& name) override;
 
  protected:
   std::unordered_map<std::string, Label> m_labels;

goalc/compiler/compilation/Block.cpp

@@ -84,14 +84,16 @@ Val* Compiler::compile_block(const goos::Object& form, const goos::Object& _rest
   auto return_type = m_ts.lowest_common_ancestor(return_types);
   block_env->return_value->set_type(coerce_to_reg_type(return_type));
 
-  // an IR to move the result of the block into the block's return register (if no return-from's are
-  // taken)
-  auto ir_move_rv = std::make_unique<IR_RegSet>(block_env->return_value, result->to_gpr(fe));
+  if (!dynamic_cast<None*>(result)) {
+    // an IR to move the result of the block into the block's return register (if no return-from's
+    // are taken)
+    auto ir_move_rv = std::make_unique<IR_RegSet>(block_env->return_value, result->to_gpr(fe));
 
-  // note - one drawback of doing this single pass is that a block always evaluates to a gpr.
-  // so we may have an unneeded xmm -> gpr move that could have been an xmm -> xmm that could have
-  // been eliminated.
-  env->emit(std::move(ir_move_rv));
+    // note - one drawback of doing this single pass is that a block always evaluates to a gpr.
+    // so we may have an unneeded xmm -> gpr move that could have been an xmm -> xmm that could have
+    // been eliminated.
+    env->emit(std::move(ir_move_rv));
+  }
 
   // now we know the end of the block, so we set the label index to be on whatever comes after the
   // return move. functions always end with a "null" IR and "null" instruction, so this is safe.

goalc/compiler/compilation/Function.cpp

@@ -182,13 +182,16 @@ Val* Compiler::compile_lambda(const goos::Object& form, const goos::Object& rest
         new_func_env->settings.is_set = true;
       }
     });
-    if (result) {
+    if (result && !dynamic_cast<None*>(result)) {
       // got a result, so to_gpr it and return it.
       auto final_result = result->to_gpr(new_func_env.get());
       new_func_env->emit(std::make_unique<IR_Return>(return_reg, final_result));
-      lambda_ts.add_arg(final_result->type());
+      func_block_env->return_types.push_back(final_result->type());
+      auto return_type = m_ts.lowest_common_ancestor(func_block_env->return_types);
+      lambda_ts.add_arg(return_type);
+      //      lambda_ts.add_arg(final_result->type());
     } else {
-      // empty body, return none
+      // empty body or returning none, return none
       lambda_ts.add_arg(m_ts.make_typespec("none"));
     }
     // put null instruction at the end so jumps to the end have somewhere to go.
@@ -316,13 +319,13 @@ Val* Compiler::compile_function_or_method_call(const goos::Object& form, Env* en
     // construct a lexical environment
     auto lexical_env = fe->alloc_env<LexicalEnv>(env);
 
-    Env* compile_env = lexical_env;
+    Env* inlined_compile_env = lexical_env;
 
     // if need to, create a label env.
     // we don't want a separate label env with lets, but we do for inlined functions.
     // either inlined through the auto-inliner, or through an explicit (inline x) form.
     if (auto_inline || got_inlined_lambda) {
-      compile_env = fe->alloc_env<LabelEnv>(lexical_env);
+      inlined_compile_env = fe->alloc_env<LabelEnv>(lexical_env);
     }
 
     // check arg types
@@ -350,13 +353,25 @@ Val* Compiler::compile_function_or_method_call(const goos::Object& form, Env* en
       lexical_env->vars[head_as_lambda->lambda.params.at(i).name] = copy;
     }
 
+    // setup env
+    BlockEnv* inlined_block_env = nullptr;
+    RegVal* result_reg_if_return_from = nullptr;
+    if (auto_inline || got_inlined_lambda) {
+      inlined_block_env = fe->alloc_env<BlockEnv>(inlined_compile_env, "#f");
+      result_reg_if_return_from =
+          inlined_compile_env->make_ireg(get_none()->type(), emitter::RegKind::GPR);
+      inlined_block_env->return_value = result_reg_if_return_from;
+      inlined_block_env->end_label = Label(fe);
+      inlined_compile_env = inlined_block_env;
+    }
+
     // compile inline!
     bool first_thing = true;
     Val* result = get_none();
     for_each_in_list(head_as_lambda->lambda.body, [&](const goos::Object& o) {
-      result = compile_error_guard(o, compile_env);
+      result = compile_error_guard(o, inlined_compile_env);
       if (!dynamic_cast<None*>(result)) {
-        result = result->to_reg(compile_env);
+        result = result->to_reg(inlined_compile_env);
       }
       if (first_thing) {
         first_thing = false;
@@ -366,6 +381,27 @@ Val* Compiler::compile_function_or_method_call(const goos::Object& form, Env* en
 
     // ignore the user specified return type and return the most specific type.
     // todo - does this make sense for an inline function? Should we check the return type?
+
+    if (inlined_block_env && !inlined_block_env->return_types.empty()) {
+      // there were return froms used in the function, so we fall back to using the separate
+      // return gpr.
+      if (!dynamic_cast<None*>(result)) {
+        auto final_result = result->to_gpr(inlined_compile_env);
+        inlined_compile_env->emit(
+            std::make_unique<IR_RegSet>(result_reg_if_return_from, final_result));
+        inlined_block_env->return_types.push_back(final_result->type());
+        auto return_type = m_ts.lowest_common_ancestor(inlined_block_env->return_types);
+        inlined_block_env->return_value->set_type(return_type);
+      } else {
+        inlined_block_env->return_value->set_type(get_none()->type());
+      }
+
+      inlined_compile_env->emit(std::make_unique<IR_Null>());
+      inlined_block_env->end_label.idx = inlined_block_env->end_label.func->code().size();
+      return inlined_block_env->return_value;
+    }
+
+    inlined_compile_env->emit(std::make_unique<IR_Null>());
     return result;
   } else {
     // not an inlined/immediate, it's a real function call.

goalc/compiler/compilation/Type.cpp

@@ -654,7 +654,6 @@ Val* Compiler::compile_new(const goos::Object& form, const goos::Object& _rest,
   } else if (allocation == "stack") {
     auto type_of_object = m_ts.make_typespec(type_as_string);
 
-    printf("type as string is %s\n", type_as_string.c_str());
     if (type_as_string == "inline-array" || type_as_string == "array") {
       bool is_inline = type_as_string == "inline-array";
       auto elt_type = quoted_sym_as_string(pair_car(*rest));

test/goalc/source_templates/functions/function-returning-none.static.gc

@@ -0,0 +1,6 @@
+(defun function-returning-none ()
+  (none)
+  )
+
+(function-returning-none)
+1

test/goalc/source_templates/functions/inline-with-block-1.static.gc

@@ -0,0 +1,11 @@
+
+; this is testing that return-from's work correctly inside of inlined functions.
+(defun inline-with-block-1 ()
+  (declare (inline))
+  (if (= 123 (block my-block 1 (return-from my-block 123) 2))
+      1
+      0
+      )
+  )
+
+(inline-with-block-1)

test/goalc/source_templates/functions/inline-with-block-2.static.gc

@@ -0,0 +1,11 @@
+;; testing that we can have a block's final statement be a none.
+
+(defun inline-with-block-2 ()
+  (declare (inline))
+  (block a-block
+         (return-from a-block 3)
+         )
+  )
+
+;; this is a little hacky, this technically returns none.
+(inline-with-block-2)

test/goalc/source_templates/functions/inline-with-block-3.static.gc

@@ -0,0 +1,10 @@
+(defun inline-with-block-3 ()
+  (declare (inline))
+  (block a-block
+         (return-from a-block 4)
+         )
+  (none)
+  )
+
+(inline-with-block-3)
+4

test/goalc/source_templates/functions/inline-with-block-4.static.gc

@@ -0,0 +1,14 @@
+(define format _format)
+
+(defun even-odd-to-1-2 ((in int))
+  (declare (inline))
+  (if (= 0 (logand 1 in))
+      (let ((x 1.0))
+        (return-from #f 1.0)
+        )
+      123123.23
+      )
+  2.0
+  )
+
+(format #t "~f~%" (+ (even-odd-to-1-2 11) (even-odd-to-1-2 12)))

test/goalc/source_templates/functions/return-from-trick.static.gc

@@ -0,0 +1,8 @@
+(defun return-from-let ()
+  (let ((x 1))
+    (return-from #f x)
+    )
+  3
+  )
+
+(return-from-let)

test/goalc/test_functions.cpp

@@ -114,3 +114,27 @@ TEST_F(FunctionTests, AllowInline) {
   EXPECT_EQ(got_mult, 1);
   EXPECT_EQ(got_call, 1);
 }
+
+TEST_F(FunctionTests, ReturnNone) {
+  runner.run_static_test(env, testCategory, "function-returning-none.static.gc", {"1\n"});
+}
+
+TEST_F(FunctionTests, InlineBlock1) {
+  runner.run_static_test(env, testCategory, "inline-with-block-1.static.gc", {"1\n"});
+}
+
+TEST_F(FunctionTests, InlineBlock2) {
+  runner.run_static_test(env, testCategory, "inline-with-block-2.static.gc", {"3\n"});
+}
+
+TEST_F(FunctionTests, InlineBlock3) {
+  runner.run_static_test(env, testCategory, "inline-with-block-3.static.gc", {"4\n"});
+}
+
+TEST_F(FunctionTests, InlineBlock4) {
+  runner.run_static_test(env, testCategory, "inline-with-block-4.static.gc", {"3.0000\n0\n"});
+}
+
+TEST_F(FunctionTests, ReturnFromTrick) {
+  runner.run_static_test(env, testCategory, "return-from-trick.static.gc", {"1\n"});
+}