Fix try/finally block

Not having an exception catching block is quite legal, so we shouldn't assume that there is one. At which point, we should avoid placeholders for finding exception types, so we don't try to get the string content of a NIL_VAL, which results in a segfault.
cometlang · Mar 2, 2023 · 37bf6f6 · 37bf6f6
1 parent 820b245
commit 37bf6f6
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 9 deletions.
diff --git a/test_scripts/exception_handler.cmt b/test_scripts/exception_handler.cmt
@@ -18,4 +18,14 @@ function second_function()
     throw Exception("My exception has a message")
 }
 
+function test_function() {
+    try {
+        print("inside the try")
+    }
+    finally {
+        print('finally after try')
+    }
+}
+
 first_function()
+test_function()
diff --git a/vmlib/compiler/src/statements.c b/vmlib/compiler/src/statements.c
@@ -253,9 +253,11 @@ void tryStatement(Parser *parser)
     emitByte(parser, OP_POP_EXCEPTION_HANDLER);
     int successJump = emitJump(parser, OP_JUMP);
     match(parser, TOKEN_EOL);
+    bool tryBlockCompleted = false;
 
     if (match(parser, TOKEN_CATCH))
     {
+        tryBlockCompleted = true;
         beginScope(parser);
         consume(parser, TOKEN_LEFT_PAREN, "Expect '(' after catch");
         consume(parser, TOKEN_IDENTIFIER, "Expect type name to catch");
@@ -279,6 +281,7 @@ void tryStatement(Parser *parser)
 
     if (match(parser, TOKEN_FINALLY))
     {
+        tryBlockCompleted = true;
         // If we arrive here from either the try or handler blocks, then we don't
         // want to continue propagating the exception
         emitByte(parser, OP_FALSE);
@@ -292,6 +295,11 @@ void tryStatement(Parser *parser)
         patchJump(parser, continueExecution);
         emitByte(parser, OP_POP);
     }
+
+    if (tryBlockCompleted == false)
+    {
+        errorAtCurrent(parser, "A try statement requires a catch, finally or both");
+    }
 }
 
 void throwStatement(Parser *parser)

diff --git a/vmlib/vm.c b/vmlib/vm.c
@@ -80,7 +80,7 @@ static bool propagateException(VM *vm)
         for (int numHandlers = frame->handlerCount; numHandlers > 0; numHandlers--)
         {
             ExceptionHandler handler = frame->handlerStack[numHandlers - 1];
-            if (instanceof(exception, handler.klass) == TRUE_VAL)
+            if (handler.klass != NIL_VAL && instanceof(exception, handler.klass) == TRUE_VAL)
             {
                 frame->ip = &frame->closure->function->chunk.code[handler.handlerAddress];
                 return true;
@@ -1089,16 +1089,20 @@ static InterpretResult run(VM *vm)
         }
         case OP_PUSH_EXCEPTION_HANDLER:
         {
-            VALUE type = READ_CONSTANT();
+            uint8_t constantIndex = READ_BYTE();
+            VALUE type = NIL_VAL;
             uint16_t handlerAddress = READ_SHORT();
             uint16_t finallyAddress = READ_SHORT();
-            Value value;
-            if ((!findModuleVariable(frame->closure->function->module, type, &value) &&
-                 !findGlobal(type, &value)) ||
-                (!IS_CLASS(value) && !IS_NATIVE_CLASS(value)))
-            {
-                runtimeError(vm, "'%s' is not a type to catch", string_get_cstr(type));
-                return INTERPRET_RUNTIME_ERROR;
+            Value value = NIL_VAL;
+            if (constantIndex != 0xff) {
+                type = frame->closure->function->chunk.constants.values[constantIndex];
+                if ((!findModuleVariable(frame->closure->function->module, type, &value) &&
+                    !findGlobal(type, &value)) ||
+                    (!IS_CLASS(value) && !IS_NATIVE_CLASS(value)))
+                {
+                    runtimeError(vm, "'%s' is not a type to catch", string_get_cstr(type));
+                    return INTERPRET_RUNTIME_ERROR;
+                }
             }
             pushExceptionHandler(vm, value, handlerAddress, finallyAddress);
             break;