Improve display of runtime errors (#1320)

- Bump Cobalt to 0.6.0. We now track both line and column numbers of each bytecode instruction, allowing us to map an error to a concrete position. - `loadfile` (and similar functions) now use the full path, rather than the file name. Cobalt truncates this to 30 characters (rather than the previous 60) so this should be less noisy. - The shell, edit and Lua REPL now display the corresponding source code alongside an error. Note this is incredibly limited right now - it won't cope with errors which cross coroutine boundaries. Supporting this is on the roadmap, but requires some careful API design.
2025-10-31 13:42:59 +00:00 · 2023-02-09 20:53:50 +00:00
parent 62e3c5f9aa
commit 5502412181
10 changed files with 204 additions and 75 deletions
--- a/gradle/libs.versions.toml
+++ b/gradle/libs.versions.toml
@@ -14,7 +14,7 @@ parchmentMc = "1.19.2"
 asm = "9.3"
 autoService = "1.0.1"
 checkerFramework = "3.12.0"
-cobalt = "0.5.12"
+cobalt = "0.6.0"
 fastutil = "8.5.9"
 guava = "31.1-jre"
 jetbrainsAnnotations = "23.0.0"
@@ -51,7 +51,7 @@ fabric-loom = "1.0-SNAPSHOT"
 forgeGradle = "5.1.+"
 githubRelease = "2.2.12"
 ideaExt = "1.1.6"
-illuaminate = "0.1.0-12-ga03e9cd"
+illuaminate = "0.1.0-13-g689d73d"
 librarian = "1.+"
 minotaur = "2.+"
 mixinGradle = "0.7.+"
--- a/projects/core/src/main/resources/data/computercraft/lua/bios.lua
+++ b/projects/core/src/main/resources/data/computercraft/lua/bios.lua
@@ -7,7 +7,7 @@ local expect

 do
    local h = fs.open("rom/modules/main/cc/expect.lua", "r")
-    local f, err = loadstring(h.readAll(), "@expect.lua")
+    local f, err = loadstring(h.readAll(), "@/rom/modules/main/cc/expect.lua")
    h.close()

    if not f then error(err) end
@@ -467,7 +467,7 @@ function loadfile(filename, mode, env)
    local file = fs.open(filename, "r")
    if not file then return nil, "File not found" end

-    local func, err = load(file.readAll(), "@" .. fs.getName(filename), mode, env)
+    local func, err = load(file.readAll(), "@/" .. fs.combine(filename), mode, env)
    file.close()
    return func, err
 end
--- a/projects/core/src/main/resources/data/computercraft/lua/rom/modules/main/cc/internal/exception.lua
+++ b/projects/core/src/main/resources/data/computercraft/lua/rom/modules/main/cc/internal/exception.lua
@@ -0,0 +1,119 @@
+--[[- Internal tools for working with errors.
+
+:::warning
+This is an internal module and SHOULD NOT be used in your own code. It may
+be removed or changed at any time.
+:::
+
+@local
+]]
+
+local expect = require "cc.expect".expect
+local error_printer = require "cc.internal.error_printer"
+
+local function find_frame(thread, file, line)
+    -- Scan the first 16 frames for something interesting.
+    for offset = 0, 15 do
+        local frame = debug.getinfo(thread, offset, "Sl")
+        if not frame then break end
+
+        if frame.short_src == file and frame.what ~= "C" and frame.currentline == line then
+            return frame
+        end
+    end
+end
+
+--[[- Attempt to call the provided function `func` with the provided arguments.
+
+@tparam function func The function to call.
+@param ... Arguments to this function.
+
+@treturn[1] true If the function ran successfully.
+
+@treturn[2] false If the function failed.
+@return[2] The error message
+@treturn[2] coroutine The thread where the error occurred.
+]]
+local function try(func, ...)
+    expect(1, func, "function")
+
+    local co = coroutine.create(func)
+    local ok, result = coroutine.resume(co, ...)
+
+    while coroutine.status(co) ~= "dead" do
+        local event = table.pack(os.pullEventRaw(result))
+        if result == nil or event[1] == result or event[1] == "terminate" then
+            ok, result = coroutine.resume(co, table.unpack(event, 1, event.n))
+        end
+    end
+
+    if not ok then return false, result, co end
+    return true
+end
+
+--[[- Report additional context about an error.
+
+@param err The error to report.
+@tparam coroutine thread The coroutine where the error occurred.
+@tparam[opt] { [string] = string } source_map Map of chunk names to their contents.
+]]
+local function report(err, thread, source_map)
+    expect(2, thread, "thread")
+    expect(3, source_map, "table", "nil")
+
+    if type(err) ~= "string" then return end
+
+    local file, line = err:match("^([^:]+):(%d+):")
+    if not file then return end
+    line = tonumber(line)
+
+    local frame = find_frame(thread, file, line)
+    if not frame or not frame.currentcolumn then return end
+
+    local column = frame.currentcolumn
+    local line_contents
+    if source_map and source_map[frame.source] then
+        -- File exists in the source map.
+        local pos, contents = 1, source_map[frame.source]
+        -- Try to remap our position. The interface for this only makes sense
+        -- for single line sources, but that's sufficient for where we need it
+        -- (the REPL).
+        if type(contents) == "table" then
+            column = column - contents.offset
+            contents = contents.contents
+        end
+
+        for _ = 1, line - 1 do
+            local next_pos = contents:find("\n", pos)
+            if not next_pos then return end
+            pos = next_pos + 1
+        end
+
+        local end_pos = contents:find("\n", pos)
+        line_contents = contents:sub(pos, end_pos and end_pos - 1 or #contents)
+
+    elseif frame.source:sub(1, 2) == "@/" then
+        -- Read the file from disk.
+        local handle = fs.open(frame.source:sub(3), "r")
+        if not handle then return end
+        for _ = 1, line - 1 do handle.readLine() end
+
+        line_contents = handle.readLine()
+    end
+
+    -- Could not determine the line. Bail.
+    if not line_contents or #line_contents == "" then return end
+
+    error_printer({
+        get_pos = function() return line, column end,
+        get_line = function() return line_contents end,
+    }, {
+        { tag = "annotate", start_pos = column, end_pos = column, msg = "" },
+    })
+end
+
+
+return {
+    try = try,
+    report = report,
+}
--- a/projects/core/src/main/resources/data/computercraft/lua/rom/programs/edit.lua
+++ b/projects/core/src/main/resources/data/computercraft/lua/rom/programs/edit.lua
@@ -51,17 +51,21 @@ end

 local runHandler = [[multishell.setTitle(multishell.getCurrent(), %q)
 local current = term.current()
-local contents = %q
-local fn, err = load(contents, %q, nil, _ENV)
+local contents, name = %q, %q
+local fn, err = load(contents, name, nil, _ENV)
 if fn then
-    local ok, err = pcall(fn, ...)
+    local exception = require "cc.internal.exception"
+    local ok, err, co = exception.try(fn, ...)

    term.redirect(current)
    term.setTextColor(term.isColour() and colours.yellow or colours.white)
    term.setBackgroundColor(colours.black)
    term.setCursorBlink(false)

-    if not ok then printError(err) end
+    if not ok then
+        printError(err)
+        exception.report(err, co, { [name] = contents })
+    end
 else
    local parser = require "cc.internal.syntax"
    if parser.parse_program(contents) then printError(err) end
@@ -452,7 +456,7 @@ local tMenuFuncs = {
            return
        end
        local ok = save(sTempPath, function(file)
-            file.write(runHandler:format(sTitle, table.concat(tLines, "\n"), "@" .. fs.getName(sPath)))
+            file.write(runHandler:format(sTitle, table.concat(tLines, "\n"), "@/" .. sPath))
        end)
        if ok then
            local nTask = shell.openTab("/" .. sTempPath)
--- a/projects/core/src/main/resources/data/computercraft/lua/rom/programs/lua.lua
+++ b/projects/core/src/main/resources/data/computercraft/lua/rom/programs/lua.lua
@@ -6,13 +6,14 @@ if #tArgs > 0 then
 end

 local pretty = require "cc.pretty"
+local exception = require "cc.internal.exception"

-local bRunning = true
+local running = true
 local tCommandHistory = {}
 local tEnv = {
    ["exit"] = setmetatable({}, {
        __tostring = function() return "Call exit() to exit." end,
-        __call = function() bRunning = false end,
+        __call = function() running = false end,
    }),
    ["_echo"] = function(...)
        return ...
@@ -44,7 +45,8 @@ print("Interactive Lua prompt.")
 print("Call exit() to exit.")
 term.setTextColour(colours.white)

-while bRunning do
+local chunk_idx, chunk_map = 1, {}
+while running do
    --if term.isColour() then
    --    term.setTextColour( colours.yellow )
    --end
@@ -74,27 +76,32 @@ while bRunning do
       term.setTextColour(colours.white)
    end

-    local nForcePrint = 0
-    local func, err = load(input, "=lua", "t", tEnv)
-    local func2 = load("return _echo(" .. input .. ");", "=lua", "t", tEnv)
+    local name, offset = "=lua[" .. chunk_idx .. "]", 0
+
+    local force_print = 0
+    local func, err = load(input, name, "t", tEnv)
+
+    local expr_func = load("return _echo(" .. input .. ");", name, "t", tEnv)
    if not func then
-        if func2 then
-            func = func2
-            err = nil
-            nForcePrint = 1
-        end
-    else
-        if func2 then
-            func = func2
+        if expr_func then
+            func = expr_func
+            offset = 13
+            force_print = 1
        end
+    elseif expr_func then
+        func = expr_func
+        offset = 13
    end

    if func then
-        local tResults = table.pack(pcall(func))
-        if tResults[1] then
+        chunk_map[name] = { contents = input, offset = offset }
+        chunk_idx = chunk_idx + 1
+
+        local results = table.pack(exception.try(func))
+        if results[1] then
            local n = 1
-            while n < tResults.n or n <= nForcePrint do
-                local value = tResults[n + 1]
+            while n < results.n or n <= force_print do
+                local value = results[n + 1]
                local ok, serialised = pcall(pretty.pretty, value, {
                    function_args = settings.get("lua.function_args"),
                    function_source = settings.get("lua.function_source"),
@@ -107,7 +114,8 @@ while bRunning do
                n = n + 1
            end
        else
-            printError(tResults[2])
+            printError(results[2])
+            require "cc.internal.exception".report(results[2], results[3], chunk_map)
        end
    else
        local parser = require "cc.internal.syntax"
--- a/projects/core/src/main/resources/data/computercraft/lua/rom/programs/shell.lua
+++ b/projects/core/src/main/resources/data/computercraft/lua/rom/programs/shell.lua
@@ -67,6 +67,7 @@ do
    require = env.require
 end
 local expect = require("cc.expect").expect
+local exception = require "cc.internal.exception"

 -- Colours
 local promptColour, textColour, bgColour
@@ -146,7 +147,7 @@ local function executeProgram(remainingRecursion, path, args)
    local env = setmetatable(createShellEnv(dir), { __index = _G })
    env.arg = args

-    local func, err = load(contents, "@" .. fs.getName(path), nil, env)
+    local func, err = load(contents, "@/" .. path, nil, env)
    if not func then
        -- We had a syntax error. Attempt to run it through our own parser if
        -- the file is "small enough", otherwise report the original error.
@@ -166,13 +167,16 @@ local function executeProgram(remainingRecursion, path, args)
        end
    end

-    local ok, err = pcall(func, table.unpack(args))
-    if ok then
-        return true
-    else
-        if err and err ~= "" then printError(err) end
-        return false
+    local ok, err, co = exception.try(func, table.unpack(args, 1, args.n))
+
+    if ok then return true end
+
+    if err and err ~= "" then
+        printError(err)
+        exception.report(err, co)
    end
+
+    return false
 end

 --- Run a program with the supplied arguments.
--- a/projects/core/src/test/java/dan200/computercraft/core/LuaCoverage.java
+++ b/projects/core/src/test/java/dan200/computercraft/core/LuaCoverage.java
@@ -6,6 +6,7 @@
 package dan200.computercraft.core;

 import it.unimi.dsi.fastutil.ints.Int2IntMap;
+import it.unimi.dsi.fastutil.ints.Int2IntMaps;
 import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
 import it.unimi.dsi.fastutil.ints.IntSet;
 import org.slf4j.Logger;
@@ -17,19 +18,14 @@ import org.squiddev.cobalt.compiler.LuaC;
 import java.io.*;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.*;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.ArrayDeque;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Queue;

 class LuaCoverage {
    private static final Logger LOG = LoggerFactory.getLogger(LuaCoverage.class);
    private static final Path ROOT = new File("src/main/resources/data/computercraft/lua").toPath();
-    private static final Path BIOS = ROOT.resolve("bios.lua");
-    private static final Path APIS = ROOT.resolve("rom/apis");
-    private static final Path STARTUP = ROOT.resolve("rom/startup.lua");
-    private static final Path SHELL = ROOT.resolve("rom/programs/shell.lua");
-    private static final Path MULTISHELL = ROOT.resolve("rom/programs/advanced/multishell.lua");
-    private static final Path TREASURE = ROOT.resolve("treasure");

    private final Map<String, Int2IntMap> coverage;
    private final String blank;
@@ -49,26 +45,20 @@ class LuaCoverage {
    }

    void write(Writer out) throws IOException {
-        Files.find(ROOT, Integer.MAX_VALUE, (path, attr) -> attr.isRegularFile() && !path.startsWith(TREASURE)).forEach(path -> {
+        Files.find(ROOT, Integer.MAX_VALUE, (path, attr) -> attr.isRegularFile()).forEach(path -> {
            var relative = ROOT.relativize(path);
            var full = relative.toString().replace('\\', '/');
            if (!full.endsWith(".lua")) return;

-            var possiblePaths = Stream.of(
-                coverage.remove("/" + full),
-                path.equals(BIOS) ? coverage.remove("bios.lua") : null,
-                path.equals(STARTUP) ? coverage.remove("startup.lua") : null,
-                path.equals(SHELL) ? coverage.remove("shell.lua") : null,
-                path.equals(MULTISHELL) ? coverage.remove("multishell.lua") : null,
-                path.startsWith(APIS) ? coverage.remove(path.getFileName().toString()) : null
-            );
-            var files = possiblePaths
-                .filter(Objects::nonNull)
-                .flatMap(x -> x.int2IntEntrySet().stream())
-                .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue, Integer::sum));
+            var possiblePaths = coverage.remove("/" + full);
+            if (possiblePaths == null) possiblePaths = coverage.remove(full);
+            if (possiblePaths == null) {
+                possiblePaths = Int2IntMaps.EMPTY_MAP;
+                LOG.warn("{} has no coverage data", full);
+            }

            try {
-                writeCoverageFor(out, path, files);
+                writeCoverageFor(out, path, possiblePaths);
            } catch (IOException e) {
                throw new UncheckedIOException(e);
            }
@@ -116,22 +106,24 @@ class LuaCoverage {

    private static IntSet getActiveLines(File file) throws IOException {
        IntSet activeLines = new IntOpenHashSet();
+        Queue<Prototype> queue = new ArrayDeque<>();
+
        try (InputStream stream = new FileInputStream(file)) {
            var proto = LuaC.compile(stream, "@" + file.getPath());
-            Queue<Prototype> queue = new ArrayDeque<>();
            queue.add(proto);
+        } catch (CompileException e) {
+            throw new IllegalStateException("Cannot compile", e);
+        }

+        Prototype proto;
        while ((proto = queue.poll()) != null) {
-                var lines = proto.lineinfo;
+            var lines = proto.lineInfo;
            if (lines != null) {
                for (var line : lines) {
                    activeLines.add(line);
                }
            }
-                if (proto.p != null) Collections.addAll(queue, proto.p);
-            }
-        } catch (CompileException e) {
-            throw new IllegalStateException("Cannot compile", e);
+            if (proto.children != null) Collections.addAll(queue, proto.children);
        }

        return activeLines;
--- a/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerTest.java
+++ b/projects/core/src/test/java/dan200/computercraft/core/computer/ComputerTest.java
@@ -23,7 +23,7 @@ public class ComputerTest {
            try {
                ComputerBootstrap.run("print('Hello') while true do end", ComputerBootstrap.MAX_TIME);
            } catch (AssertionError e) {
-                if (e.getMessage().equals("test.lua:1: Too long without yielding")) return;
+                if (e.getMessage().equals("/test.lua:1: Too long without yielding")) return;
                throw e;
            }

--- a/projects/core/src/test/resources/test-rom/spec/apis/parallel_spec.lua
+++ b/projects/core/src/test/resources/test-rom/spec/apis/parallel_spec.lua
@@ -39,7 +39,9 @@ describe("The parallel library", function()
        end)

        it("passes errors to the caller", function()
-            expect.error(parallel.waitForAny, function() error("Test error") end):str_match("Test error$")
+            local ok, err = pcall(parallel.waitForAny, function() error("Test error") end)
+            if ok then fail("Expected function to error") end
+            expect(tostring(err)):str_match("Test error$")
        end)

        it("returns the number of the function that exited first", function()
@@ -98,7 +100,9 @@ describe("The parallel library", function()
        end)

        it("passes errors to the caller", function()
-            expect.error(parallel.waitForAll, function() error("Test error") end):str_match("Test error$")
+            local ok, err = pcall(parallel.waitForAll, function() error("Test error") end)
+            if ok then fail("Expected function to error") end
+            expect(tostring(err)):str_match("Test error$")
        end)

        it("completes all functions before exiting", function()
--- a/projects/core/src/test/resources/test-rom/spec/base_spec.lua
+++ b/projects/core/src/test/resources/test-rom/spec/base_spec.lua
@@ -28,8 +28,6 @@ describe("The Lua base library", function()
    end)

    describe("loadfile", function()
-        local loadfile = _G.native_loadfile or loadfile
-
        local function make_file()
            local tmp = fs.open("test-files/out.lua", "w")
            tmp.write("return _ENV")
@@ -48,7 +46,7 @@ describe("The Lua base library", function()

        it("prefixes the filename with @", function()
            local info = debug.getinfo(loadfile("/rom/startup.lua"), "S")
-            expect(info):matches { short_src = "startup.lua", source = "@startup.lua" }
+            expect(info):matches { short_src = "/rom/startup.lua", source = "@/rom/startup.lua" }
        end)

        it("loads a file with the global environment", function()