CC: Tweaked

CC: Tweaked

57M Downloads

Standardize Error Handling in Builtin Programs

fatboychummy opened this issue ยท 7 comments

commented

I am currently discussing with Xella the implementation of a feature to detect failed installations for PineStore download tracking. Specifically, if an installation fails, it should not log a download. However, we've encountered an issue with many built-in programs (such as pastebin get/run) not returning an error state directly through error(), but instead using printError().

This poses a challenge as there is no clear indication of installation failure when tracking the program's status. Consequently, since the program consistently returns an OK status, determining if the installation actually failed becomes problematic.

Thus, I would like to go over all builtin CC programs and change any printError's/io.stderr:write()'s into proper error()s. I am, however, not entirely sure how this may affect things overall, so I would appreciate any insights, suggestions, or alternative solutions.

I am not sure of a timeframe on this as well, as I'm running into finals, but I would like to look into this sometime in the next month.

commented

Yeah, the lack of a good os.exit(1) alternative is definitely a pain. The trouble with error is there's no good way to tell if something is user-friendly error (where we just want to print it) vs a bug (where we might want to print the context or a stack trace). CC currently does this by checking the error contains a filename and line number, but other shells like mbs don't bother.

The "best" pattern here is probably to display the message normally (printError/io.stderr:write), and then exit with a no-args error().

Edit: Though lua and edit's run wrapper don't handle that very well. Technically the most standard-compliant option is to do error(setmetatable({}, {__tostring = function() return "My error message" end })), but that's even uglier! We could have a helper for this I guess??? Not sure!


As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!

commented

As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!

How come?

commented

Yeah, the lack of a good os.exit(1) alternative is definitely a pain. The trouble with error is there's no good way to tell if something is user-friendly error (where we just want to print it) vs a bug (where we might want to print the context or a stack trace). CC currently does this by checking the error contains a filename and line number, but other shells like mbs don't bother.

The "best" pattern here is probably to display the message normally (printError/io.stderr:write), and then exit with a no-args error().

Edit: Though lua and edit's run wrapper don't handle that very well. Technically the most standard-compliant option is to do error(setmetatable({}, {__tostring = function() return "My error message" end })), but that's even uglier! We could have a helper for this I guess??? Not sure!

As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!

You can just do error("My error message", 0), that will lead same result than printError if no one catched it

commented

Finally looking like I have time to work on this a bit.

Edit: Though lua and edit's run wrapper don't handle that very well. Technically the most standard-compliant option is to do error(setmetatable({}, {__tostring = function() return "My error message" end })), but that's even uglier! We could have a helper for this I guess??? Not sure!

The helper should (hopefully) be easy enough to do, but I'm more curious about what we would want for data in the error object, if we went this route. My initial thoughts would be have a type field with specific defined "types", and each type would then have a set of defined fields. For some rough examples:

-- Instead of
if err == "Too long without yielding" then

-- Use
if err.type == "timeout" then -- or something along these lines



-- Instead of
if err:match("attempt to use a closed file") then

-- Use
if err.type == "closed_file" then
  print("Attempted to use closed filehandle of file", err.filename)



-- And for a more extreme example...
-- Instead of
local line, argn, expected, got = err:match(":(%d+): Bad argument #(%d+): Expected (%S+), got (%S+)")
if line then
  print("Bad argument:", line, argn, expected, got)
  -- ...

-- Use
if err.type == "argument_type_mismatch" then
  print("Bad argument:", err.line, err.argn, err.expected, err.got)
  -- ...

I'm blanking on other things that would be useful to have available in every such object, other than possibly line number and epoch time? Perhaps just stuff from debug.getinfo?

Do we also want to override error so that it converts error("some message") into an error object with some simple data, say:

error("some message")
-> setmetatable(
     {
       type = "message",
       rawMessage = "some message",
       currentline = 71,
       short_src = "/error_test.lua", 
       -- ...
     },
     {__tostring = function() return "/error_test.lua:71: some message" end}
   )

Do we want errors raised in java methods to have this object structure?

Edit: Woops, forgot to rewrite part of the first paragraph before I sent this.

commented

As an aside, installers really shouldn't be shelling out to pastebin/wget. Those programs make sense as the entrypoint to an installer, but anything else should just use the http API normally!

How come?

Ideally you should handle the requests/downloads yourself for more control over whether or not there was an issue downloading your software.

PineStore's issue (and the main reason I noticed that some programs don't actually throw errors) is that it is less an install script, and more an install command fetcher and download logger, if that makes sense. It's not actually the script that is installing a user's program, instead it wraps around a command. I think I explained this part poorly in the original issue body, so I'll give a better explanation below.

Explanation

In PineStore, you can set an install command. The script PineStore uses acts as a wrapper around this command, and it pretty much just does something along the lines of if shell.run(install_command) then log_download() else error("Installation failed!") end.

However, wget never explicitly calls error (instead just calling printError). Thus, if your install command is something like wget run https://example-site.com/installer.lua and example-site.com is down for whatever reason, the following events will happen:

  1. The wrapper will run the install command.
  2. wget run https://example-site.com/installer.lua will fail, since the server is down/unreachable. No error is raised, however.
  3. shell.run returns an OK status -- the install command didn't error.
  4. The PineStore wrapper erroneously logs a successful download.
commented

I think it's useful to split errors into two categories:

  • Runtime errors: these are errors that occur due to bugs in the program (e.g. Too long without yielding, attempt to index nil, etc...). These (typically) have a source position associated with them, and errors are pretty printed in the shell.
  • User-facing errors: This is what you mention in the original issue. These are errors intentionally thrown by programs to indicate a problem to the user.

There's definitely some overlap here, but I think it's probably a mistake to mix the two up. I think we should probably just focus on user-facing errors for now, as that's much simpler.


As far as structured runtime errors goes, it's a bit of a backwards compatibility nightmare.

CC does have an (undocumented) notion of "rich" errors, which are errors that capture the context where they were thrown. This allows coroutine managers to rethrow errors and have the shell report the error in a fancy way (see an example in metis).

In #1320, I originally had parallel use this rich error system, but that broke several programs doing pcall(parallel.waitForAny) and pattern-matching on the error. This is terrible code, but unfortunately we have seen it in the wild!

Changing the actual Lua VM to throw structured errors is a definite no-no. I think we could improve error reporting by matching on the error and maybe parsing the code (see #1245 (comment)), but this isn't something that needs/should be exposed to the user.

commented

I originally had parallel use this rich error system, but that broke several programs doing pcall(parallel.waitForAny) and pattern-matching on the error. This is terrible code, but unfortunately we have seen it in the wild!

I'll admit this is really ugly looking, but we could do something like this to still support pattern matching (and other string operations) on these rich error objects:

local rich_error = setmetatable(
  {
    message = "Match test 1-2-3",
  },
  {
    __tostring = function(self)
      return self.message
    end,
    __index = function(_, idx)
      if string[idx] then
        if type(string[idx]) == "function" then
          return function(self, ...)
            return string[idx](tostring(self), ...)
          end
        end
        return string[idx]
      end
    end,
    __concat = function(a, b)
      return tostring(a) .. tostring(b)
    end,
  }
)

print(rich_error:match("Match test"))
print(rich_error .. " noot")

Thoughts? If I went through with this and wrote such a rich error builder module, should it be put in as a cc.internal module, or expose it as something like cc.errors?

If you think this will lead to too many issues, I'll probably just go ahead and start doing what was originally suggested -- printing the error, then no-arg erroring. Though that brings up another question: In programs which do call error instead of printError, should these also be updated to call printError(message); error()?