Help wanted writing I6 library tests

Yeah, I’ve noticed that GIRL, DROP ROCKS is being treated equivalently to GIRL, DROP FKFKFHDH. The command’s not understood (result assembled by part H of the parser, I think), so the orders code in actor_act transforms the command into ANSWER “DROP ROCKS” TO GIRL, with the player as the new actor, and jumps back to begin__action. Maybe some interaction between held status and multiple objects, since it only fails for multiple rocks and only when the rocks are in the wrong place (not held for DROP, held for TAKE). I’ll take a look.

(Totally bringing the thread back on topic) I ran the automated test suite on the new library for kicks. There were ~50 tests (some test files contain more than one test), and 158 failures. Each chunk of game output that doesn’t match expectations counts as a failure, so a single test can produce many failures. Over half of the tests had no failures at all. Some failures are probably due to minor changes in default library messages or behavior. I think we’ll need a 6.12-specific version of the tests, eventually.

Is SAY “TAKE STONE” TO GIRL supposed to work as though you did GIRL, TAKE STONE?

I don’t think so. I think “SAY TO NPC” (or an order that is not understood and not handled by an orders routine) creates an ##Answer action that goes to the NPC’s life routine (this based on §17 and §18 of the DM4 and what I’ve seen of the library code), and it’s the responsibility of the author to parse and handle that stuff using consult_from/consult_words.

Where should I start looking for the bug?

I’d go into the test game, type TRACE 5, followed by GIRL, DROP ROCKS, and capture the output. Then, I’d review the output, look for interesting trace msgs in it (about not matching the rocks to the word “rocks”), and then search the source code to determine what parser code is producing those messages and why. (Note: comma counts as a word, so rocks is word 4.) Comparison with trace output from GIRL, DROP ROCK could also be enlightening.

I’ve been looking into this issue and making some progress. Full update soon.

For now, I want to mention that the girl is a red herring. The issue has nothing to do with conversation or orders. We can see the same thing when the player is the actor:

The DROP ROCK and TAKE ROCK commands make it past the parser and the responses that we see come from action handling.

The DROP ROCKS and (the second) TAKE ROCKS commands generate parser errors and never make it to the action handling stage.

Ran into a new issue while investigating the DROP ROCKS issue.

I thought that it might be due to some changes that I was testing, but it appears in 6/12-beta1 as originally released and, in a more benign form, in 6/11.

Same results with the current version of 6.12.1 in git (8f9a0ad9).

I’ll take a look at it after I’ve wrapped up this current issue.

I’ve finished looking into the DROP ROCKS issue.

TL;DR: It’s not a bug, it’s a feature.

Read on for the gory details, some actual bugs, and some thoughts on inform6lib development.

What’s the issue?

When we run the test program for issue 1885, we see:

When all of the rocks are on the ground and we try to drop a single rock, parsing succeeds and we receive a response from the Drop action telling us that the rock is already here.

When we try to drop multiple rocks, parsing fails and no action is generated. Instead, we receive an error message from the parser.

When we order the girl to drop the rocks, this error is transformed into a NotUnderstood action, sent to the girl’s orders, and redirected to her life, where it ultimately produces the suboptimal default response that we see.

In this post, we’ll focus on the simpler DROP ROCKS command with the player as the actor.

What’s causing it?

At parser trace level 1, we can see the big picture:

There are 3 grammar lines for DROP, and “drop rocks” fails to match any of them. Line 0 (* multiheld → Drop) is one that we’d expect to match our input and the one that we’ll focus on here.

The grammar requires the word ‘drop’ followed by text matching a multiheld token. The DM4 describes multiheld as “one or more held objects”, but, as we can see from our success parsing DROP ROCK, this isn’t a hard and fast rule; the reality is a little more nuanced.

If we crank up the trace level to 5, we can see more detail as the parser tries to parse “rocks” as a multiheld token:

>drop rocks
...
[line 0 * multiheld -> Drop]
 [line 0 token 1 word 2 : multiheld]
  [Object list from word 2]
  [Calling NounDomain on location and actor]
   [NounDomain called at word 2]
   seeking definite object
...
   [NounDomain made 4 matches]
   [Adjudicating match list of size 4 in context 3]
   indefinite type: plural
   number wanted: all
   most likely GNAs of names: 4095
   Scoring match list: indef mode 1 type 8, satisfying 0 requirements:
     The rock (27) in the Somewhere : 156 points
     The rock (28) in the Somewhere : 156 points
     The rock (29) in the Somewhere : 156 points
     The rock (30) in the Somewhere : 156 points
   Best guess the rock (27)   
   Rejecting it
   Best guess the rock (28)
   Rejecting it
   Best guess the rock (29)
   Rejecting it
   Best guess the rock (30)
   Rejecting it
   Best guess ran out of choices
   Made multiple object of size 0]
...

This trace is mostly produced by NounDomain and Adjudicate in parserm.h. The call chain that got us there is:
InformLibrary::play → InformParser::parse_input → Parser__parse (Part G) → ParseToken__ (Part D) → NounDomain → Adjudicate

The parser calls ParseToken__ to match a multiheld token (with input “rocks”), which calls NounDomain to find the best matching object or objects in the actor’s scope.

Notice that, when NounDomain is first called, we’re “seeking definite object”, but, by the beginning of Adjudicate, we’re looking for all of an indefinite group of objects (“indefinite_type: plural, number wanted: all”). This happens because, when NounDomain calls SearchScope to find matching objects in the actor’s scope, TryGivenObject finds that a rock matches the plural dictionary word ‘rocks//p’. This causes the parser to switch into indefinite plural mode. We look for “all” (represented by 100) because no specific number has been requested (we didn’t type DROP TWO ROCKS):

            if (dict_flags_of_noun & DICT_PLUR) {
                if (~~allow_plurals) k = 0;
                else {
                    if (indef_mode == 0) {
                        indef_mode = 1; indef_type = 0; indef_wanted = 0;
                    }
                    indef_type = indef_type | PLURAL_BIT;
                    if (indef_wanted == 0) indef_wanted = 100;
                }
            }

In contrast, DROP ROCK does not trigger this code and remains in definite mode, accounting for the difference in results that we’ll see.

After NounDomain finds multiple objects that match “rocks” (“NounDomain made 4 matches”), it calls Adjudicate to score them and disambiguate among them. This process depends on the actor, action_to_be, the token that we’re trying to match, whether we’re in definite or indefinite mode, and the state of the world model at the time. It’s described in some detail in §33 of the DM4 on pages 240-242, about which more below.

During disambiguation, we reach a block of code specific to indefinite plural mode, and (spoiler) it rejects all four of our rocks! (For I7 people: this code is analogous to the deciding whether all includes activity.):

    if (indef_mode == 1 && indef_type & PLURAL_BIT ~= 0) {
        if (context ~= MULTI_TOKEN or MULTIHELD_TOKEN or MULTIEXCEPT_TOKEN
                     or MULTIINSIDE_TOKEN) {
            etype = MULTI_PE;
            return -1;
        }
        i = 0; offset = multiple_object-->0; sovert = -1;
        for (j=BestGuess() : j~=-1 && i<indef_wanted && i+offset<63 : j=BestGuess()) {
            flag = 1;
            if (j has concealed && j has worn) flag = 0;
...
            if (context == MULTIHELD_TOKEN or MULTIEXCEPT_TOKEN && parent(j) ~= actor)
                flag = 0;
...
            n = ChooseObjects(j, flag);
            if (n == 0) n = LibraryExtensions.RunWhile(ext_chooseobjects, 0, j, flag);
            switch (n) {
              2: flag = 0;  ! forcing rejection
              1: flag = 1;  ! forcing acceptance
             !0:            ! going with parser's decision
            }
            if (flag == 1) {
                i++; multiple_object-->(i+offset) = j;
                #Ifdef DEBUG;
                if (parser_trace >= 4) print "   Accepting it^";
                #Endif; ! DEBUG
            }
            else {
                i = i;
                #Ifdef DEBUG;
                if (parser_trace >= 4) print "   Rejecting it^";
                #Endif; ! DEBUG
            }
        }
 ...
        return 1;
    }

The for loop iterates over all candidate objects and accepts or rejects each one in turn based on a series of tests. For DROP ROCKS, all of the rocks are rejected, resulting in a multiple object of size 0 which percolates up the parser and triggers a parse error.

The specific test that rejects the rocks is " if (context == MULTIHELD_TOKEN or MULTIEXCEPT_TOKEN && parent(j) ~= actor)".

So, that’s what’s happening.

In indefinite plural mode, for a multiheld token, an object is rejected unless the actor is holding it, even when there are no other candidates.

In definite mode, an object might be disfavored for multiheld because it’s not held, but it’s not rejected outright when there are no other candidates.

This code raises a couple issues that I’ll discuss later in this post.

  • Why is MULTIEXCEPT also included in this test when it ostensibly has nothing to do with held objects but simply excludes the indirect object?
  • This indef plural code differs from the 6/11 version, with the initial value of flag reversed. The 6/12 version of the test for concealed and worn is incorrect.

Notice also that an author can provide a ChooseObjects function that can override the parser’s decision (stored in flag). See DM4 §33 page 239.

Is this behavior intended?

Yes.

It’s mentioned in the DM4’s explanation of disambiguation:

Section (iii) makes it clear that this is intended behavior.

For additional evidence, in issue 1488, filed against I7 about a multiexcept issue, but stemming from the exact same line of code, GN says:

(The I7 does the player mean rules are roughly analogous to ChooseObjects.)

Ok. It’s intended behavior. Why?

So that this happens:

instead of this:

(Similarly for DROP ALL.)

The idea behind the indef plural disambiguation is that we want to decide what should reasonably be included in “all” or a group of objects based on the context of the actor, the action, and the world.

The particular rule that caused our issue says that, when an actor is performing an action that prefers held objects, those are the objects that the parser should choose preferentially. It only breaks down when there are no candidate objects that meet the criteria, as in DROP ROCKS when all rocks are on the ground.

So what can we do?

If we want the best of both worlds, we can write a ChooseObjects routine for our test program that will allow the parser to reject rocks on the ground when there are better candidates, while forcing it to accept rocks on the ground when there are none. (This also handles Take which suffers from a similar issue.)

[ ChooseObjects obj code x;
  if (code == 2) rfalse;
  if (action_to_be == ##Drop && obj ofclass Rock && obj notin actor) {
      objectloop (x in actor) {
          if (x ofclass Rock) rfalse;
      }
      rtrue;
  }
  if (action_to_be == ##Take && obj ofclass Rock && obj in actor) {
      objectloop(x ofclass Rock) {
          if (TestScope(x, actor) && x notin actor) rfalse;
      }
      rtrue;
  }
  rfalse;
];

With this ChooseObjects in place, we get these results:

Can we do more?

We could change the parser to relax this rejection filter when there is an undifferentiated set of candidates while still using it to disambiguate among candidates with differing characteristics.

However, it would take a bit of restructuring since the indef plural loop rejects candidates one at a time in decreasing BestGuess order. It’s only later in Adjudicate that objects are grouped into equivalence classes.

Also, it’s not clear that everyone would agree that the behavior in the transcript above is desirable. To quote the DM4’s introduction to ChooseObjects:

My conclusion: use ChooseObjects.

Here ends the discussion of DROP ROCKS. Next up are a couple of side issues, and then some thoughts about I6 library development moving forward.

Side issue: MULTIEXCEPT

When looking at the test that caused our issue:

if (context == MULTIHELD_TOKEN or MULTIEXCEPT_TOKEN && parent(j) ~= actor)

and reading code elsewhere in the parser, it wasn’t clear to me why MULTIEXCEPT was so often grouped with MULTIHELD. The DM4 summarizes it as “one or more in scope, except the other object” and later describes it in more depth, saying:

What does “everything but a particular object” have to do with held objects? The reference to a player’s possessions is suggestive, but it was only when I looked at grammar.h that it became clear.

These are the verbs in 6/12 that use MULTIEXCEPT:

Verb 'drop' 'discard'
    * multiheld                                 -> Drop
    * multiexcept 'in'/'into'/'down' noun       -> Insert
    * multiexcept 'on'/'onto' noun              -> PutOn;
...
Verb 'insert'
    * multiexcept 'in'/'into' noun              -> Insert;
...
Verb 'put'
    * multiexcept 'in'/'inside'/'into' noun     -> Insert
    * multiexcept 'on'/'onto' noun              -> PutOn
    * 'on' held                                 -> Wear
    * 'down' multiheld                          -> Drop
    * multiheld 'down'                          -> Drop;

All of the grammar lines that use the multiexcept token involve Insert or PutOn actions. In other words, placing held objects somewhere. The purpose of the “except” part of multiexcept is to exclude the indirect object so that things are not placed in or on themselves, which would cause cycles in the object tree (making it not a tree (well, not a forest)).

So, MULTIEXCEPT really should be thought of as MULTIHELDEXCEPT. I don’t know what the original intention was, but that’s what it’s become in practice today.

Side issue: changes to “rule 7ip” indef plural code since lib 6/11

The indef plural code in Adjudicate that we quoted above differs from the lib 6/11 version.

Here’s 6/11:

        for (j=BestGuess() : j~=-1 && i<indef_wanted && i+offset<63 : j=BestGuess()) {
            flag = 0;
            if (j hasnt concealed && j hasnt worn) flag = 1;

Here’s 6/12:

        for (j=BestGuess() : j~=-1 && i<indef_wanted && i+offset<63 : j=BestGuess()) {
            flag = 1;
            if (j has concealed && j has worn) flag = 0;

In both cases, the meaning of the flag variable is the same. flag == 1 means accept an object, and flag == 0 means reject it.

In 6/11 the flag is initialized to 0 (reject), and various tests set it to accept object j if it’s a good choice in context. In 6/12, the flag is initialized to 1 (accept) and various tests clear it to reject object j if it’s not a good choice.

In 6/11, we see that j is provisionally accepted if it’s not concealed and it’s not worn. This makes sense in the context of commands like TAKE ALL (where we wouldn’t want to include concealed objects) or DROP ALL (where we wouldn’t want to drop things that the actor is wearing), and the excerpt from the DM4 that we quoted above is consistent with this: “mark each as accept unless (i) it has worn or concealed”.

In 6/12, because the default value of the flag was reversed, this test also had to be reversed. Instead of accepting if j isn’t concealed and isn’t worn, we want to reject j if it is concealed or is worn. However, the code, as written, rejects it only if it’s both concealed and worn. This is a bug.

The code should read:

if (j has concealed or worn) flag = 0;

Inform 6 Library Development and Inform 7’s I6 Template Code

In an earlier post in this thread, I described the source of an error in how the parser constructs its results after calling an actor’s grammar routine and provided a patch for it along with some concerns (multiple objects, noun = 0 and second ~= 0).

Let’s look at the history of this issue.

In 6/11, we have this:

        i = RunRoutines(actor, grammar);
...
        if (i == 1) {
            results-->0 = action;
            results-->1 = noun;
            results-->2 = second;
            rtrue;
        }
...

This is clearly a bug, because the parser uses the first four words of results to store the action, the number of parameters (0, 1, or 2), the noun, and the second respectively. Here, someone forgot to place the number of params in results–>1 and instead shifted both noun and second one word closer to the beginning of the array. This creates a bad num params value based on the obj id of the noun, ignores the noun, treats the second as the noun, and leaves the second unset.

In 6/12, this fix was applied:

       if (i == 1) {
            results-->0 = action;
            results-->1 = 2;            ! Number of parameters
            results-->2 = noun;
            results-->3 = second;
            rtrue;
        }

However, this wasn’t quite correct, because it unconditionally sets num params to 2 and so doesn’t handle cases where the grammar routine sets noun or second to 0.

Then we have my recent proposed fix:

       if (i == 1) {
            results-->0 = action;
            results-->1 = 0;            ! Number of parameters
            results-->2 = noun;
            if (noun ~= 0) (results-->1)++;
            results-->3 = second;
            if (second ~= 0) (results-->1)++;
            rtrue;
        }

This ensures that num params matches the number of nonzero parameters, but, as I said, I had concerns about some cases. In considering this, I decided to see what Inform 7’s I6 template code does.

I7 does this (translated to inform6lib-speak):

        if (i == 1) {
            results-->0 = action;
            results-->1 = 0;
            results-->2 = noun;
            results-->3 = second;
            if (noun) results-->1 = 1;
            if (second) results-->1 = 2;
            rtrue;
        }

This looks like the way to go for 6.12.1. Though I’ll note that it allows grammar to return noun as a multiple object but not second. I’m not sure if we can ever have actions with a single noun and a multiple second (DRAW MUSTACHE ON ALL STATUES). Maybe it’s not a big deal, because we could reverse noun and second and effectively DECORATE ALL STATUES WITH MUSTACHE.

So, looking at this history, I wonder how many other fixed bugs are sitting in the I6 template code and not incorporated back into the regular I6 lib or vice versa?

Should we consider the I7 template layer permanently forked from inform6unix? The template layer docs suggest so:

Would it be possible to share code between the two projects – in particular, the parser code? One of the main differences seems to be the use of activities to allow the game to intercede instead of the traditional entry points and LibraryExtensions hooks. What if we implemented rulebooks in the I6 lib (we have I7’s generated intermediate code as proof of concept) and used #ifdefs to conditionally define BeginActivity, ForActivity, and EndActivity?

If the two codebases won’t be able to converge, should we allow them to diverge freely? Does I7 depend in any way on I6 lib remaining the same? After the automated tests are in place, should we decruft/refactor the library? Should we add features to it?

Thanks for doing this deep analysis – I cannot spend anywhere near that much time on I6 parser issues.

I think that ship has sailed. It will be worth comparing the I6 and I7 parsers for the foreseeable future, but they’re never going to share code again. (No, I7 does not depend on the I6 library in any way.)

Refactoring the I6 library might be a good idea, but I’m not going to push for it because I can’t dedicate any time to helping do it.

I’ve recently been investigating inform6lib issue 38 (analysis and patch forthcoming). The issue involves library messages, and it motivated me to read through the library and look at all calls to L__M for similar errors.

This post describes four issues that I found during this sweep and proposes fixes for them. (Pull request sent on github with these patches.)

Voices and tenses for disambiguation questions

The parser asks a disambiguation question to the player when the object of a command is ambiguous. These questions differ slightly depending on whether the command’s grammar requires a creature token for the object or not. Reading through the messages in english.h, I noticed that two such messages (Miscellany 48 and 49) are implemented inconsistently. Message 49 makes use of the new voices and tenses feature in lib 6/12 while message 48 doesn’t.

Here’s a test program demonstrating the issue:

Constant Story "VOICE, TENSE, AND DISAMBIGUATION";
Constant Headline "^An Interactive Regression Test^";
Constant DEBUG;

! Voice and tense incorporated into disambiguation message for non-creature
! tokens but not for creature tokens.

! Test me with "take / rock / kiss / chris / drop rock / bob, take / coin /
!     bob, kiss".

Include "parser.h";
Include "verblib.h";

Object Start_Room "Somewhere"
  with description "I was not sure where I was.",
  has  light;

Object -> Bob "Bob"
  with name 'bob',
  has  animate male proper;

Object -> Chris "Chris"
  with name 'chris',
  has  animate male proper;

Object -> rock "rock"
  with name 'rock';

Object -> coin "coin"
  with name 'coin';

[ Initialise;
  location = Start_Room;
  player.narrative_voice = 1;
  player.narrative_tense = PAST_TENSE;
];

Include "grammar.h";

Here’s the code in NounDomain responsible for generating these disambiguation prompts:

    ! Now we come to the question asked when the input has run out
    ! and can't easily be guessed (eg, the player typed "take" and there
    ! were plenty of things which might have been meant).

  .Incomplete;

    ...
    if (context == CREATURE_TOKEN) L__M(##Miscellany, 48);
    else                           L__M(##Miscellany, 49, actor);

And here’s the code in LanguageLM that constructs these messages:

  Miscellany: switch (n) {
        ...
        48: print "Whom do you want";
            if (x1 ~= player && x1 ~= nothing) print " ", (the) x1;
            print " to "; PrintCommand(); "?";
        49: print "What ";
            CSubjectVoice(player, "do", "do", "does", "did");
            print " ";
            CSubjectVerb(player, false, true, "want", "want", "want", "want");
            if (x1 ~= player && x1 ~= nothing) print " ", (the) x1;
            print " to "; PrintCommand(); "?";
        ...
    }

We can see two issues here: (1) the call to L__M(##Miscellany, 48) doesn’t pass actor, even though message 48 expects it to be passed as x1. This causes questions about third party actions (“Whom do you want Bob to kiss?”) to instead be phrased as questions about player actions (“Whom do you want to kiss?”), because x1 is nothing, and (2) message 48 does not use CSubjectVoice and CSubjectVerb to apply voice and tense, so the message is always in the 2nd person present, even if voice and tense are configured differently.

The solution to the first issue is to add actor to the L__M(##Miscellany, 48) call in NounDomain:

    if (context == CREATURE_TOKEN) L__M(##Miscellany, 48, actor);
    else                           L__M(##Miscellany, 49, actor);

The solution to the second issue is to rewrite message 48 to make use of CSubjectVoice and CSubjectVerb in the same way that message 49 does:

  Miscellany: switch (n) {
        ...
        48: print "Whom ";
            CSubjectVoice(player, "do", "do", "does", "did");
            print " ";
            CSubjectVerb(player, false, true, "want", "want", "want", "want");
            if (x1 ~= player && x1 ~= nothing) print " ", (the) x1;
            print " to "; PrintCommand(); "?";
        ...
    }

With the fix applied, we have this output:

WriteAfterEntry

Reading verblibm.h, I noticed an issue with WriteAfterEntry in which a container containing a concealed or scenery object can be flagged as empty even if it contains a visible object (depending on the order of the container’s children).

Here’s a test program demonstrating the issue:

Constant Story "~EMPTY~ CONTAINER";
Constant Headline "^An Interactive Regression Test^";

Constant DEBUG;

! WriteAfterEntry:
! Concealed or scenery objects inside a container can cause it to be described
! as empty even if there are also visible objects inside.

! Test me with "put rock in bucket / l".

Include "Parser";
Include "VerbLib";

Object TestChamber "Test Chamber"
  with description "This is the Test Chamber.",
  has  light;

Object -> bucket "bucket"
  with name 'bucket',
  has  container open;

Object -> -> bump "bump"
  with name 'bump',
  has  scenery;

Object -> rock "rock"
  with name 'rock';

[ Initialise;
  location = TestChamber; 
];

Include "Grammar";

Here’s the output:

"EMPTY" CONTAINER
An Interactive Regression Test
Release 1 / Serial number 160428 / Inform v6.33 Library v6.12.1 SD

Test Chamber
This is the Test Chamber.

You can see a bucket (which is empty) and a rock here.

>put rock in bucket
(first taking the rock)
You put the rock into the bucket.

>l

Test Chamber
This is the Test Chamber.

You can see a bucket (which is empty) (in which is a rock) here.

>tree
...
Test Chamber (25)
  yourself (20)
  a bucket (26)
    a rock (28)
    a bump (27)

WriteAfterEntry uses 3 bits to represent objects as being combinations of empty (4), closed (2), and providing light (1). It stores these flags in the variable combo and calls L__M(##ListMiscellany, combo) to print an appropriate message about an object. ListMiscellany msgs 1-7 are are used to describe objects with various combos of these attributes.

Here’s the code that WriteAfterEntry uses to determine whether a container is empty and thus whether the empty bit should be set in combo.

        if ((o has container && (o has open || o has transparent))) {
            objectloop(i in o) {
                if (i has concealed or scenery) j = false;
                if (i hasnt concealed && i hasnt scenery) j = true;
            }
            if (~~j) combo=combo+4;
        }

In 6/11, a container was considered empty if it had no children. This gave away information to the player. If a concealed object was hidden in an otherwise empty container, the container would not be described as empty in object listings. In 6/12, the code above was added to ignore concealed or scenery objects when determining a container’s emptiness.

This code iterates through the objects in a container setting j to true if a visible object (not concealed and not scenery) is found and setting it to false otherwise. After the loop, if j remains false, the container is considered to be empty.

The intent appears to be to set j to true if any visible object is in the container, but every time the loop encounters a concealed or scenery object, j is cleared again, so the end result depends on the order in which the container’s contents are encountered. This is what leads to the results that we see with the test program above, where the bucket contains a rock and a scenery object. The rock is encountered first and causes j to be set to true, but the scenery object causes j to be set back to false, leaving the bucket considered empty despite the rock being inside.

The solution is to not clear j once it’s been set. Any visible object should cause the container to be considered nonempty. Having encountered such an object, there’s no need to continue iterating over the container’s contents. Here’s my proposed fix for this issue:

        if ((o has container && (o has open || o has transparent))) {
            objectloop(i in o) {
                if (i hasnt concealed && i hasnt scenery) {
                    j = true; break;
                }
            }
            if (~~j) combo=combo+4;
        }

With this fix in place, we get this output:

ImplicitDisrobe issue

[ ImplicitDisrobe obj
    res ks;
    if (obj hasnt worn) rfalse;
    res = CheckImplicitAction(##Disrobe, obj);
    ! 0 = Take off object, Tell the user (normal default)
    ! 1 = Take off object, don't Tell
    ! 2 = don't Take object  continue       (default with no_implicit_actions)
    ! 3 = don't Take object, don't continue
    if (res >= 2) rtrue;
    ks = keep_silent; keep_silent = 1; <Disrobe obj, actor>; keep_silent = ks;
    if (obj has worn && obj in actor) rtrue;
    if (res == 0 && ~~keep_silent) L__M(##Drop, 3, noun);
    rfalse;
];

ImplicitDisrobe is passed a single argument, obj, that is the item to be disrobed. While the rest of the routine operates on obj, the call to L__M(##Drop, 3) is passed noun as an argument. The existing library code always calls ImplicitDisrobe on noun, so there is no practical difference, but it’s still inconsistent to pass noun to L__M instead of obj and could lead to bugs in the future.

Proposed fix:

    if (res == 0 && ~~keep_silent) L__M(##Drop, 3, obj);

RemoveSub issue

When the player attempts to remove an object from a closed container that can’t be implicitly opened, an incorrect response is printed.

Here’s a test program demonstrating the issue:

Constant Story "REMOVE OBJECT FROM UNOPENABLE CONTAINER";
Constant Headline "^An Interactive Regression Test^";
Constant DEBUG;

! Test me with "remove emerald from box".

Include "parser.h";
Include "verblib.h";

Object Start_Room "Somewhere"
  with description "You're not sure where you are.",
  has  light;

Object -> glass_box "glass box"
  with name 'glass' 'box',
  has  container openable locked transparent;

Object -> -> emerald "emerald"
  with name 'emerald' 'gem';

[ Initialise;
  location = Start_Room;
];

Include "grammar.h";

This is the library code responsible:

[ RemoveSub i;
    i = parent(noun);
    if (i && i has container && i hasnt open && ImplicitOpen(i)) return L__M(##Remove, 1, noun);
    ...
];
  Remove: switch (n) {
        1:  CSubjectIs  (x1,true); " unfortunately closed.";
        ...
    }

If noun, the object being removed, is in a closed container (called i), and ImplicitOpen(i) fails, then we call L__M(##Remove, 1) to print a message that i is closed. However, RemoveSub passes noun rather than i to L__M, producing a message about the object being removed being closed instead of its container being closed.

The solution is to pass i (the container) to L__M instead of noun (the object contained):

[ RemoveSub i;
    i = parent(noun);
    if (i && i has container && i hasnt open && ImplicitOpen(i)) return L__M(##Remove, 1, i);
    ...
];

With the fix in place, we get this output:

Too many arguments passed to L__M

The majority of the action routines in the library pass extra unused args to L__M for at least one message.

Here’s a list of the actions that have messages with extra args: Answer, Ask, Attack, Burn, Buy, Climb, Cut, Dig, Drink, Drop, EmptyT, Enter, Examine, Insert, Jump, JumpOver, Kiss, Listen, ListMiscellany, Look, LookUnder, Mild, Miscellany, Objects, Pray, Pull, Push, PushDir, Remove, Rub, Search, Sing, Sleep, Smell, Sorry, Squeeze, Strong, Swim, Swing, Take, Taste, Tell, Think, ThrowAt, Tie, Touch, Turn, Wait, Wake, WakeOther, WaveSub, WaveHands.

The most common form of this is an extra noun arg passed to L__M that is mapped to parameter x1 and unused.

Earlier in the 6/12 beta process, a post from auraes informed us that sometimes extra information is required for non-English language messages compared with English ones. This requires passing args to L__M that appear to be unused when looking only at english.h.

I think that the library has gone a bit overboard with The Great Nounification of L__Ms, but auraes’s issue is one to keep in mind if we ever start pruning these seemingly superfluous calls back.

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:

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 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?).

When this level of the parser was designed, there was no other way to do action-aware disambiguation inside the parser. The only disambiguation mechanism was ChooseObjects, but this was a stub routine so it had to be left entirely up to the game author.

I’ve looked at inform6lib issue 32: Responses in DropSub need work.

In lib 6/11, an attempt to drop an indirectly contained object would produce a response like this:

People found this misleading, because the player does in fact have the object, despite not holding it directly.

This lead to Suggestion #122 on the old Inform 6 Suggestions List:

Notice the “at least” in the suggestion. The implication is that there are two levels at which this issue could be solved: (1) the library could provide a more helpful message indicating that the object must be taken out of the container before it’s dropped instead of saying that the player doesn’t have it, or (2) the library could go even further and allow the item to be dropped directly from the sack by performing an implicit take.

The first level solution was implemented in lib 6/12, producing the following results:

The second level solution was implemented in the inform-2006 tree, ported into the inform6lib tree, disabled because of bugs, and finally re-enabled with the recent fix for issue 31. The resulting behavior is:

This brings us to issue 32. The new “Perhaps you should take the foo out of the bar” library message was intended for dropping objects that are indirectly contained by the actor, but it also applies to dropping objects that are outside of the actor, but not siblings of the actor. For example, if an object is on the floor of a room, but the actor is on an enterable supporter in that room (or vice versa). In those cases, we see inappropriate output like:

There are several possible ways to resolve this.

In an earlier post, I suggested broadening the constraint “if noun in parent(actor)” (the test used to decide when to say “The foo is already here.”) to something like “if noun in location”.

However, having researched the history of this issue, I now favor simply reverting L__M(##Drop, 2) back to its pre-Suggestion #122 lib 6/11 form, with updates for the 6/12 voices and tenses feature.

The message was changed as a response to Suggestion #122. Now that the better solution (implicit taking) is in place, there’s no longer a need for the message change. Indirectly contained objects are now implicitly taken if possible (and a parser NOTHELD error msg is produced if not - not ideal, but a separate issue related to an ImplicitTake call being added to the parser for multiheld tokens, distinct from its notheld mode for held tokens), so the library message is only produced for objects that are outside the actor. In these cases, the new message can be inappropriate (“Perhaps you should take the metal tin out of the Start Room first”), and the original “You haven’t got that” message is more broadly applicable.

The change is minimal, which is good for stability this close to a release, and it maintains consistency with lib 6/11 and Inform 7 behavior (not strictly necessary, but I don’t see a need to diverge gratuitously).

Here’s the new msg, in english.h:

[ LanguageLM n x1 x2;
  ...
  Drop: switch (n) {
        ...
        2:  CSubjectVerb(actor, false, false, "haven't got", 0, "hasn't got",
                         "didn't have");
            " ", (the) x1, ".";
        ...

I retained “haven’t/hasn’t got” for the present tense, but “hadn’t gotten” seemed slightly off to me for the past tense, so I went with “didn’t have”. I also followed the 6/12 trend of replacing “that/those” with the name of the object.

The output with this change in place is:

I also found a separate minor issue while testing:

You is capitalized incorrectly, because the nocaps flag isn’t set for the call to CSubjectIsnt for L__M(##GetOff):

  GetOff:   print "But ";
            CSubjectIsnt(actor,true,false); " on ", (the) x1, " at the moment.";

The fix is to set the nocaps flag to true (because the subject is preceded by a conjunction and doesn’t begin the sentence):

  GetOff:   print "But ";
            CSubjectIsnt(actor,true,true); " on ", (the) x1, " at the moment.";

With the fix in place, we get:

I’ve submitted a PR on github with these fixes.