Improve "key not found" error messages for string/symbol keys

[d-torrance]

We don't know how to represent arbitrary objects as strings at this low level, but this takes care of two very common cases.

Before

i1 : x = hashTable{};

i2 : x#"foo"
stdio:2:2:(3): error: key not found in hash table

i3 : x#bar
stdio:3:2:(3): error: key not found in hash table

After

i1 : x = hashTable{};

i2 : x#"foo"
stdio:2:1:(3): error: key not found in hash table: "foo"

i3 : x#bar
stdio:3:1:(3): error: key not found in hash table: bar

[pzinn]

I already mentioned in another thread that I advocate (and have already implemented, in fact) printing error messages at the m2 level. This is a typical situation where it can improve error messages, cf this screenshot:

[d-torrance]

Oh that's awesome!

[mahrud]

Several comments:

1. I don't have any particular opinions on where to printing errors, but I prefer whatever is more straightforward in a given situation. I am not sure if Paul's "detect errors in interpreter, print in top-level" rule applies everywhere.
2. I do prefer printing the missing key after the message "key not found in hash table", because the key can be very long or many lines or whatever and that would make it hard to read the error message.
3. I think we have to be careful about printing objects that take a long time to print in the error. e.g. if the missing key was a giant module, there are cases when (due to bugs) printing would require a gb computation! One approach would be to use the stuff in robust.m2 to print with a time limit. In particular, we need to print "key not found in hash table" first, flush it, then try to print the key, so that if that fails at least the message is there.
4. If we go with the robust.m2 route, I really want to make sure the debugger doesn't end up in robust.m2 or debugging.m2 if there was an error while printing the error (e.g. if printing the error took too long and the user interrupted it). That's the worst ...

[pzinn]

well, my PR is going to take a while to come, because there are problems with my approach, as you have pointed out:

1. what if the error message is too slow to print?
2. what if the error message is too long?
3. what if the error message printing causes an error?
   1 & 2 can to some extent be resolved using robust.m2 type coding, but I'm not very happy with that stuff and I want to rewrite it better. so, still a lot of work to do.
   3 I find the most problematic issue. note that in a way we already have this issue when writing error ("argument "|toString argument|" invalid") or whatever: we trust toString to not cause an error at this stage. What I'm suggesting only transfers the issue to later, i.e., now one write something like error ("argument ",argument," invalid") and it's the printing routine that deals with the potential problems with output routines.

Though I have a more or less working implementation of my ideas (which is currently tested in Macaulay2Web), I'm not going to rush to submit a PR.

[mahrud]

Sounds good. When you do make the PR, I would appreciate it if it was broken up into multiple commits each with a specific objective.

[mahrud]

Okay I actually thought about this and I think I know how to call a robust printing method from interpreter. Can I push here or should I make a new PR?

[mahrud]

I'll push this for now, but feel free to revert of course. Here's the proposed output:

i1 :  h#123 
currentString:1:2:(3):[4]: error: key not found in hash table: 123 (of class ZZ)

i1 :  h#(id_(ZZ^2)) 
currentString:1:2:(3):[4]: error: key not found in hash table:
        | 1 0 | (of class Matrix)
        | 0 1 |

i1 :  h#blah 
currentString:1:2:(3):[4]: error: key not found in hash table: blah (of class Symbol)

i1 :  h#"blah\"foo" 
currentString:1:2:(3):[4]: error: key not found in hash table:
        "blah\"foo" (of class String)

(Whether it goes on a new line or not depends on length/depth/height)

Paul can edit the RobustPrintMethod to return the tex or html output in web mode.

I can't think of an example that breaks the time limit, but if you can think of one let me know.

[mahrud]

Oh this actually works beautifully even in the event of slow printing or errors in printing:

i1 : h = new HashTable from {};

i2 : T = new Type of List

o2 = T

o2 : Type

i3 : t = new T from {"test", "me"};

i4 : elapsedTime h#t -- should instantly print "{test, me} (of class T)"
stdio:4:13:(3): error: key not found in hash table: {test, me} (of class T)
 -- 0.000618107 seconds elapsed

i5 : net T := t -> error "can't print me"

o5 = FunctionClosure[stdio:5:9-5:36]

o5 : FunctionClosure

i6 : elapsedTime h#t -- should instantly print "new T from {"test","me"} (of class T)"
stdio:6:13:(3): error: key not found in hash table:
        new T from {"test","me"} (of class T)
 -- 0.000275408 seconds elapsed

i7 : net T := t -> horizontalJoin("1", (sleep 1; toString(#t)), "3")

o7 = FunctionClosure[stdio:7:9-7:63]

o7 : FunctionClosure

i8 : elapsedTime h#t -- should print after 1s "124 (of class T)"
stdio:8:13:(3): error: key not found in hash table: 123 (of class T)
 -- 1.0007 seconds elapsed

i9 : net T := t -> horizontalJoin("1", (sleep 5; toString(#t)), "3")

o9 = FunctionClosure[stdio:9:9-9:63]

o9 : FunctionClosure

i10 : elapsedTime h#t -- should print after 3s "new T from {"test","me"} (of class T)"
stdio:10:13:(3): error: key not found in hash table:
        new T from {"test","me"} (of class T)
 -- 3.00068 seconds elapsed

[d-torrance]

Looks good to me!

because I'm now a contributor also.

[mahrud]

Do you think the extra lines of the output should be commentized or not? We're not very consistent:

i1 : A + B
stdio:1:3:(3): error: no method for binary operator + applied to objects:
--            A (of class Symbol)
--      +     B (of class Symbol)

i2 : sum(A, B)
stdio:2:1:(3): error: no method found for applying sum to:
     argument 1 :  A (of class Symbol)
     argument 2 :  B (of class Symbol)

[d-torrance]

I'm leaning towards not commenting them so that they can be syntax highlighted.

[mahrud]

For consistency, I decided it's simpler to always print the missing key in a new line, and also to remove the comments from missing operator method errors. (e.g. A+B).

[pzinn]

yes this looks similar to my changes, except I have a mode-dependent behaviour (similar to Print, AfterPrint, etc), and also it applies to all error messages, obviously. will review more carefully now.

[pzinn]

by now the only failure would be something like:

i1 : h=hashTable{};

i2 : a=new Type of BasicList;

i3 : net a := toString a := toExternalString a := x->1/0

o3 = FunctionClosure[stdio:3:45-3:51]

o3 : FunctionClosure

i4 : h#(new a)
stdio:3:49:(3):[5]: error: division by zero
stdio:4:1:(3): error: key not found in hash table

which admittedly is pretty rare.

[mahrud]

yes this looks similar to my changes, except I have a mode-dependent behaviour (similar to Print, AfterPrint, etc), and also it applies to all error messages, obviously. will review more carefully now.

This was the motivation for the name RobustPrintMethod (I think the others should probably be AfterPrintMethod etc as well). I assumed you've made silentRobustNetWithClass to be mode-dependent, which would also make this mode-dependent, so I'll leave that part to you.

Regarding the error in toString, I wasn't sure what's best: skip printing the key or still print some indication of the error. I will push a commit that skips it now, but we can always go back. The question is what's more useful.

[pzinn]

in principle there are two different places where to make code mode-dependent:

• at the d level, as is the case of Print, AfterPrint, etc.
• at the m2 level, which I think is what you're suggesting for RobustPrintMethod.

I had indeed done the latter for [my analogue of] RobustPrintMethod, but I'm a little annoyed by the inconsistency -- in fact, why do we ever need d level mode-dependent branching?

Relatedly, I also experimented with a new type of Function (at the m2 level), something like ModeDependentFunction, which automatically takes care of running the correct function depending on the mode. Could be useful for Print, AfterPrint, etc, but also for m2-level functions like print (lowercase!), show, edit, etc.

[mahrud]

in fact, why do we ever need d level mode-dependent branching?

Say we wanted to use xterm-256color in terminal output, then doing it at the interpreter would make a lot more sense, both for speed of character setting and because that's where printing really happens.

But in principle I agree with you that if it can be in top-level there's no reason to force it in the interpreter.