Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message mechanism #18

Open
masak opened this issue Nov 28, 2019 · 10 comments
Open

Improve error message mechanism #18

masak opened this issue Nov 28, 2019 · 10 comments

Comments

@masak
Copy link
Owner

masak commented Nov 28, 2019

Right now when something goes wrong with a function call, the error symbol 'cannot-apply is emitted. Although this is correct by spec, I'd like us to do better.

I'd like to have the following error message:

  • The source code that went wrong
  • An arrow or some kind of marker pointing to the operator that couldn't be applied
  • Line and column (hm, maybe only important when it's not the REPL)
  • Some more detailed explanation in polite, empathic English
  • A suggestion of what to do to make things right

Ideally this should go for all error messages emitted by spec, but let's start here.

@masak
Copy link
Owner Author

masak commented May 24, 2020

Concretely, here is what I would like to see:

Right now, when you call srrecip and disobey the type protocol, you get this:

> (srrecip '(+ nil (t t)))
'mistype

I would like to see something like this instead:

> (srrecip '(+ nil (t t)))
'mistype

In the following function call, the `n` parameter did not satisfy its type
predicate:

                   '+   nil          '(t t)
                   |    |            |
                   v    v            v
    (def srrecip ((s (t n [~= _ i0]) d))
      (list s d n))

The type predicate is: [~= _ i0]    (`i0` has the value `nil`)
The type checker reduced the predicate to: "any value except `nil`"

But the value passed in to `n` was: nil
In order to make the call work, `n` needs to bind to a value which is not `nil`.

The value `nil` was passed to `srrecip` from the REPL:

                   here
                   |
                   v
    > (srrecip '(+ nil (t t)))

I think this checks all the boxes from the checklist in OP. (Except line and column, which is more important for source files. We might want to have it if the REPL script grows to be several lines as well.)

@masak
Copy link
Owner Author

masak commented Oct 27, 2020

  • I think this one would be good to do sooner rather than later. Not having good error messages is climbing up towards the top of the most annoying things about this implementation at this point. (Maybe in a tie with "it's still way too slow".)

  • Doing them all at once would be counterproductive. Let's start with one. Maybe the mistype one above. Also overargs and underargs are good candidates. Also cannot-apply as mentioned in OP. Oh, and car-on-atom and cdr-on-atom! These are the ones that happen to me the most often.

  • I had a case that happened to me when I started to write the compiler. I'll dig it up.

  • It would absolutely be a good idea to write the documentation for each error at the same time as writing the error text for the error.

  • Some errors are quite specialized, and will basically only happen if you willfully abuse the interpreter. We can do those last.

@masak
Copy link
Owner Author

masak commented Apr 16, 2021

Just stumbled over this excellent blog post, which talks very lucidly about different types of errors/warnings/linting warnings, and their fixes. I really liked the terminology of symptom → diagnosis → treatment plan... and I think that's definitely something to keep in mind when working on error messages for Bel.

@masak
Copy link
Owner Author

masak commented Jun 28, 2021

Sanctuary's run-time type checking provides another nice source of inspiration; one that is perhaps a little bit closer in genealogy than Elm.

@masak
Copy link
Owner Author

masak commented Jul 16, 2022

I just realized that there are two ways the source code to show in an error message might not exist:

  • "A wizard did it." No, I mean, the function where the error occurred wasn't defined in any source code, but created programmatically somehow. That certainly wouldn't be common, but it's legal.

  • Macros. Think about it — even let is a macro! Just as do, and, case, when, and all the loop constructs. In some sense, most Bel code in the source file won't be the code that runs (and gets errors) — the code that runs is the one generated by the macro.

@masak
Copy link
Owner Author

masak commented Jul 28, 2022

Just had another fun one. The error report was

Error: mistype

and the actual problem was that I had written (set (board 4 4 \.)) when I meant (set (board 4 4) \.). board here is a 2D array, whose shape is statically knowable.

The flow of that code is going to end up here, which invokes a virtual function here, which fails with the observed type error here (because n is bound to \. which is not a pint). But all of this could be discovered up-front, statically, given that we know board is an array.

@masak
Copy link
Owner Author

masak commented Sep 7, 2022

Another good one. The following code:

(mac each/i (kv expr . body)
  (let (k v) kv
    `(let ,k 0
       (each ,v ,expr
         (++ ,k)
         ,@body))))

(mac each/bi (kv expr . body)
  (let (k v) kv
    `(catch
       (bind break (fn () (throw 'break))
         (each/i ,kv ,expr ,@body)))))

(each/bi row (array-elements A)
  (when (all is.0 (array-elements row))
    (pr "The first all-zero row is " row-number \lf)
    (break)))

Gives this error message:

Error: atom-arg

Because the each/bi macro actually expects a list of two variables to bind (such as (row-number row)), but instead row was provided.

Of course a remotely decent error message would (a) pinpoint the actual cause to be the row variable, and (b) explain better what went wrong.

This is a "static" error, in the sense that we don't need to run code in order to catch it.

@masak
Copy link
Owner Author

masak commented Oct 10, 2022

  • Macros. Think about it — even let is a macro! Just as do, and, case, when, and all the loop constructs. In some sense, most Bel code in the source file won't be the code that runs (and gets errors) — the code that runs is the one generated by the macro.

The when macro is very straightforward — let's call it a "simple polynomial macro", because it only recombines the arguments using a quasiquote.

do and and use a reduce, and case uses a recursive (macro) call. I think running that code is somehow "provably harmless" (based on the fact that the input is finite, and that both the reduce and the recursion guarantee that we iterate on a smaller subproblem — furthermore, this feature is statically evident).

A similar thing (totality, maybe even bounded work in some sense) seems to be true of most macros in bel.bel; maybe all of them. (Scanning through them quickly, the only one I'm not sure about is bquote.)

We can run the macro expansion through an interpreter, one which is instrumented to connect the parts of the generated code that were generated from the macro argument, back to the actual location in those arguments.

@masak
Copy link
Owner Author

masak commented Nov 22, 2023

  • "A wizard did it." No, I mean, the function where the error occurred wasn't defined in any source code, but created programmatically somehow. That certainly wouldn't be common, but it's legal.

I think we (or at least I, but probably all of us) have a blind spot for how bad this one is. It's not common, but it's bad. To put it into words, yes, getting information about where in the source code the error comes from is of course great, some would argue practically necessary. But the assumption is that it comes from somewhere in the source code. In a language where runnable artifacts can be created dynamically at runtime, this simply does not always hold true.

I think when people say "code is data", they might mean any number of things. Here are three possible meanings:

  • The category of things that are code is a subset of the category of things that are data.
  • The thing you think is code is in actual fact merely data.
  • Code has all the aspects and behaviors of data (and then possibly more).

I can use my list-builder library to illustrate my point, by creating a bit of executable code that has no corresponding source code:

> (set double (build
                (link 'lit)
                (link 'clo)
                (link nil)
                (link '(n))
                (link '(* n 2))))
(lit clo nil (n) (* n 2))
> (double 5)
10

The code runs and works. But if I change the n in the body to a k:

> (set double (build
                (link 'lit)
                (link 'clo)
                (link nil)
                (link '(n))
                (link '(* k 2))))
(lit clo nil (n) (* k 2))
> (double 5)
Error: ('unboundb k)

There isn't really any source location we can put this unboundb error on. Probably the best we can do is to show the error in a "synthetic" representation of the generated code, and possibly show where this code was assigned to the double variable.

@masak
Copy link
Owner Author

masak commented Jan 5, 2024

It just struck me, as I got an overargs error (very frustrating) that, at least in this particular case, the error had two sites where the blame ought to be pointed out:

  • The call in reduce that goes (f (car xs) (reduce f (cdr xs))) (f being the first parameter, a function)
  • My first argument to reduce, which was [list 'app _] (a one-argument function)

These two do not mesh. A sufficiently deep static analysis would be able to notice as much without even running the program, essentially by abstract interpretation. Therefore, at least a warning could be shown in the IDE (on the [list 'app _] argument, surely).

A sufficiently fervent inliner would be able to replace the call to reduce by a specialized bit of code where the entire call (f (car xs) (reduce f (cdr xs))) was replaced by (err 'overargs), perhaps with all the blame details sent on a side channel somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
v1.0 milestone
  
Up Next
Development

No branches or pull requests

1 participant