AnssiR66 / AlanStdLib

The Standard Library for ALAN Interactive Fiction Language
Other
5 stars 2 forks source link

BUG: Giving Worn Clothes to Actors Crashes Alan #57

Closed tajmone closed 5 years ago

tajmone commented 5 years ago

[ LINKS UPDATED AFTER BRANCH DELETION AND MERGE ]

I've uncovered a nasty bug affecting clothing, which causes Alan to crash:

SYSTEM ERROR: Stack is not empty in main loop

The crash was tested and confirmed with different Alan SDKs, both latest relea and dev snapshot:

It affects both the StdLib and the Italian Library, and most likely it's a problem rooted in Alan which surfaces when using clothing features of the library.

The test files that show this bug can be found in commit 2e5f8c3 093e551 of the temporary clothing-tests-prep work branch:

The crash always occurs when examining an actor after having given him a clothing item that was being worn by the hero:

> inventory
You are empty-handed. You are wearing your undershirt, your sneakers, your 
boxers and your socks.

> examine boxers
Just your boxer shorts (the loose type), white and plain.

> ; NOTE: boxers are currently bing worn by hero!
> give boxers to assistant
(taking your boxers first)
You give your boxers to the assistant.

> ; Now comes the crash (using 3.0beta6):
> examine assistant
She's your personal shopping assistant. The assistant is carrying your 
boxers (being worn). 

As you enter the twilight zone of Adventures, you stumble and fall to
your knees. In front of you, you can vaguely see the outlines of an
Adventure that never was.

SYSTEM ERROR: Stack is not empty in main loop
At source line 655 in 'clothing.alan':
WHEN hero AT dressing_room

<If you are the creator of this piece of Interactive Fiction, please help
debug this Alan system error. Collect *all* the sources, and, if
possible, an exact transcript of the commands that led to this error, in
a zip-file and send it to support@alanif.se. Thank you!>

The referenced line 655 in the error seems unrelated to the actuall problem, for this can be reproduced in any adventure created with the StdLib (English and Italian alike), so its cause seems related to the transfer of worn clothing items.

AnssiR66 commented 5 years ago

Seems the 'give' verb must be modified with clothing items. In this code example, the hero gives the boxers to the assistant but when the hero examines the assistant, the assistant is already wearing the boxers, not just carrying them. I'll take a look. -Anssi


From: Tristano Ajmone notifications@github.com Sent: Tuesday, January 29, 2019 11:14 AM To: AnssiR66/AlanStdLib Cc: Subscribed Subject: [AnssiR66/AlanStdLib] BUG: Giving Worn Clothes to Actors Crashes Alan (#57)

I've uncovered a nasty bug affecting clothing, which causes Alan to crash:

SYSTEM ERROR: Stack is not empty in main loop

The crash was tested and confirmed with different Alan SDKs, both latest relea and dev snapshot:

It affects both the StdLib and the Italian Library, and most likely it's a problem rooted in Alan which surfaces when using clothing features of the library.

The test files that show this bug can be found in commit 093e551https://github.com/AnssiR66/AlanStdLib/commit/093e551 of the temporary clothing-tests-prephttps://github.com/AnssiR66/AlanStdLib/tree/clothing-tests-prep/tests work branch:

The crash always occurs when examining an actor after having given him a clothing item that was being worn by the hero:

inventory You are empty-handed. You are wearing your undershirt, your sneakers, your boxers and your socks.

examine boxers Just your boxer shorts (the loose type), white and plain.

; NOTE: boxers are currently bing worn by hero! give boxers to assistant (taking your boxers first) You give your boxers to the assistant.

; Now comes the crash (using 3.0beta6): examine assistant She's your personal shopping assistant. The assistant is carrying your boxers (being worn).

As you enter the twilight zone of Adventures, you stumble and fall to your knees. In front of you, you can vaguely see the outlines of an Adventure that never was.

SYSTEM ERROR: Stack is not empty in main loop At source line 655 in 'clothing.alan': WHEN hero AT dressing_room

<If you are the creator of this piece of Interactive Fiction, please help debug this Alan system error. Collect all the sources, and, if possible, an exact transcript of the commands that led to this error, in a zip-file and send it to support@alanif.se. Thank you!>

The referenced line 655 in the error seems unrelated to the actuall problem, for this can be reproduced in any adventure created with the StdLib (English and Italian alike), so its cause seems related to the transfer of worn clothing items.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/57, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHtgHrkF2Q-Y5DuGLdnP4K1toenOPks5vIBD8gaJpZM4aXmgu.

tajmone commented 5 years ago

Notes on Approacing Solutions

Ok, I want to jot down some notes here before tweaking the library code, for there are a variety of aspects to consider in handling transferal of worn items, so it's best to do some brainstorming first.

I just want to make sure that we don't entangle things up, because there seem to be already quite a few places in the code that are handling/manipulating clothing!

All links to library sources provided here will refer to the latest current commit (7a7b6d8), so they'll always reference the current status of the code.

Also, I think that this might ultimately be an internal Alan bug. Even if we forgot to handle some attributes or moving clothing into/out of worn, this shouldn't cause a stack error crashing Alan (ie. Alan should protect memory to avoid this). But trying to understand why there is a strict correlation between clothing and the crash is indeed interesting.

Do you think the bug is on Alan side, in the library, or both?

Clothing Status

So, we have to keep in account:

Fix Proposals

I'm pasting here you'r proposals from the Yahoo list email:

Add Give Verb to Clothing Class

One problem is at least that when the hero gives the piece of clothing to an NPC, the NPC is then described as wearing the item, even if he should be just holding it. So, the item still is donned even if it shouldn't.

We need to add a give verb to the clothing class:

VERB give
  DOES
    MAKE THIS NOT donned.
    EXCLUDE THIS FROM wearing OF hero.
END VERB.

Here we need to consider carefully the possibility of the give verb failing, due to some tweaked verbs added by an adventure author (we already came across a similar problem in the past, with liquids switching vessels, see #39).

So, the safe bet here would be to use DOES AFTER and check that obj is effectively in the actor possesion.

Here we must be careful about the worn_clothing_check EVENT, which at every turn iterates all the clothing items IN the wearning of every actor, making the clothing donned and placing it either IN the NPC or IN worn if it's the Hero. In theory it shouldn't interfere, for the above fix should result in the clothing item not being in the wearing of any actor, but it's always good to double check that this is effectively the case, at the end of the turn.

Using EXTRACT to Fix Wearing/Donned

On the other hand, there might be other implicit taking verbs ("put (worn clothing) in box") etc that might cause a crash when examining the box? (can you try this out, too?) and that's why an EXTRACT statement that makes the item not donned/excludes it from the wearing set might come in handy for the clothing class.

Using EXTRACT seems a good idea, for it would work whenever a clothing item is moved out of a container. So, my guess is that we could place it on the ACTOR class (which already has an EXTRACT to check compliancy) and safely assume that any clothing leaving an actor container should be considered undonned.

My worries here are in case of verbs take are implicitly extracting the clothing item in order to make it worn by the Hero (or an NPC, in case of custom verbs by authors). Just want to make sure that the order of execution is such that even if the EXTRACT makes it undonned it won't interfere with the body of a verb that wants to make it donned (ie, that execution of a verb DOES block effectively comes in execution after the EXTRACT resulting from implicit taking).

thoni56 commented 5 years ago

I've been able to reproduce the underlying root cause for the crash, which of course should not happen whatever a library does...

It seems that it is the existence of the Extract which is triggered during the worn_clothing_check Event that is the culprit. I suspect that it tries to prevent the extraction ("That seems to belong to ...") and I don't really know what happens with the deep nesting in this situation.

The Extract clause was devised to prevent a player to take things out of containers, so when the Check triggers it might return to the wrong place.

I'll keep digging...

tajmone commented 5 years ago

The Extract clause was devised to prevent a player to take things out of containers, so when the Check triggers it might return to the wrong place.

I'm not sure I understand the full implications of this, but at least I do understand the area of the problem.

Mhhh ... this is going to be tricky, for it clashes with the EXTRACT solution I had in mind (see below).

Updated Test

I've updated the test to make it show more details about the clothing item before and after it's given to the NPC:

> dbg boxers
'boxers' VALUES: | botcover: 2 | 
DONNED: Yes 
IN WORN: Yes 
IN WEARING OF: | hero

> give boxers to assistant
(taking your boxers first)
You give your boxers to the assistant.

> ; Now comes the crash (using 3.0beta6):
> dbg boxers
'boxers' VALUES: | botcover: 2 | 
DONNED: Yes 
IN WORN: No. 
IN WEARING OF: | hero 

So, it looks like the problem here is that after handing the boxers to the assistant there are still marked as donned and in the Hero's wearing set — but no longer in the Hero's worn.

For this reason, the worn_clothing_check EVENT kicks in to ensure that the item gets backs into worn (it sees the item as present in the wearing set of Hero but not in worn). Probably it's not really what the player types that crashes the game, rather when this event kicks in and trigger the EXTRACT, as Thomas mentioned.

First thing to do here is to ensure that worn items leaving any actor become undonned, no longer in worn entity nor the actor's wearing set. As I explain below, this shold ideally be handled outside any library verb that might transfer clothes.

Using EXTRACT(s) to Handle Worn Items?

It looks like the cleanest solution to library problem would be to handle donned items via EXTRACT:

This approach would garantee correct clothing handling even with custom author verbs — whereas leveraging only the give VERB on clothing would not cover custom verbs added to an adventure.

But there is another problem here too, the VERB wear moves items in and out of worn when the action fails, to show the list of currently worn items. This would tigger the WORN EXTRACT and would have to considered.:

    IF wear_flag OF hero >1
      THEN
        IF THIS NOT IN hero
          THEN "You pick up" SAY THE THIS. "."
        END IF.

        LOCATE THIS IN hero.
        EMPTY worn IN tempworn.
        LIST tempworn.

        "Trying to put" SAY THE THIS. "on isn't very sensible."

        EMPTY tempworn IN worn.

Personally, I think that the EMPTY worn IN tempworn. part and also the code that handles CONTAINS_* branching to produces "(being worn)" in MESSAGES should be removed completely, for it only produces redundancy — "You are currently wearing: a hat (being worn), a shirt (being worn)" — and it's not used except in the wear verb.

thoni56 commented 5 years ago

To remove the extraneous manipulation of list seems reasonable, if for nothing else to clean up the code.

(This might be a bit out of scope and perhaps belongs somewhere else, alanif-group? manual? Please quote me or copy this there if you see a better place for it.)

I think the broader implication here is that Alan was created as a language to very closely "mimic", "mirror" the player level of narration. The deeper you go into using Alan as a general Turing-complete programming language, which it seems some parts of the library are close to doing here, the trickier it will be to get around limits imposed by that initial design idea.

E.g. Alan has containers, containers inherently have limits and extract checks on them so that the player should not attempt to add or remove stuff that the author did not want him to be able to do. But if the Alan "program" starts to manipulate, explicit or implicitly those containers, that abstraction, if you will, will start to break. The "program" might really want to move things in and out of containers without those rules applying, or maybe they should. There is no way to tell. And we don't want two statements for manipulation of containers.

Having said that, I think there is no other option than being aware of the "execution context" somehow.

(There are actually two problems here, one is the bug that happens since the Extract check jumps right out of the execution and leaves an unhandled stack, the other, which is the focus of this discussion, how should/could Extract be handled in "program" code.)

In the current example, what is the expected player experience? As the worn_clothing_check is an Event it is not actually the player doing this. So how about that "It seems to belong to..."?

Maybe the library has to cater for these cases by "knowing" which things to not move out of other containers.

As I'm not too familiar with the deep logic of the library, I'm unsure what the strategy is, on how moving things belonging to other people ended up in an event. So, please enlighten me, although there is probably a lot of background on the library design... E.g.

tajmone commented 5 years ago

Ciao Thomas,

(This might be a bit out of scope and perhaps belongs somewhere else, alanif-group? manual? Please quote me or copy this there if you see a better place for it.)

Not at all, for it actually brings up the question of why the library currently needs resorting to these type of parallel and cross item "hacks" to achieve this, so to speak.

I think that the nature of the problem is, as you pointed out, the separation between player commands and internal actions of the adventure. In this respect, one missing feature in Alan seems to be ability to trigger a VERB from within the code, so that an action could be carried out as if invoked by the player.

For example, implicit taking could be carried out by the "give" verb by triggering the "take" verb. This is actually the limitation we are facing in this very context, for we have no way of implicitly disrobing a clothing item in the course of the "give" verb (or any verb that would move the item away from the actor's wearing it).

So, probably, if Alan were to allow invoking verbs from within code the whole codebase of the library would be rather different. Of course, triggering verbs from code might also introduce new problems (eg. should these be executed in "silent" mode regard messages they might produce?).

The "program" might really want to move things in and out of containers without those rules applying, or maybe they should. There is no way to tell. And we don't want two statements for manipulation of containers.

This is a question which should be discussed with whole community, for it's always delicate when it comes to tinkering with Alan behavior. One of the great things about Alan is that it allows total freedom in creating adventures by providing a clean-slate approach to the adventure world. No other IF authoring system allows this, the only exception being TADS — but then, since in TADS the grammar parser is part of the Standard Library, a clean-slate approach would require high expertise to be put into practice.

The problem with libraries is that they tend to limitate that freedom in a manner which is directly proportional to the amount of features and complexity they introduce. So, if on the one hand a library simplifies authoring, on the other it also imposes a "philosophy" on how an adventure should be structured/desgined. But I think that the StdLib has always been clear about this, and it always encouraged authors to customize it, and use it as a starting point rather than a fixed means.

So, what might seem reasonable in the context of the StdLib might not be seen as something good by free-form authors who have different approaches the adventure world.

But to answer your original question:

As I'm not too familiar with the deep logic of the library, I'm unsure what the strategy is, on how moving things belonging to other people ended up in an event. So, please enlighten me, although there is probably a lot of background on the library design...

Handling moving items ended up in EVENTs because Alan doesn't provide a means to trigger VERBs from code. If VERBs were executable from the library code (like they are in many IF systems) then implicit actions would all go through the same code, and we wouldn't have to deal with out-of-synch elements that need to be intercepted at various points.

Before "giving" a clothing items to another actor, we could "remove" it implicitly, and then all the attributes would be fine.

At least, that is my understanding of this situation, having studied the code, but I can't speak for Anssi of course.

thoni56 commented 5 years ago

Thank you, for that explanation.

I'm sure there is more to the "event strategy" than being able to trigger verbs from code, because you can always duplicate the code of the verb, right? Granted, that would be a lot of duplication, so I'm not saying that the library should do that instead, just trying to figure out what is, or are, the root problems here.

Perhaps duplication is one reason to move this code to an event, I know there has been discussions about a "procedure" that you can call, and I think the current best thinking is to use an event for this.

When you say, trigger Verb, which part of the verb handling are you actually after? The Checks? The resolution through the class inheritance tree? The additive behaviour of Checks and Does clauses? Or is it just a few lines in the specific Does clause of the basic "Take" verb that you want to call?

tajmone commented 5 years ago

I would also add to the above considerations:

The problem is not so much ensuring that library verbs manipulate attributes correctly — that would be easy to achieve, we'd only need to replicate the code from remove to give.

The problem is ensuring that custom author verbs added to an adventure won't break up handling of special classes (like clothes). The whole idea is to take off the burden from authors by having the library handle in the background chores related to the classes it introduces.

Let's say an author wants to add a new verb "tear clothes" (eg. as an act of keriah in a Jewish funeral). This action implies removing the clothing item, and if the author forgets to do it, the item will remain worn by the Hero (or the NPC) and in his wearing set.

Forcing authors to have to handle library internals would impose quite a burden on end users, which kind of defies the whole purpose of using a library to simplify authoring. Of course, at times this is inevitable with specialized classes with complex behavior. But, whenever possible, the library tries to handle these chores in a manner that is safe in view of (unknown) custom verbs.

EVENTs are one the most viable tools to achieve this, lacking invocation of VERBs from code.

Another feature which might help here would be having "functions", so to speak, ie. block of code that could be invoked anywhere from code for immediate execution. That would ensure reusability of snippets across the library, and that the order of execution of those snippets is controllable (unlike Events, which could end up executing in out-of-synch order).

Informing author about the need to invoke specific code "functions" when handling entities of special classes would not be a huge burden. Since this functions would be sharable by library- and user-code, they would allow to break up VERBs in smaller chunks and avoid redundancy of code.

If functions were to introduce to much of a complexity, then having MACROS to replicate shared code could be a viable alternative (define once, use anywhere), although they would add some overhead in code/memory size.

I think that this problem of snippets reusability (whether by allowing invoking verbs from code, or introducing functions or macros) is a general problem that affects not only the library but Alan adventures in general, and I've seen the issue come up more than once in features requests. Of course, in a big codebase like the StdLib, which also aims to be extensible by end users, this problem is felt with more urge than free form adventures.

tajmone commented 5 years ago

When you say, trigger Verb, which part of the verb handling are you actually after? The Checks? The resolution through the class inheritance tree? The additive behaviour of Checks and Does clauses? Or is it just a few lines in the specific Does clause of the basic "Take" verb that you want to call?

That's difficult to answer. And probably having aribtrarily executable code snippets would be an easier solution, for then we'd only need to move the shared parts of a VERB into such "functions" (or MACROS).

I'd be happy with having MACROS, not really worried about code duplication at all (memory not being a real issue nowadays), and probably macros would be easier to implement via some preprocessor, instead of altering Alan internals.

Something like:

MACRO remove_clothing_item
    -- some code here.
END MACRO.

and then the macro could be usable anywhere by just mentioning it's ID:

VERB give
  DOES
    remove_clothing_item

or some other syntax (eg: #remove_clothing_item, etc.) which would make it clear that there is macro involved.

thoni56 commented 5 years ago

Good point, with being "immune" to author additions.

But doesn't that again bring up the question on how to proceed when the "action" aborts, like is/was the case with Extract. If the player is "calling" the Verb a failed check should, and I think, must abort the execution. It would be very strange if the execution continued with other pending statements in this case. But if a library does some manipulation, calls a Verb and the check fails, should that be allowed to continue? Otherwise the world might be in disarray when the library thought it was finished.

That sounds like standard exception handling. Perhaps that is a thought to explore.

Again, two issues, function/macros/snippets, and handling aborted execution from "calling a Extract/verb/function/macro/snippet".

tajmone commented 5 years ago

Well, now that you mentioned the issue of aborting VERBs execution, I might add to this that currently the situation with VERBs DOES [AFTER|BEFORE] is lacking some way to explicitly abort.

Let's say I need various classes to execute some DOES AFTER code after a standard verb, just to handle some extra bits but without having to rewrite the whole code from the main verb's DOES block. Currently there isn't a way to abort the execution chain of VERBs. Either one uses a DOES ONLY, or the whole chain of AFTER verbs will be executed in heritance order.

As for MACROS, I'd say that it's implicit that author should use them wisely, and only as a means to control same code in different places by a single definition point. The idea of a macro would be twofold:

Obviously, the assumption here is that every macro should be created with a specific use in mind, and that they'd be safe to use only in the intended context.

I was thinking that a macro could be used to handle attributes, etc. But of course they need to rely on parameters names which are shared across the places where the macro will be used (eg. THIS or obj). Probably this would not provide a universal solution, but this is a genral problem with macros.

Some languages (eg. Fasm assembly) provide complex macros systems, allowing parameters and nested macros, conditional branching macro definitions, etc. But these would probably be to complex to use, as well as implement.

FUNCTIONs, on the other hand, would probably allow using parameters in a more explicit way when invoked, so they could handle some cases that macros can't. Eg., THIS inside the function would refer to its parameter of invocation, allowing them to be used to reference different instance during execution.

I'm not saying any of this is simple to implement, but definitely this is an area of Alan which is worth addressing with one solution or another (probably there are simpler solutions, I just can't think of any).

AnssiR66 commented 5 years ago

Hi, popping into the conversation in the middle but anyway : )

I will look into the clothing code as soon as possible. There should be a way to make the clothing items to be transferred to the recipient correctly without too much hassle. I mentioned Extract in passing, because that came to mind, but there might be simpler ways. And especially if it conflicts with the way Alan is meant to work, like Thomas mentioned, we can find alternative routes.

I would keep the "(being worn)" for now, because it is especially needed when examining NPCs:

x man The man is carrying a bag, a key and a grey suit (being worn).

The library distinguishes only between what the hero is carrying or wearing:

x me You are carrying a bag and a key. You are wearing a grey suit.

but with NPCs the description only mentions carrying. With the hero, the "being worn" should never show, so if that happens, it needs fixing.

The above as a short comment on the points you took up.

As to functions: The closest way that it is possible at present to imitate functions is to make an instance and give it a description:

THE remove_clothes ISA THING DESCRIPTION FOR EACH cl ISA CLOTHING, IN hero DO MAKE cl NOT donned. EXCLUDE cl FROM wearing OF hero. EMPTY worn. END FOR. END THE.

and then call this 'function' by "DESCRIBE remove_clothes." etc.

But here it is difficult to refer to the exact object the player referenced, it can only be on a general level, like above when removing all clothes, etc... as far as I can see.

(it could even be possible to say "Every Function Isa Thing", and use functions that way (The remove_clothes Isa Function) if you don't want to use 'thing' etc.)

I'll experiment for my part and let you know if there is something I can present as a solution for the transfer of clothing items...

-Anssi

From: Tristano Ajmone notifications@github.com Sent: Wednesday, January 30, 2019 1:45 PM To: AnssiR66/AlanStdLib Cc: AnssiR66; Comment Subject: Re: [AnssiR66/AlanStdLib] BUG: Giving Worn Clothes to Actors Crashes Alan (#57)

Well, now that you mentioned the issue of aborting VERBs execution, I might add to this that currently the situation with VERBs DOES [AFTER|BEFORE] is lacking some way to explicitly abort.

Let's say I need various classes to execute some DOES AFTER code after a standard verb, just to handle some extra bits but without having to rewrite the whole code from the main verb's DOES block. Currently there isn't a way to abort the execution chain of VERBs. Either one uses a DOES ONLY, or the whole chain of AFTER verbs will be executed in heritance order.

As for MACROS, I'd say that it's implicit that author should use them wisely, and only as a means to control same code in different places by a single definition point. The idea of a macro would be twofold:

Obviously, the assumption here is that every macro should be created with a specific use in mind, and that they'd be safe to use only in the intended context.

I was thinking that a macro could be used to handle attributes, etc. But of course they need to rely on parameters names which are shared across the places where the macro will be used (eg. THIS or obj). Probably this would not provide a universal solution, but this is a genral problem with macros.

Some languages (eg. Fasm assembly) provide complex macros systems, allowing parameters and nested macros, conditional branching macro definitions, etc. But these would probably be to complex to use, as well as implement.

FUNCTIONs, on the other hand, would probably allow using parameters in a more explicit way when invoked, so they could handle some cases that macros can't. Eg., THIS inside the function would refer to its parameter of invocation, allowing them to be used to reference different instance during execution.

I'm not saying any of this is simple to implement, but definitely this is an area of Alan which is worth addressing with one solution or another (probably there are simpler solutions, I just can't think of any).

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AnssiR66/AlanStdLib/issues/57#issuecomment-458915190, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AF2lHr33fpHBsbF1pmWbDiFBczo_Rph5ks5vIYXRgaJpZM4aXmgu.

tajmone commented 5 years ago

[ LINKS UPDATED AFTER BRANCH DELETION AND MERGE ]

Anssi, I've update the tests to check how the put verb handles worn items, and I confirm it fails to handle them:

> DBG boxers
'boxers' VALUES: | botcover: 2 | 
DONNED: Yes 
IN WORN: Yes 
IN WEARING OF: | hero

> put boxers in basket
You put your boxers into the basket.

> ; ** ERROR!!! ** The boxers are still considered donned and worn by Hero:
> DBG boxers
'boxers' VALUES: | botcover: 2 | 
DONNED: Yes 
IN WORN: Yes 
IN WEARING OF: | hero

But further tests also show that the wear verb isn't handling them correctly either:

> DBG jeans
'jeans' VALUES: | botcover: 16 | 
DONNED: No. 
IN WORN: No. 
IN WEARING OF:

> wear jeans
You pick up the jeans and put them on.

> DBG jeans
'jeans' VALUES: | botcover: 16 | 
DONNED: No. 
IN WORN: Yes 
IN WEARING OF:

> ; ** ERROR! ** Wearing the jeans should have made them 'donned' and added them
> ;              to the 'wearing' set of Hero!
> 

We need to start checking and fixing various clothing specific verbs before attempting changes to solve the issue relating to the crash bug, otherwise things might get out of control. There are too many cross factors at play here, and we need to get straight the basic verbs first.

tajmone commented 5 years ago

Corrupt Memory Bug?

@thoni56,

An update to the tests reveals a new non-crashing bug related to clothing:

Where, initially (line 49), trying to remove a non existent item produces the expect respons from ARun:

> remove XYZZY
I don't know the word 'XYZZY'.

But after moving around some worn items (not properly handled) the same command produces no response (line 148):

> remove XYZZY

> ; ** ERROR!!! ** ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ;                Now 'remove' is failing silently, which seems to indicate that
> ;                somthing during the tests has messed up Alan memory!
> ;                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm not sure wether it's related to the same bug of EVENTs/EXTRACT, but I though of pointing it out.

tajmone commented 5 years ago

I've been running tests with the latest developer snaphshot Alan 3.0beta6 build 1866 (in the Italian library I'm now using alway latest snapshots to work) and noticed that the bug crash has now shifted place (it occurs in other point of the source code); so chances are that this problem might actually be solved.

I'm looked into the Alan commits history and have noticed that @thoni56 has been tackling this issue in various commits, trying to unwind the problem by changing various parts of Alan source.

So, a bit thank to Thomas for all the hard work, and let's see if this gets solved so we can use Events to handle clothing.

thoni56 commented 5 years ago

Yes, rather than keeping up with the conversation ;-) I have been working on understanding the core problem. And as I stated earlier, I think, it is the effect of the "thrown Extract" that leaves an unhandled stack, which in this particular context contains the loop variables for the "rest" of the iterations for manipulating the clothing containers.

I'm in no manner close to a solution, because I think it is a principle one. And I have no easy fix for that. (The case that the Extract should prevent and abort a player action of removing something from a container that shouldn't be allowed. And that we in this case want to do that "silently".)

If I just think about the logic here I would investigate a strategy where the library first tried to "get" the item if possible, which might abort by means of an Extract, and once that is done do all the manipulations using sets as an intermediary storage, sets not being possible to prevent, restrict or abort movement in and out of.

This does not solve the primary problem, what to do with an unhandled stack when returning to the main loop. I suspect that some games silently just have been running with a larger and larger unhandled stack. (Not that that is a real problem since each "layer" only takes a handful of bytes...)

tajmone commented 5 years ago

Thanks for the insight @thoni56.

unfortunately, since the AMachine is undocumented I have no idea of how these internals work. I imagine that some Alan keywords behave like functions, having a stack frame for their execution.

If I've understood correctly, the nature of the problem here is that the Event and the Extract don't share a common context (references to instances, etc.) and that there isn't an underlying system in AMachine to handle failing executions (in this case, for the Event to know that the Locate failed due to Extract preventing it)?

Or is just that unexpected cross-triggerings are causing mishandled stacks which are not being accounted for?

I'm trying here to think in terms of the Glulx VM, which I've studied a bit and have some idea of how it handles such cases — Glulx is the only model I can rely to in this context, for AMachine is a black box to me.

thoni56 commented 5 years ago

The AMachine is a stack machine pushing values on the stack which are then "ate" by the next instruction. Also local and loop variables are also kept on the stack in "frames". A nested loop would cause two frames to be used and some data space for the loop variables. If, as in this case, an instruction to move an instance from a container triggers an Extract the valuation of the Extract is done "on top" of the current stack. If nothing exceptional happens, the Extract will execute and exit normally, causing the next iterations of the innermost loop to be executed until that loop is executed and the inner "frame" popped of. If the outer loop is not also finished that "frame" will also be popped and the stack should be empty when, e.g., the event terminates.

What happens is that the extract "aborts" and does not return, but skips all the pending loops and returns to the main player loop without terminating/removing/handling the two "frames".

The issue here is should this ever happen? It is possible to just throw the pending frames away when aborting. But is this the right thing to do? Yes, probably in some cases, and no, in other cases.

It feels that the "clothing" problem is an instance that we would it more "correct" to actually keep looping and not aborting. But if it is the player that is more directly responsible for the removal from the container, then we probably would consider an abort the correct option.

I would probably have to learn more about what exactly the worn_clothing_check is actually trying to do, to help out in what other options there is to do that. But again, I would look into using sets instead of the containers directly, if possible. And remember that containment (being in a container) does not exclude being part of a set, so you could have a container with the actual clothing but at the same time include all those in a set which can be added to and removed from. Just let me know if you have tried that, and I'll stop mentioning this ;-)

There is a problem with the contexts of Extract, I know we had conversations about this in the past. I think that discussion got lost, so I'll have to think that through again to find a solution (and probably a fix) to access command parameters as well as the current actor, and both the instance being removed and the one being removed from. (I think the latter was the problem, $1 can only hold one of these, I wonder what this is inside an Extract...)

tajmone commented 5 years ago

Thanks for the detailed reply @thoni56! I'll have to read it thouroughly later on, because my dog is sick today and keeps asking me to take him out every 2 hours ... so today I'be on-and-off the PC like a yoyo.

I wonder what this is inside an Extract...

I've tested that, and using THIS always returns the CONTAINER. If there is a verb running, "$o" returns the parameter of the verb. But attempting to use "$1", etc., inside an EXTRACT won't even compile (errors regarding parameters not being available there).

I wonder if the "$o" (depreated) is supposed to be visible there or if it's just being dragged over from the current executing verb — and whether it's safe to use it at all.

thoni56 commented 5 years ago

Thanks. So this is doing what it should, designating the instance we are executing code in. Check.

$o is deprecated, but will probably work (in general) for a long(ish) while.

The problem with parameters is that it is impossible for the compiler to know which verb was executed to trigger the Extract. In fact, as we have seen in the original issue, it might not even be a verb that trigger the extract. So, no, $o is not safe, only difference to $n is that the compiler doesn't catch it.

Alan is built on the notion that the compiler should be able to know enough about the execution context to make through analysis possible. Extract is very problematic since there is no way to know where it was triggered from, except analysing every statement that move instances around to see if it might involve moving an instance out of the container, and then aggregate all those into some conclusion about the possible contexts.

I'm starting to think that Extract was a wrong decision. But removing it would force checks instead...

tajmone commented 5 years ago

I'm starting to think that Extract was a wrong decision. But removing it would force checks instead...

I think that keeping Extract is fine, as long as end users have a clear picture of its intended use and limits. Also, it's quite conceivable that the compiler won't be able to catch every possible compile time error, and warning and error message usually provide a good hint on the nature of the problem.

Again, I think that if users are trying to use some Alan keywords beyond their originally intended context is to workaround the lack of an explicit way to execute verbs (I'm thinking here of Inform's way to use instead to execute another verb instead of verb X).