Help wanted writing I6 library tests

vlaviano · May 7, 2016, 2:45am

I’ve finished looking at inform6lib issue 38.

We discovered this issue while investigating issue 30. Command sequences such as “TAKE ROCKS. TAKE ALL. TAKE ROCKS.” and “TAKE ALL FROM GIRL” produce bad output when given to the test program for issue 30.

This issue is actually several intertwined issues. It seems simple, but a 5 whys style analysis leads us deep into the parser and 13 years back through its history.

(The following resources were useful in researching the history of the parser code:

rec.arts.int-fiction archives (google groups, ifarchive),
inform-maintenance mailing list archives,
inform 6 library patches page,
inform6 suggestions list,
inform6lib git history,
inform-2006 git history, incorporating history from the earlier inform cvs repo,
inform 6 release notes, covering lib 6/11,
the DM4, particularly §28 - §33,
and, last but not least, these forums.)

Let’s begin with the simplest issue. In the output above, we see a runtime error. (Later, we’ll investigate why the girl is involved at all when we ask to take a pile of rocks from the ground.)

This response is generated by a call to the L__M library messages function. This function takes an action, a message number (specific to the action), and two optional args, x1 and x2, that usually refer to the action’s direct and indirect objects, and it prints a message based on these parameters. Specifically, the response is generated by L__M(##Take, 6, …), message number 6 for the ##Take action, which is printed when an actor tries to take something held by a creature.

In lib 6/11, L__M(##Take, 6, …) called this code:

  Take: switch (n) {
        ...
        6:  if (noun has pluralname) print "Those seem "; else print "That seems ";
            "to belong to ", (the) x1, ".";
        ...
    }

lib 6/12 adds voices and tenses, and its library messages were updated to incorporate this feature:

  Take: switch (n) {
        ...
        6:  CSubjectVerb(x2,true,false,"seem",0,"seems","seemed"); " to belong to ", (the) x1, ".";
        ...
    }

We can see that the 6/12 version takes two explicit args, x2 (the item being taken) and x1 (the creature carrying it), and prints the short name of x2 as the subject of its msg. CSubjectVerb applies the correct voice and tense to the subject x2 and the verb “to seem”. In contrast, 6/11’s version of this msg took only one arg, x1 (the creature), interpreted noun as the item being taken, and selected “That” or “Those” as the subject of the msg depending on the pluralname attribute of noun.

The runtime error tells us that this call to L__M(##Take, 6, …) is testing an attribute of nothing. Why? Parser section I, the part of the parser that prints error messages, is calling L__M(##Take, 6, …) with only one arg instead of the two that are required:

            if (noun has animate) L__M(##Take, 6, noun);

This causes x2 to be nothing. This value is passed to CSubjectVerb and, in turn, to SubjectNotPlayer (as obj), where we execute:

        if (obj has pluralname) { print (The) obj, " ", (string) v2; return;}
        else                    { print (The) obj, " ", (string) v3; return;}

and attempt to test the pluralname attribute of nothing.

Taking a step back, what’s going on with “TAKE ROCKS. TAKE ALL. TAKE ROCKS.”? Why are we in parser section I printing an error msg, and why is the girl involved?

The ‘take’ grammar has the following four syntax lines:

Verb 'take' 'carry' 'hold'
    * multi                                     -> Take
    * 'off' held                                -> Disrobe
    * multiinside 'from'/'off' noun             -> Remove
    * 'inventory'                               -> Inv;

What happens during parsing of our final TAKE ROCKS? The parser tries to match the input with each of the 4 grammar lines in turn.

Line 0: the multi token matches the rocks, but the indef plural code in Adjudicate rejects the rocks because the actor is already holding them, leaving us with a multiple object of size 0. See the post earlier in this thread about issue 30 for much more on this. Note that the ChooseObjects solution described there will work here also: the rocks will be accepted, we’ll have a nonempty multiple object, this first grammar line will match, and all will be fine. But let’s assume, for the sake of finding the root cause of this issue, that we don’t do that. The parser calls ReviseMulti and, because the size of the multiple object is 0, it sets etype (the parser error type) to NOTHING_PE, sets results–>0 to action_to_be, and moves on to the next grammar line.

Line 1: we fail to match the word ‘off’ in the input after ‘take’, resulting in STUCK_PE.

line 2: we see the multiinside token, and parser section F attempts to perform lookahead to answer the question “inside what?” before trying to match it. There’s not enough input, so the parser is forced to abort the lookahead and continue without this info. The matching for multiinside proceeds as with multi on line 0 above: we match the rocks, but Adjudicate rejects them, because the action_to_be is ##Remove and the actor is already holding them. However, unlike with line 0, we run out of input before finishing the grammar line. So, the parser infers (FROM ) and looks for a likely object to match its inferred noun token. If we hadn’t taken the stone as part of our previous TAKE ALL command, the indirect object would have been ambiguous and we’d fail to choose one. However, since we’re holding the stone, the girl is considered the best single possibility, and we infer (FROM GIRL) and set results–>3 = girl. After all of this, ReviseMulti is called and, just as with line 0, because the multiple object for the multiinside token is empty, etype is set to PE_NOTHING and the line fails.

Line 3: we fail to match the word ‘inventory’ in the input after ‘take’, resulting in STUCK_PE.

Now that the parser has failed to match every single grammar line, it needs to produce an error message, but we’ve seen that different grammar lines produced different error types. The parser produces “the best” error message across all lines. To do this, it maintains several variables: etype (parser error for the current token), line_etype (best etype for the current line), and best_etype (best etype across all lines so far). We’ll discuss line_etype more later. For now, note that best_etype is set based on which error is “the most specific”. Parser errors are numbered in increasing order of specificity, and a higher number trumps a lower number:

Constant STUCK_PE     = 1;
Constant UPTO_PE      = 2;
Constant NUMBER_PE    = 3;
Constant CANTSEE_PE   = 4;
Constant TOOLIT_PE    = 5;
Constant NOTHELD_PE   = 6;
Constant MULTI_PE     = 7;
Constant MMULTI_PE    = 8;
Constant VAGUE_PE     = 9;
Constant EXCEPT_PE    = 10;
Constant ANIMA_PE     = 11;
Constant VERB_PE      = 12;
Constant SCENERY_PE   = 13;
Constant ITGONE_PE    = 14;
Constant JUNKAFTER_PE = 15;
Constant TOOFEW_PE    = 16;
Constant NOTHING_PE   = 17;
Constant ASKSCOPE_PE  = 18;

After attempting to match all grammar lines, the parser sets etype = best_etype and enters section I to print an appropriate error message. In our case, both line 0 and line 2 produced NOTHING_PE, one of the most specific error types that trumps almost everything. Also recall that a ReviseMulti failure for the direct object (results–>2) sets results–>0 to action_to_be. Our most recent ReviseMulti failure was during parsing line 2, where action_to_be was ##Remove. Finally, recall that inferring the girl as the indirect object in line 2 caused results–>3 to be set to the girl and nothing during our abortive attempt to match line 3 (‘take’ ‘inventory’) caused results–>3 to be overwritten.

All of which leads us to this code in parser section I:

    if (etype == NOTHING_PE) {
        if (results-->0 == ##Remove && results-->3 ofclass Object) {
            noun = results-->3; ! ensure valid for messages
            if (noun has animate) L__M(##Take, 6, noun);
            else if (noun hasnt container or supporter) L__M(##Insert, 2, noun);
            else if (noun has container && noun hasnt open) L__M(##Take, 9, noun);
            else if (children(noun)==0) L__M(##Search, 6, noun);
            else results-->0 = 0;
        }
        if (results-->0 ~= ##Remove) {
            if (multi_wanted == 100)    L__M(##Miscellany, 43);
            else {
                #Ifdef NO_TAKE_ALL;
                if (take_all_rule == 2) L__M(##Miscellany, 59);
                else                    L__M(##Miscellany, 44, verb_word);
                #Ifnot;
                L__M(##Miscellany, 44, verb_word);
                #Endif; ! NO_TAKE_ALL
            }
        }
    }

For the reasons mentioned above, we pass the first two tests and wind up in the top block of code.

results–>3 (the girl) is placed in noun. The comment “ensure valid for messages” is because lib 6/11’s version of L__M(##Take, 6, …) implicitly uses the value of noun. Note that noun and second are usually not set until after the parser returns its results to the main loop in InformLibrary::play, and the ##Take 6 msg was designed to be printed from action routines in verblibm.h.

The code then produces specialized L__M messages based on the nature of the indirect obj now stored in noun. The 4 scenarios are: the indirect object is animate, the indirect object is neither a container nor a supporter, the indirect object is a closed container, or the indirect object is empty. If none of these are true, we clear results–>0 so that we’ll pass the test for results–>0 ~= ##Remove just below and print out a more generic L__M(##Miscellany, …) msg.

In our particular case, noun is the girl, who is animate, so we call L__M(##Take, 6, noun). The ##Take 6 msg requires x1 and x2, but we only pass x1, so x2 is nothing and we get the runtime error as described earlier. This is our superficial root cause, our first why.

It should be easy to fix: just pass the direct object also. The problem is that the direct object is a size 0 multiple object, so results–>2 is 0 (nothing). This is hardly surprising, since we’re in a clause for printing a parser error when the best etype is NOTHING_PE. We’re forced to conclude that, even if we were to pass results–>2 as x2, we’d see the same runtime error. (We’re glossing over another issue here: best_etype isn’t necessarily from the same grammar line that most recently set results–>2, which isn’t necessarily from the same grammar line that most recently set results–>3. More on that later.)

The possible solutions appear to be: change ##Take 6 to behave differently if x2 is nothing, or change the L__M call site in parser section I when x2 is nothing.

The message “ seems to belong to the girl” doesn’t make sense unless there’s a specific something belonging to the girl that the actor is trying to take. Also, there are two other call sites with valid direct objects that are passed as x2. So, it seems better to leave the msg code alone and change this specific call site. We could test for results–>2 == nothing, but, as mentioned, we’d always expect that to be true since etype is NOTHING_PE and results–>3 is ofclass Object. So, we need to do something different unconditionally.

The easiest and least disruptive solution is to change the message back to the default ##Miscellany 44 one (“There is nothing to take.”), which is what would have been printed in this scenario prior to this ##Remove-specific code being introduced. This eliminates mention of the girl, which is good, because the parser only prints inferences like “(from the girl)” after a grammar line succeeds. The reference to the girl when the grammar line has failed and no inference msg has been printed can be confusing for the player.

The fix (I’ve submitted a pull request for this):

            if (noun has animate) L__M(##Miscellany, 44, verb_word);

With this fix in place, we see the following output with our test program:

We could end this analysis here, but let’s keep asking questions. (Spoiler: I’m not going to propose any more code changes for this issue in 6.12.1.)

Why is this special case code for ##Remove even here? What about these other L__M calls? Do we need to fix them, too? All of them pass the correct args, so they don’t create runtime errors like ##Take 6 did. However, all of them represent situations where an actor is trying to remove nothing from something and, instead of saying “there’s nothing to take/remove”, we’re printing messages describing what’s wrong with the indirect object from which the actor is trying to remove the nothing. Why would we do that? What’s wrong with ##Miscellany 44?

This special case code appeared in 6/11. The 6/10 code for NOTHING_PE was much simpler:

    if (etype==NOTHING_PE) { if (multi_wanted==100) L__M(##Miscellany, 43);
                             else L__M(##Miscellany, 44);  }

Looking at the inform-2006 git history, we find that the new code was added in commit 0fd75712 by Cedric Knight in Dec 2003, with commit msg: “multi_pe2.patch fixes a few parser errors and gives more informative NOTHING_PE. Also fixes (the) nothing errors which occur with TRACE 4.”.

My first attempt to diff this patch against the preceding commit produced the whole of parserm.h. Apparently, in between these two commits, dos line endings were converted to unix ones, creating a change on every line. A couple of git checkouts, a dos2unix, and a diff later, we have the elusive multi_pe2.patch:

[rant=“multi_pe2.patch”][code]
— parserm-58c6 2016-05-01 20:41:14.000000000 -0700
+++ parserm-0fd7 2016-05-01 20:34:35.000000000 -0700
@@ -277,7 +277,8 @@
Global meta; ! Verb is a meta-command (such as “save”)

Array multiple_object → 64; ! List of multiple parameters
-Global multiflag; ! Multiple-object flag
+Global multiflag; ! Multiple-object flag passed to actions

                               ! Also used to prevent misleading MULTI_PE

Global toomany_flag; ! Flag for “multiple match too large”
! (e.g. if “take all” took over 100 things)

@@ -1622,11 +1623,14 @@
#Endif; ! DEBUG

 best_etype = STUCK_PE; nextbest_etype = STUCK_PE;

multiflag = false;

! “best_etype” is the current failure-to-match error - it is by default
! the least informative one, “don’t understand that sentence”.
! “nextbest_etype” remembers the best alternative to having to ask a
! scope token for an error message (i.e., the best not counting ASKSCOPE_PE).
! multiflag is used here to prevent inappropriate MULTI_PE errors
! in addition to its unrelated duties passing information to action routines

!!!
!
@@ -1718,7 +1722,7 @@
while (wn < num_words) {
l=NextWord();
if ( l && (l->#dict_par1) &8 ) { ! if preposition

                               l = Descriptors(false);  ! skip past THE etc

                               l = Descriptors();  ! skip past THE etc
                               if (l~=0) etype=l;  ! don't allow multiple objects
                               l = NounDomain(actors_location, actor, NOUN_TOKEN);
                               #Ifdef DEBUG;

@@ -1860,7 +1864,7 @@

             if (parameters >= 1 && results-->2 == 0) {
                 l = ReviseMulti(results-->3);

               if (l ~= 0) { etype = l; break; }

               if (l ~= 0) { etype = l; results-->0 = action_to_be; break; }
           }
           if (parameters >= 2 && results-->3 == 0) {
               l = ReviseMulti(results-->2);

@@ -2035,8 +2039,17 @@
if (etype == JUNKAFTER_PE) L__M(##Miscellany, 41);
if (etype == TOOFEW_PE) L__M(##Miscellany, 42, multi_had);
if (etype == NOTHING_PE) {

   if (multi_wanted==100)  L__M(##Miscellany, 43);

   else                    L__M(##Miscellany, 44);

   if (results-->0 == ##Remove && results-->3 ofclass Object) {

       noun = results-->3; ! ensure valid for messages

       if (noun has animate) L__M(##Take, 6, noun);

       else if (noun hasnt open) L__M(##Take, 9, noun);

       else if (children(noun)==0) L__M(##Search, 6, noun);

```
       else results-->0 = 0;
```
```
       }
```
```
   if (results-->0 ~= ##Remove) {
```

       if (multi_wanted==100)  L__M(##Miscellany, 43);

       else                    L__M(##Miscellany, 44);

```
   }
```
}
if (etype == ASKSCOPE_PE) {
scope_stage = 3;
@@ -2138,7 +2151,7 @@
indef_nspec_at = 0;
];

-[ Descriptors allow_multiple o x flag cto type n;
+[ Descriptors o x flag cto type n;
ResetDescriptors();
if (wn > num_words) return 0;

@@ -2177,7 +2190,7 @@
if (take_all_rule == 1) take_all_rule = 2;
indef_type = indef_type | PLURAL_BIT;
}

   if (allow_plurals && allow_multiple) {

   if (allow_plurals) {
       n = TryNumber(wn-1);
       if (n == 1) { indef_mode = 1; flag = 1; }
       if (n > 1) {

@@ -2191,7 +2204,6 @@
wn–; ! Skip ‘of’ after these
}
wn–;

if ((indef_wanted > 0) && (~~allow_multiple)) return MULTI_PE;
return 0;
];

@@ -2250,7 +2262,7 @@

[ ParseToken__ given_ttype given_tdata token_n
token l o i j k and_parity single_object desc_wn many_flag

```
        token_allows_multiple;
```

```
        token_allows_multiple prev_indef_wanted;
```
!!!
!
@@ -2448,7 +2460,7 @@
.TryAgain;

! First, we parse any descriptive words (like “the”, “five” or “every”):

l = Descriptors(token_allows_multiple);

l = Descriptors();
if (l ~= 0) { etype = l; return GPR_FAIL; }

.TryAgain2;
@@ -2478,14 +2490,20 @@
#Endif; ! DEBUG
l = NounDomain(actors_location, actor, token);
if (l == REPARSE_CODE) return l; ! Reparse after Q&A

   if (indef_wanted == 100 && l == 0) l = 1;

   if (indef_wanted == 100 && l == 0 && number_matched == 0)

          l = 1;  ! ReviseMulti if TAKE ALL FROM empty container

   if (token_allows_multiple && ~~multiflag) {

       if (best_etype==MULTI_PE) best_etype=STUCK_PE;

```
       multiflag = true;
```

   } 
   if (l == 0) {
       if (indef_possambig) {
           ResetDescriptors();
           wn = desc_wn;
           jump TryAgain2;
       }

       if (token_allows_multiple && etype==MULTI_PE) etype=0;

       if (etype == MULTI_PE or TOOFEW_PE && multiflag) etype = STUCK_PE;
       etype=CantSee();
       jump FailToken;
   } ! Choose best error

@@ -2561,7 +2579,7 @@
wn = desc_wn;
jump TryAgain2;
}

       etype = CantSee(); return GPR_FAIL;            ! Choose best error

       etype = CantSee(); jump FailToken;            ! Choose best error
   }

   ! ...until it produces something not held by the actor.  Then an implicit

@@ -2618,6 +2636,7 @@
#Endif; ! DEBUG

     if (~~token_allows_multiple) {

       if (multiflag) jump PassToken; ! give UPTO_PE error
       etype=MULTI_PE;
       jump FailToken;
   }

@@ -2670,11 +2689,18 @@
! Descriptors)

 if (allow_plurals && indef_guess_p == 1) {

```
   #Ifdef DEBUG;
```

   if (parser_trace >= 4) print "   [Retrying singulars after failure ", etype, "]^";

```
   #Endif;
```

   prev_indef_wanted = indef_wanted;
   allow_plurals = false;
   wn = desc_wn;
   jump TryAgain;

}

return -1;

if ((indef_wanted > 0 || prev_indef_wanted > 0) && (~~multiflag)) etype = MULTI_PE;
return GPR_FAIL;

]; ! end of ParseToken__

@@ -6233,9 +6259,8 @@
];

[ Defart o i;

if (~~o ofclass Object) {print (PSN__) o; return;}
i = indef_mode; indef_mode = false;
if (o has proper) {

if ((~~o ofclass Object) || o has proper) {
indef_mode = NULL; print (PSN__) o; indef_mode = i;
return;
}
@@ -6249,9 +6274,8 @@
return;
}
#Endif;

if (~~o ofclass Object) {print (PSN__) o; return;}
i = indef_mode; indef_mode = false;
if (o has proper) {

if ((~~o ofclass Object) || o has proper) {
indef_mode = NULL; print (PSN__) o; indef_mode = i;
return;
}
[/code][/rant]
One thing to note for later is that the comments make reference to preventing misleading MULTI_PE errors.

Looking for a rationale behind this change, I found a lone message about the patch from Cedric Knight on the inform-maintainers mailing list in Aug 2004. He says:

(Note the reference to patch L61109. It wasn’t implemented in 6/11, but it has been ported to 6/12 from the inform-2006 repo. So we have 3 sets of behaviors to consider: 6/10 (pre multi_pe2), 6/11 (post multi_pe2 but pre L61109), and 6/12 (post multi_pe2 and post L61109). More on this below.)

A search for David Porterfield shows that he reported library issue L61035 around the same time, which seems to motivate the changes in the multi_pe2.patch. It looks like, back in the mists of time, TAKE ALL FROM X produced a suboptimal CANTSEE_PE msg (“You can’t see any such thing.”) and REMOVE ALL FROM X produced an incorrect MULTI_PE msg (“You can’t use multiple objects with that verb.”). The multi_pe2 patch fixes both of these issues, but it seems that the more specific NOTHING_PE messages with which they were replaced aren’t ideal in all cases.

Let’s look at the differences in these responses between 6/10, 6/11, and 6/12 (pre-fix):

TAKE/REMOVE ALL FROM GIRL (who doesn’t have anything):
6/10: TAKE: “You can’t see any such thing.”
REMOVE: “You can’t use multiple objects with that verb.”
6/11: “That seems to belong to the girl.”
6/12: “[** Programming error: tried to test “has” or “hasnt” of nothing **]
nothing seems to belong to the girl.”

TAKE/REMOVE ROCKS FROM GIRL (who doesn’t have them):
6/10: “You can’t see any such thing.”
6/11: “You can’t see any such thing.”
6/12: “You can’t see any such thing.”

TAKE/REMOVE ALL FROM STONE (not a container or supporter):
6/10: TAKE: “You can’t see any such thing.”
REMOVE: “You can’t use multiple objects with that verb.”
6/11: “That can’t contain things.”
6/12: “That can’t contain things.”

TAKE/REMOVE ROCKS FROM STONE (not a container or supporter, rocks aren’t on it):
6/10: “You can’t see any such thing.”
6/11: “You can’t see any such thing.”
6/12: “You can’t see any such thing.”

TAKE/REMOVE ALL FROM CHEST (closed):
6/10: TAKE: “You can’t see any such thing.”
REMOVE: “You can’t use multiple objects with that verb.”
6/11: “The chest isn’t open.”
6/12: “The chest is not open.”

TAKE/REMOVE ROCKS FROM CHEST (closed):
6/10: “You can’t see any such thing.”
6/11: “You can’t see any such thing.”
6/12: “You can’t see any such thing.”

TAKE/REMOVE ALL FROM CHEST (open and empty):
6/10: TAKE: “You can’t see any such thing.”
REMOVE: “You can’t use multiple objects with that verb.”
6/11: “The chest is empty.”
6/12: “The chest is empty.”

TAKE/REMOVE ROCKS FROM CHEST (open and empty):
6/10: “You can’t see any such thing.”
6/11: “You can’t see any such thing.”
6/12: “You can’t see any such thing.”

TAKE ROCKS (when carried by player and multiple possibilities for inferred indirect obj available):
6/10: What do you want to take those things from?
6/11: What do you want to take those things from?
6/12: There is nothing to take.

(In this last case, lib 6/12’s parser trace output says “Unable to choose best group, so ask player.]”, but it doesn’t. Instead it soon says “[token resulted in failure with error type 4]”. lib 6/10 and 6/11 trace output shows that they print the same “ask player” trace message and then actually ask the player. I haven’t investigated this yet, but it’s likely the result of one of the many patches that was integrated from inform-2006. See below for general comments on that.)

TAKE ROCKS (when carried by player and girl is the only possibility for inferred indirect obj available):
6/10: “There are none at all available!”
6/11: “That seems to belong to the girl.”
6/12: [** Programming error: tried to test “has” or “hasnt” of nothing **]
nothing seems to belong to the girl.

(Here we see that an inferred (from the girl) behaves differently from an explicit “from the girl”, and similarly to “take all from girl”. It produces NOTHING_PE instead of CANTSEE_PE. We could pursue this further, but, again, see below for general comments.)

The first thing that we see looking at this output is that the messages for taking a specific set of things (e.g., ROCKS) are uniformly based on CANTSEE_PE across all 3 versions of the lib and seem reasonable. The player has asked to take something from a place where it isn’t (or, in the case of the closed chest, isn’t visible). Hence, “You can’t see any such thing.”

In contrast, lib 6/10’s messages for TAKE/REMOVE ALL FROM X are bad to mediocre, and a fix was needed. MULTI_PE is a straight up lie, since ‘remove’ can be used with multiple objects, and CANTSEE_PE misleadingly suggests that it’s the indirect object that can’t be seen (i.e., >TAKE ALL FROM CHEST. “You can’t see any such thing.” suggests that there’s no chest.) However, it’s arguable that the generic “There’s nothing to take/remove.” would have been better in many cases than the indirect object focused messages that were selected. Specifically:

TAKE ALL FROM GIRL → “That seems to belong to the girl.” Not terrible (but, as shown above, untenable in 6/12 when ALL is nothing and we want to print a series of “The X2 seems to belong to the girl” lines, one for each object that she has). “There’s nothing to take.” works equally well and sidesteps this issue, hence our proposed fix.

TAKE ALL FROM STONE → “That can’t contain things.” A good msg when the player is trying to put something on the stone, but less good here. It’s more straightforward to just say that there’s nothing in/on it than to say that it’s not possible for something to be in/on it. It would be best to say “There’s nothing in/on the stone.”, but “There’s nothing to take” is almost as good.

TAKE ALL FROM (CLOSED) CHEST → “The chest isn’t open.” Good if the chest isn’t transparent, since the player doesn’t have x-ray vision. Ok if the chest is transparent, but “There’s nothing in the chest.” would be slightly better in that case, imo. Maybe even “There’s nothing in the chest (and it’s closed).” Note that if the chest is transparent and there’s something in it, the chest is implicitly opened if the player asks to take that item. This is a case where “There’s nothing to take.” is a worse choice.

TAKE ALL FROM (EMPTY AND OPEN) CHEST → “The chest is empty.” Perfect. “There’s nothing to take” would be ok, but not as good.

Overall, I think that the fix for MULTI_PE is good, but I’m ambivalent about the NOTHING_PE piece of it. Another issue that I touched on above is that each grammar line produces its own results–>0, 1, 2 and 3, but old values of results–>2 and 3 aren’t overwritten unless a subsequent grammar line requires them. It’s assumed that, if results–>1 is 0, then results–>2 and 3 will be safely ignored. However, this patch relies on results–>3 without guaranteeing that the line that produced the results–>3 that it uses matches the line that produced the etype that it uses. While it might seem, based on grammar.h, that this isn’t a problem in practice, an author is free to add new grammar and change existing grammar for ##Remove.

So, I’m not convinced that this full patch is desirable. However, it’s in 6/11, which has been serving as a baseline for 6/12. Given that, I think that we should leave it alone for now and make the minimal possible change to resolve issue 38’s runtime error.

Recall that Cedric Knight said, about the multi_pe2 patch, "Probably some of those changes can be reversed if we adopt inform-fiction.org/patches/L61109.html. So, let’s look at patch L61109. The problem, briefly, is that GIVE/SHOW TO sometimes produces a bogus ANIMA_PE (“You can only do that to something animate.”) when there’s a parse error. The proposed solution is to change the way that the parser chooses “the best” etype. Cedric has this to say about the patch:

This is quite a deep-seated problem, requiring a general solution. At the moment, each grammar line is checked until it fails, and the error given is the least serious and most specific. The appropriateness of error messages can be improved by continuing to scan the line to find the worst, most general error, and base the error on the line with the ‘best worst problem’. The general philosophy is to first guess which sense of a command is most likely to be intended, then see why it doesn’t make sense, rather than just look at the first few words.

This change i[s] fairly radical; it may need some refinement with the plural errors, and can affect some hacks that assume that scope or GPR routines are not called if a previous token has failed. The bailout condition is also slightly arbitrary.

This change was not included in lib 6/11, but it was in the inform-2006 tree as commit ed1595b5 and ported into the current inform6lib tree as commit 65cfc1f1, so it exists in 6/12.

Let’s look in more detail at the problem that this patch solves. Here’s 6/11:

Why does this happen? As we mentioned earlier, the parser error messages are arranged in increasing numerical order from least specific to most specific. etype is set when the parser fails to match a token, and the parser maintains a best_etype reflecting the best etype found so far; this is the error message that will be shown to the user if all grammar lines fail. Prior to the L61109 patch, as soon as the parser failed to match a token, it would update etype and best_etype and move on to the next grammar line.

This is the grammar for ‘give’:

Verb 'give' 'feed' 'offer' 'pay'
    * held 'to' creature                        -> Give
    * creature held                             -> Give reverse
    * 'over' held 'to' creature                 -> Give;

Consider how the parser would parse GIVE ROCK OF AGES TO GIRL.

Line 0: ROCK matches the held token (despite not being held; the parser will enter notheld_mode and generate an implicit take), but we fail to match OF in the input with ‘to’ in the grammar. So, this line fails with etype = STUCK_PE (1), and best_etype is also set to STUCK_PE.

Line 1: We try to match ROCK with a creature token, but the rock objects aren’t animate. So, this line fails with etype = ANIMA_PE (11). Since ANIMA_PE is considered more specific than STUCK_PE (11 > 1), best_etype is now set to ANIMA_PE.

Line 2: We fail to match ‘over’ in the input and fail with etype = STUCK_PE. best_etype remains ANIMA_PE.

Having failed to match all grammar lines, the parser sets etype = best_etype and enters section I to print an error message. This is how we get the inappropriate ANIMA_PE error msg.

L61109 tries to solve this issue by changing the way that grammar lines are parsed. The parser no longer stops at the very first token error, but continues to parse the line, choosing the “best” / most informative etype instead of the first… A new variable called line_etype is introduced to hold the best etype found while parsing the current line. When a line fails, best_etype is updated using line_etype. The patch considers the least specific etype to be best within a line, while the most specific etype is still considered the best when comparing different lines.

Here’s 6/12, with L61109:

This produces a better error message in the GIVE/SHOW TO case, but it introduces new problems.

In 6/11, prior to this patch, we instead have:

6/11’s MULTI_PE is correct, while 6/12’s CANTSEE_PE is misleading; the rocks are present in the location. (Interestingly, 6/10 produces ANIMA_PE for GIVE ROCKS TO GIRL.)

This code in section G of the parser (referred to by Cedric as “the bailout condition”) seems to be responsible for the change:

                if (l == false) {
                    if (etype < line_etype) line_etype = etype;
                    if (etype == STUCK_PE || wn >= num_words) break;

Instead of unconditionally stopping when ParseToken fails, we only stop parsing a line if we encounter STUCK_PE (the “best” in this context, so why continue?) or reach the end of the input.

If we recall the comments about this patch, higher numbered etypes are called “the least serious and most specific”, while lower numbered ones are called “the worst, most general error”. I think that this description conflates generality and severity. Looking at the list of errors, I find it difficult to put them into a strict order of severity or specificity. This is another patch that I’m ambivalent about.

If we look at the changes in the library behavior as we’ve applied the multi_pe2 patch and the L61109 patch, it seems as though we repeatedly fix an issue with one inappropriate message and open up another one. It has a very ad hoc, whack-a-mole feeling.

Let’s ask our final why.

Why is the parser producing messages like “the chest is empty” or “you must do that to something animate”? Why does the parser blend syntax and semantics in such as way as to create these problems instead of pushing more semantic processing to higher level action routines? An obvious answer is that some semantic info is required to perform disambiguation, and, since it’s already in the parser, we might as well be efficient and stop processing earlier when the context tells us that an action is doomed to failure. It’s convenient to have a restrictive grammar token like creature, but… is it really true that “ask chair about lunch” is meaningless in the same way that “about ask lunch chair” is meaningless? There is a distinction to be made between a clearly understood command that is, in some way, nonsensical vs. a truly meaningless or ambiguous command. If we say “ask chair about lunch” and it’s unclear which chair is meant, then the parser can guess using context or ask the player to clarify. If there’s only one chair that we could be talking to, why not pass that command along to AskSub and let it run an ObjectIsAnimate check similar to the ObjectIsUntouchable check that many actions run? If the chair doesn’t want to answer, the action routine can say “There is no response.”

My conclusion here is that the parser needs to be primarily in the parsing business and to push some of its semantic work to the action level. This is the root cause of the parser being a tangle.

So what do we do now? Looking at L61109… this is a single patch described as a “fairly radical change” addressing “a deep seated problem”, and it’s just one of many library patches and many user suggestion patches that have come into 6/12 from the inform-2006 tree. At this point, there’s been a significant amount of churn, with potentially subtle and widespread effects, and we have to deal with results. I think that our goal at this point needs to be to minimize changes and stabilize the library. I think that the best thing we can do is to turn our attention to inform6-test and build up a large set of library tests so we can understand 6/12s current behavior and have confidence in the effects of future changes. We need to think about the best way for inform6-test to service both 6/11 and 6/12 (different subdirs of tests, different branches, different forks?).