try/catch improvements (roblox-ts#2726)

Fixes roblox-ts#2647
Fixes roblox-ts#2725

- Removes unused `traceback` variable
- Uses `pcall` for simplicity
- Rethrows error after running `finally` if `catch` block doesn't exist
- If `catch` errors, it will run `finally` and then rethrow the error
- Added new tests

Author: osyrisrblx

include/RuntimeLib.lua

@@ -184,27 +184,56 @@ TS.TRY_RETURN = 1
 TS.TRY_BREAK = 2
 TS.TRY_CONTINUE = 3
 
-function TS.try(func, catch, finally)
-	local err, traceback
-	local success, exitType, returns = xpcall(
-		func,
-		function(errInner)
-			err = errInner
-			traceback = debug.traceback()
+function TS.try(try, catch, finally)
+	-- execute try
+	local trySuccess, exitTypeOrTryError, returns = pcall(try)
+	local exitType, tryError
+	if trySuccess then
+		exitType = exitTypeOrTryError
+	else
+		tryError = exitTypeOrTryError
+	end
+
+	local catchSuccess = true
+	local catchError
+
+	-- if try block failed, and catch block exists, execute catch
+	if not trySuccess and catch then
+		local newExitTypeOrCatchError, newReturns
+		catchSuccess, newExitTypeOrCatchError, newReturns = pcall(catch, tryError)
+		local newExitType
+		if catchSuccess then
+			newExitType = newExitTypeOrCatchError
+		else
+			catchError = newExitTypeOrCatchError
 		end
-	)
-	if not success and catch then
-		local newExitType, newReturns = catch(err, traceback)
+
 		if newExitType then
 			exitType, returns = newExitType, newReturns
 		end
 	end
+
+	-- execute finally
 	if finally then
 		local newExitType, newReturns = finally()
 		if newExitType then
 			exitType, returns = newExitType, newReturns
 		end
 	end
+
+	-- if exit type is a control flow, do not rethrow errors
+	if exitType ~= TS.TRY_RETURN and exitType ~= TS.TRY_BREAK and exitType ~= TS.TRY_CONTINUE then
+		-- if catch block threw an error, rethrow it
+		if not catchSuccess then
+			error(catchError, 2)
+		end
+
+		-- if try block threw an error and there was no catch block, rethrow it
+		if not trySuccess and not catch then
+			error(tryError, 2)
+		end
+	end
+
 	return exitType, returns
 end
 

tests/src/tests/try.spec.ts

@@ -18,6 +18,7 @@ export = () => {
 			return exception;
 		}
 		expect(foo()).to.be.a("string");
+		expect(tostring(foo()).find("bar")[0]).to.be.ok();
 	});
 
 	it("should support try/catch with throwing objects", () => {
@@ -33,13 +34,35 @@ export = () => {
 		expect(foo()).to.be.a("table");
 	});
 
-	it("should support try/catch with return", () => {
+	it("should support try/catch with return in try block", () => {
 		function foo(): unknown {
 			try {
 				return "foo";
 			} catch (e) {}
 		}
-		expect(foo()).to.be.a("string");
+		expect(foo()).to.equal("foo");
+	});
+
+	it("should support try/catch with return in catch block", () => {
+		function foo(): unknown {
+			try {
+				throw undefined;
+			} catch {
+				return "foo";
+			}
+		}
+		expect(foo()).to.equal("foo");
+	});
+
+	it("should support try/catch with return in finally block", () => {
+		function foo(): unknown {
+			try {
+				return 1;
+			} finally {
+				return 2;
+			}
+		}
+		expect(foo()).to.equal(2);
 	});
 
 	it("should support try/catch with break", () => {
@@ -181,4 +204,164 @@ export = () => {
 
 		expect(foo()).to.equal(2);
 	});
+
+	it("should rethrow the error if there's no catch block", () => {
+		function foo() {
+			try {
+				throw "bar";
+			} finally {
+			}
+		}
+
+		expect(() => foo()).to.throw("bar");
+	});
+
+	it("should run try -> catch -> finally in order", () => {
+		const array = new Array<number>();
+		try {
+			let condition = true;
+			try {
+				array.push(1);
+				if (condition) throw "error";
+				array.push(999);
+			} catch {
+				array.push(2);
+			} finally {
+				array.push(3);
+			}
+		} catch {}
+
+		expect(array[0]).to.equal(1);
+		expect(array[1]).to.equal(2);
+		expect(array[2]).to.equal(3);
+	});
+
+	it("should run try -> finally in order", () => {
+		const array = new Array<number>();
+		try {
+			let condition = true;
+			try {
+				array.push(1);
+				if (condition) throw "error";
+				array.push(999);
+			} finally {
+				array.push(2);
+			}
+		} catch {}
+
+		expect(array[0]).to.equal(1);
+		expect(array[1]).to.equal(2);
+	});
+
+	it("should run finally even if catch throws", () => {
+		let ranFinally = false;
+		try {
+			try {
+				throw "try error";
+			} catch {
+				throw "catch error";
+			} finally {
+				ranFinally = true;
+			}
+		} catch {}
+
+		expect(ranFinally).to.equal(true);
+	});
+
+	it("should throw if finally throws", () => {
+		function foo() {
+			try {
+			} finally {
+				throw "bar";
+			}
+		}
+
+		expect(() => foo()).to.throw("bar");
+	});
+
+	it("should discard errors if finally has a control flow statement", () => {
+		function tryErrorReturn() {
+			try {
+				throw "try error";
+			} finally {
+				return true;
+			}
+			return false;
+		}
+
+		expect(tryErrorReturn()).to.equal(true);
+
+		function catchErrorReturn() {
+			try {
+				throw "try error";
+			} catch {
+				throw "catch error";
+			} finally {
+				return true;
+			}
+			return false;
+		}
+
+		expect(catchErrorReturn()).to.equal(true);
+
+		function tryErrorBreak() {
+			for (let i = 0; i < 10; i++) {
+				try {
+					throw "try error";
+				} finally {
+					break;
+				}
+				return false;
+			}
+			return true;
+		}
+
+		expect(tryErrorBreak()).to.equal(true);
+
+		function catchErrorBreak() {
+			for (let i = 0; i < 1; i++) {
+				try {
+					throw "try error";
+				} catch {
+					throw "catch error";
+				} finally {
+					break;
+				}
+				return false;
+			}
+			return true;
+		}
+
+		expect(catchErrorBreak()).to.equal(true);
+
+		function tryErrorContinue() {
+			for (let i = 0; i < 1; i++) {
+				try {
+					throw "try error";
+				} finally {
+					continue;
+				}
+				return false;
+			}
+			return true;
+		}
+
+		expect(tryErrorContinue()).to.equal(true);
+
+		function catchErrorContinue() {
+			for (let i = 0; i < 1; i++) {
+				try {
+					throw "try error";
+				} catch {
+					throw "catch error";
+				} finally {
+					continue;
+				}
+				return false;
+			}
+			return true;
+		}
+
+		expect(catchErrorContinue()).to.equal(true);
+	});
 };