Closed DavidGriffith closed 8 years ago
DavidGriffith
commented
8 years ago Here's the 6/11 form:
Release 1 / Serial number 160420 / Inform v6.33 Library 6/11 SD
...
>take rocks
rock: Taken.
rock: Taken.
rock: Taken.
rock: Taken.
>take all
stone: Taken.
girl: I don't suppose the girl would care for that.
>take rocks
That seems to belong to the girl.Here's the test program again. If we give the stone a before routine, the commands "TAKE ROCKS, TAKE ALL, TAKE ROCKS" will complete as though nothing was wrong.
Constant Story "GRAMMAR AND ORDERS";
Constant Headline "^An Interactive Bug Reproduction^";
Constant DEBUG;
Include "parser.h";
Include "verblib.h";
!statusline time;
Object Start_Room "Somewhere"
with description "You're not sure where you are.",
has light;
Class Rock
with name "rock" "rocks//p",
short_name "rock",
plural "rocks",
has supporter;
Rock ->;
Rock ->;
Rock ->;
Rock ->;
Object -> stone "stone"
with name "stone",
! before [;
! Take: "Nope.";
! ],
has supporter;
Object -> girl "girl"
with name 'girl',
orders [;
WaveHands: "The girl waves energetically.";
Take:! print (name) noun, "^";
<<Take noun, actor>>;
Drop:! print (name) noun, "^";
<<Drop noun second, actor>>;
PutOn: <<PutOn noun second, actor>>;
],
grammar [;
if (verb_word == 'jump') {
action = ##WaveHands;
rtrue;
}
! Didn't understand any of this, bailing out.
],
has animate female transparent;
[ Initialise;
location = Start_Room;
"It is time to do some bugfixing...";
];
Include "grammar.h";Fixed by @vlaviano in 42bf2225bbde58938a3cc1270a08281caf27d905
vlaviano
commented
8 years ago I've finished looking this issue, which we discovered 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
>take rocks
[** Programming error: tried to test "has" or "hasnt" of nothing **]
nothing seems to belong to the girl.This is a multilayered issue, but I've chosen to address it in a minimally invasive way consistent with our goal of stabilizing the library for the upcoming 6.12.1 release. The underlying issues are discussed in a longer version of this comment on the intfiction.org forums.
Let's start with the most obvious problem. 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 LM 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 LM(##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 LM(##Take, 6, ...) is testing an attribute of nothing. Why? Parser section I, the part of the parser that prints error messages, is calling LM(##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 this post 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
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). 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 LM 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 LM(##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.
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.)
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 "The x2 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:
if (noun has animate) L__M(##Miscellany, 44, verb_word);With this fix in place, we see the following output with our test program:
GRAMMAR AND ORDERS
An Interactive Bug Reproduction
Release 1 / Serial number 160506 / Inform v6.33 Library v6.12.1pre SD
Somewhere
You're not sure where you are.
You can see four rocks, a stone and a girl here.
>take all from girl
There is nothing to take.
>take rocks from girl
You can't see any such thing.
>take rocks
rock: Taken.
rock: Taken.
rock: Taken.
rock: Taken.
>g
There is nothing to take.
>take all
stone: Taken.
>take rocks
There is nothing to take.
Reported by Vince Laviano at http://www.intfiction.org/forum/viewtopic.php?f=7&t=19868&start=40#p108543