Bug - Fixed ASH Parser errors

Veracity

Developer
Staff member
This ash script:

Code:
record rec
{
    int x;
};

rec r = new rec[1);

is obviously erroneous: there is a "[" instead of a "(".

ASH's error checking could be a bit more robust, though:

Code:
> badrec.ash

Variable 'null' cannot be indexed (badrec.ash, line 6, char 9 to char 17)

In fact, this script:

Code:
record rec
{
    int x;
    int y;
};

rec[int] map1;
rec[int] map2;

foreach i, r in map1 {
    map2[i] = new rec[r.y, r.x);
}

which has what looks like the same error is even less robust:

Code:
> badrec2

Unexpected error, debug log printed.

with this stack trace:

Code:
Unexpected error, debug log printed.
class net.sourceforge.kolmafia.textui.ScriptException: Internal error: key is not a Value
net.sourceforge.kolmafia.textui.ScriptException: Internal error: key is not a Value
    at net.sourceforge.kolmafia.textui.parsetree.RecordType.getDataType(RecordType.java:80)
    at net.sourceforge.kolmafia.textui.parsetree.CompositeReference.getType(CompositeReference.java:43)
    at net.sourceforge.kolmafia.textui.Parser.autoCoerceValue(Parser.java:1042)
    at net.sourceforge.kolmafia.textui.Parser.parseAssignment(Parser.java:3364)
    at net.sourceforge.kolmafia.textui.Parser.parseEvaluable(Parser.java:3779)
    at net.sourceforge.kolmafia.textui.Parser.parseCommand(Parser.java:1244)
    at net.sourceforge.kolmafia.textui.Parser.parseScope(Parser.java:558)
    at net.sourceforge.kolmafia.textui.Parser.parseLoopScope(Parser.java:2935)
    at net.sourceforge.kolmafia.textui.Parser.parseLoopScope(Parser.java:2920)
    at net.sourceforge.kolmafia.textui.Parser.parseForeach(Parser.java:2575)
    at net.sourceforge.kolmafia.textui.Parser.parseCommand(Parser.java:1210)
    at net.sourceforge.kolmafia.textui.Parser.parseScope(Parser.java:558)
    at net.sourceforge.kolmafia.textui.Parser.parseFile(Parser.java:480)
   ...
 

heeheehee

Developer
Staff member
This is tied to `new rec` being shorthand for `new rec()`. The message is not great, of course (variable "null" being the name because it's not yet bound to a variable, so it doesn't have a name).

The second one is obviously bad because debug-log-generating crashes are bad. You can reproduce with the simpler
Code:
record rec {
  int x;
};
rec a;
rec b = new rec[a.x];

which looks less clearly wrong. (The syntax still doesn't make any sense for ASH, although those familiar with Java might expect this to be a new rec[0] array.)
 

heeheehee

Developer
Staff member
The weird part is that in your second case (as well as mine), the rhs leads to a successful expression parse. You'll note that the stack trace in the debug log points to autoCoerceValue, as opposed to somewhere in parseExpression.
 

heeheehee

Developer
Staff member
This looks related to multi-error-diagnostic handling being incomplete. We're recognizing an error in parseVariableReference (since we're trying to index into a non-aggregate), but then trying to continue the parse. I've started https://github.com/kolmafia/kolmafia/pull/806 which addresses the immediate crash (and helps a little with observability), but t.here are still several other deficiencies as noted.

I might poke at this more later, but otherwise, feel free to massage that PR as desired.
 

Veracity

Developer
Staff member
I've worked my way through the other projects I had queued up and am ready to take a look at your PR now.
Thanks!
 

Veracity

Developer
Staff member
I found the root cause. I love your fix to display multiple errors. I removed the exception catching, made it not throw an error, made a test script showing four compile errors, and committed the fix and the test to your PR branch.
 

heeheehee

Developer
Staff member
Thanks! I've been super busy this past couple of weeks but might be able to poke around some more this weekend.

Quoting one of your comments from the PR:

Veracity said:
I wish that "new rec" displayed as "anonymous record" rather than "Variable 'null'".
I may look further at that.

I tried tinkering with that earlier but wasn't particularly happy with the result.

My approach was roughly: we have the location of the error. Parser.currentLine is a node in a doubly-linked list. Walk backwards to find the error, then build up the substring that corresponds to the relevant tokens. My ideal error message is something like Unnamed variable (new rec) ... (line A char B to line C char D.

The VariableReference probably knows its own Location. I don't recall having good results with that, though.

(I also left a comment in the PR for more exposition regarding some questions you had.)
 

heeheehee

Developer
Staff member
I've updated the error messages to now read
Code:
'new rec' cannot be indexed (bad_record_indexing.ash, line 10, char 9 to char 17)
...
Too many keys for '{1: new rec(1)}' (bad_record_indexing.ash, line 13, char 15 to char 34)
Too many keys for '{ ... }' (bad_record_indexing.ash, line 14, char 15 to line 16, char 6)

Because of an implementation detail, we don't quote the aggregate literal type (the type isn't part of the AggregateLiteral's location). We won't always have that type, though (e.g. if deduced from a function call).
 

heeheehee

Developer
Staff member
Anyways, from my perspective, with the additional context, this is good enough to merge. We could also change the error message to be more verbose, e.g. 'new rec' cannot be indexed because it isn't an aggregate, but I'm not sure how much that adds (or if it's just clutter).
 

Veracity

Developer
Staff member
I think we can assume the the user will know that only aggregates are indexable.
Perhaps they had a misunderstanding about what ASH will let them do - and this points it out for them.
Or perhaps they simply had an error. As in my initial discovery of this issue. :)

This looks good. Thanks!
 
Top