Add support for basic arithmetic metamethods, improve operator error messages

[Aeledfyr]

Adds support for the __add, __sub, __mul, and __div metamethods. In addition, this PR adds slightly more context to errors when no metamethod was found; errors include the attempted operation and the relevant types. (The formatting is done through thiserror and the standard formatting machinery.)

The error handling and metamethod changes are in separate commits, for ease of review / cherry-picking.

I implemented this before I saw #57, so feel free to close this if that gets merged. I'm mainly creating this PR in case my implementation ends up being helpful; even if this impl isn't used, the test cases are a bit more extensive and could be merged into #57.

[Aeledfyr]

Other minor things to note:

• This adds two semi-unrelated TODOs regarding index and call chains (triggering errors rather than hanging on {}; setmetatable(t, { __index = t }); t.a and {}; setmetatable(t, { __call = t }); t())
• This removes raw_ops::{add, mul, div}, but leaves in raw_ops::sub -- the integer/number-only subtract is used in numeric for-loops, and is required to only support integers/floats. That seems like it should be handled a bit more explicitly, but that's out of scope for this PR.

[kyren]

I like this PR a lot, thank you!

I'm still reviewing it but mostly what I see so far is just very minor nits.

[kyren]

I'm mainly creating this PR in case my implementation ends up being helpful; even if this impl isn't used, the test cases are a bit more extensive and could be merged into https://github.com/kyren/piccolo/pull/57.

I actually think I prefer this PR over #57

[kyren]

Also, since I was just complaining about the error situation, I should add that the new error messages are great! I'm definitely not asking you to get rid of the new (much better) meta ops errors.

If you want to change VMError and track down all the implications of this, then that's fine, but that's not necessary at all, and I can instead change it after merging (or it could be a separate PR).

I'm interested if you have any thoughts though about what the final error stack should look like. I see now why MetaOperatorError is the way it is: because several meta ops may return MetaOperatorError::Unary / MetaOperatorError::Binary or MetaOperatorError::Call, so a combined error type makes sense to exist. Should MetaCallError be folded into that, since MetaOperator::Call(MetaMethod::Call, ...) can be constructed? Should it stay separate?

[Aeledfyr]

MetaCallError was extracted out of MetaOperatorError to avoid making MetaOperatorError a recursive struct -- since the nesting depth is always bounded to 1, it's nice to reflect that in the type. (It also avoids the need for boxing the inner error.) I'll see how straightforward cleaning up the VMError cases, but a larger changes to the VM error system should probably be done separately.

I'm not sure on the overall error hierarchy, but explicit error types would be nice; the tests that try to downcast an anyhow errors to specific error types end up being very fragile, since they still fail at runtime if the error tree structure changes. One question is whether the error types should implement Copy; a number of the current types do, but that limits what can be reasonably put in them.

On that topic, though, I experimented with a very WIP patch to add backtraces to errors for debugging: https://github.com/Aeledfyr/piccolo/commit/99c1cb43a875653f59b09d6aaddf244a9ff6576e

To handle backtraces cleanly, (specifically preserving backtrace info when passing the error through a Sequence), the error would have to expose the backtrace information in some form, but it would also need to allow the sequence to modify the error, like adding context, create a new error linked to the previous one, etc. Another quirk I noticed, more on backtraces than on errors, is that a lot of the Rust to Lua calls will be tail calls, and thus won't show up in the backtrace. This is good for efficient execution, but it could make stack traces more confusing.

[kyren]

MetaCallError was extracted out of MetaOperatorError to avoid making MetaOperatorError a recursive struct -- since the nesting depth is always bounded to 1, it's nice to reflect that in the type.

I think we're talking past each other because I wasn't suggesting anything recursive, I was suggesting making a variant like:

pub enum MetaOperatorError {
    #[error("could not call metamethod {}: could not call a {} value", .0.name(), .1)]
    Call(MetaMethod, &'static str),
    ...
}

As in just flattening the MetaOperatorError and getting rid of MetaCallError.

To handle backtraces cleanly, (specifically preserving backtrace info when passing the error through a Sequence), the error would have to expose the backtrace information in some form, but it would also need to allow the sequence to modify the error, like adding context, create a new error linked to the previous one, etc. Another quirk I noticed, more on backtraces than on errors, is that a lot of the Rust to Lua calls will be tail calls, and thus won't show up in the backtrace. This is good for efficient execution, but it could make stack traces more confusing.

I know this already because it's what I was working on next, backtraces and doing the redesign of the Error system to accommodate backtraces in exactly the way you described.

I know that tail calls will mess up stacktraces, but I care way more about tail calls than good stacktraces, if the two were completely in conflict. It's not even a matter of "efficiency" because most of the time it's a matter of something working vs not working at all. Really good tail calls and proper symmetric coroutines should remain a part of piccolo.

[Aeledfyr]

Flattening MetaCallError into MetaOperatorError works, but it makes the implementation of the other metaoperators messy; take len, for example:

let len = metatable.get(ctx, MetaMethod::Len);
if !len.is_nil() {
    return Ok(MetaResult::Call(MetaCall {
        function: call(ctx, len)
            .map_err(|e| MetaOperatorError::Call(MetaMethod::Len, e))?,
        args: [v],
    }));
}

Since call currently returns a MetaCallError, we can just wrap it in a MetaOperatorError with the correct MetaMethod, and we get the full error.

If call instead returned a MetaOperatorError:

• Since it's an error of the same type, we could just return it; however, that would discard the information about which metamethod was originally being called being called.
• We could discard the value and reconstruct the type info in len, but that would only let us determine the outer type, and wouldn't match if the error was from whatever was in that object's call metatable entry.
• We could explicitly inspect the error and then construct the inlined MetaOperatorError::Call with the right type name, but that requires matching on the MetaOperatorError variants, even though call will never construct most of them. (It could assume that it will return Unary, but then what's the point of returning the enum?)

Basically, the those errors are primarily structured to make calling call from other meta operators clean. If the goal is a unified error type on the external interface, it could make sense to have call be a wrapper around an internal function that returns an error directly, but then have the internal interface expose it wrapped in a MetaOperatorError?

I'm not sure what the best solution is; I'm fine with changing it, I just don't see a clean solution besides separating out the MetaCall error, or having fully nested MetaOperatorErrors (such that len could just wrap the error from call in another MetaOperatorError).

[kyren]

Basically, the those errors are primarily structured to make calling call from other meta operators clean. If the goal is a unified error type on the external interface, it could make sense to have call be a wrapper around an internal function that returns an error directly, but then have the internal interface expose it wrapped in a MetaOperatorError?

Okay, that makes sense now! I hadn't thought through how re-wrapping the call error was irritating, and you're right, it is.

I don't have much of an agenda here, I was trying to work backwards from VMError having a MetaCallError variant but not the other kind of meta op error, and there being two ways for a MetaCallError to bubble up through run_vm. I'm actually fine with the second point now that I understand it, it's not that big of a deal.

In short, it's fine the way it is now, I was trying to ask to make sure it wasn't the way it is now only to support the incidental current design of run_vm and LuaFrame.

[Aeledfyr]

I believe that commit should fix most of the remaining weird error types in run_vm, or at least centralize them into VMError so that we can fix them later (in the case of ConcatError).

[kyren]

Yeah, everything looks good now, I think.

[kyren]

Full disclosure, I may soften the wording of the comments around infinite looping in __index. After thinking more about it, there are many ways to infinite loop in Lua and the (near term at least) future of piccolo will always be to do the most obvious possible thing and avoid any magic limits where at all possible, so adding checks here is not really my goal.

[Aeledfyr]

Yeah, that's reasonable. Thank you for merging!

[kyren]

NP, thank you for the excellent PR!