Bug - Fixed JavaScript code cannot call custom ASH function under special circumstances

[philmasterplus]

• KoLmafia version: r20777

When calling a custom ASH function from JavaScript, the function call is "skipped" when the ASH function meets certain conditions.

I have an ASH script named test-lib.ash that defines a function:

// test-lib.ash
void speak(string message) {
    int unused;
    print(message);
    return;
}

...and a JavaScript script that imports test-lib.ash:

// test.js
const { speak } = require("test-lib.ash");
speak("Called by JS 1");
speak("Called by JS 2");

When I execute test.js, only the first call to speak() is executed:

 > test.js
Called by JS 1

For comparison, I also wrote another ASH script that also imports test-lib.ash:

// test.ash
import <test-lib.ash>
speak('Called by ASH 1');
speak('Called by ASH 2');

Running it works as expected:

 > test.ash
Called by ASH 1
Called by ASH 2

I experimented a little bit with the code in speak(), and the following conditions appear to be necessary to trigger this bug:

1. The first line must declare a variable without initializing it
2. The last line must be a return statement, although the return type is not important

P. S. I discovered this bug while attempting to import and call ZLib's kmail() function in JavaScript code.

 

[katyarn]

I tried it out and was able to reproduce on r20779 using AdoptOpenJDK-16.0.1+9 (build 16.0.1+9).

 

[heeheehee]

I had slightly modified test.js when tinkering with this.

Interesting difference in `debug ash on` output for the two:

Starting script: test.js
Executing top-level commands
   [NORMAL] <- 0
      Param #1: arg
      [NORMAL] <- "called by js 1"
      Entering function print
      Function print returned: void
   [NORMAL] <- void
   [RETURN] <- void
   [RETURN] <- 0
   [RETURN] <- 0
Finished script: test.js
Starting script: test.js
Executing top-level commands
      Param #1: arg
      [NORMAL] <- "called by js 1"
      Entering function print
      Function print returned: void
   [NORMAL] <- void
   [RETURN] <- void
      Param #1: arg
      [NORMAL] <- "called by js 2"
      Entering function print
      Function print returned: void
   [NORMAL] <- void
   [RETURN] <- void
      Param #1: arg
      [NORMAL] <- "called by js 3"
      Entering function print
      Function print returned: void
   [NORMAL] <- void
   [RETURN] <- void
Finished script: test.js

In particular, it looks like after the first call, the ASH function in JS-land is returning a different value -- specifically, the default value of the declared variable.

Some more experimenting:

I checked to see if I could reproduce via a recursive call within ASH, then via indirection from another ASH function afterwards. No dice. And, reloading the function between invocations causes it to work as expected:

var { speak } = require("test-lib.ash");
speak("called by js 1");
var { speak } = require("test-lib.ash");
speak("called by js 2");
var { speak } = require("test-lib.ash");
speak("called by js 3");

prints all three times.

 

[heeheehee]

judicious print debugging indicates that the AshRuntime gets into the RETURN state and then never gets reset.

interpreter state: NORMAL
command: unused
interpreter state: NORMAL
interpreter state: NORMAL
command: print()
Called by JS 1
interpreter state: NORMAL
interpreter state: NORMAL
command: return null
interpreter state: RETURN
interpreter state: RETURN
command: unused
interpreter state: RETURN
interpreter state: RETURN
command: unused
interpreter state: RETURN

(I'm going to bed now if anyone else wants to continue poking where I left off)

 

[heeheehee]

Observations:

When there's a variable assignment (as opposed to a declaration), we invoke AshRuntime.captureValue(), which calls setState(NORMAL). This is done intentionally to swallow errors, e.g. if use() fails for whatever reason.

When there's a FunctionCall (e.g. print, happens within ASH), at the end of the call, we call setState(NORMAL).

When there's a FunctionReturn (ASH processing of the `return` statement), we call setState(RETURN), unsurprisingly.

When there's a variable declaration, we have no such implicit call to setState.

In BasicScope, when it executes on a statement and notices that the interpreter state is not NORMAL, it breaks out of the loop and returns that latest result, which matches the observation that the return type mutated from void to int.

In AshStub (used by the JavascriptRuntime), we invoke the UserDefinedFunction directly, which bypasses the setState(NORMAL) in FunctionCall. Now, UserDefinedFunction ensures that the type of the return value matches the return type of the function: if it doesn't match, then it returns the default value of the type (this.type.initialValue() if you're looking at the code).

I think the (logically) correct fix is to wrap the UserDefinedFunction in a FunctionCall. There are a number of other possible workarounds that I saw, like implicitly converting the declaration to an Assignment with default value as the rhs, or resetting the state in BasicScope, but I'm not confident those changes would be self-contained.

 

[heeheehee]

Initial implementation attempt; if I had more time I'd add a test and then commit it myself.

But, feel free to try it out.

 

[philmasterplus]

Huh. @gausie recently pointed out that function calls in ASH have different semantics depending on their position (standalone expression vs rhs of an assignment). E.g. retrieve_item(a); will abort immediately on failure, but boolean stuff = retrieve_item(a); won't.

I personally dislike this and would prefer it gone altogether.

Edit: I found an explanation of this behavior on the wiki.

 

[Veracity]

ASH is the "Advanced Script Handler".
It was intended to write "scripts".
A "script" - as in a "shell script" - aborts execution when a statement has an error.
The expectation is that your program's state is not what it "expects" and you will end up with cascading errors if you simply continue, so, abort.

Only some functions go into an ERROR state if they "fail".
Those tend to return a "continue value" - a boolean - to indicate if the function failed.
Thus: failure = ERROR state and false, success = no ERROR state and true.

Many years ago, I recognized that if a script "captures" the return value of a function call, it is declaring intent to handle an error.
Therefore, if a function call fails, ASH goes into an ERROR state, but if you use the return value of the function - store it in a variable, use it in an experession or conditional, whatever - ASH clears the ERROR state.

I'm sorry you "personally dislike this".
I see no reason to break thousands of scripts to soothe your feelings.

 

[philmasterplus]

@heeheehee Thanks, I'll try the patch tomorrow when I have the time.

@Veracity Thanks for the info. I already suspected that there was a reason why ASH was implemented so. I already knew that a change is impossible since plenty of code depend on the current behavior. Looking back, I regret that I was unintentionally rude; I'll try to be more careful with my language.

 

[philmasterplus]

Initial implementation attempt; if I had more time I'd add a test and then commit it myself.

But, feel free to try it out.

I tested your patch and it makes the code in #1 work correctly. It also made sending multiple kmails possible:

const { kmail } = require('zlib.ash');
kmail(myId(), 'test1', 0);
kmail(myId(), 'test2', 0);

I ran my daily farming script and Philter--both are JavaScript scripts that rely on many builtin and user-defined ASH functions--and saw no issues.

Note: Your patch does not affect the behavior of boolean-returning ASH functions called from JS code--they still throw instead of returning false. Changing this is not a high priority for me, though.

 

[gausie]

Note: Your patch does not affect the behavior of boolean-returning ASH functions called from JS code--they still throw instead of returning false. Changing this is not a high priority for me, though.

I think we fix this by making sure `.captureValue()` is run on every function call in JS

 

[gausie]

I think we fix this by making sure `.captureValue()` is run on every function call in JS

Done in r20780

 

[philmasterplus]

Done in r20780

Thanks. I just checked r20780 and it fixes both the original issue (skipping function calls) and the throwing-instead-of-returning-false problem.

 

[gausie]

Oh really? I didn't include @heeheehee's patch - did you? Or is it just not necessary now

 

[philmasterplus]

I actually tested using r20781, but the following code now sends two kmails instead of one:

js const { kmail } = require('zlib.ash'); kmail(myName(), 'test1', 0); kmail(myName(), 'test2', 0);

Your fix didn't include heeheehee's patch, but somehow killed two birds with one stone.

 

[heeheehee]

As I mentioned above, captureValue implicitly invokes setState(NORMAL), so this was another option for working around the underlying issue.

That wiki page is imprecise -- it's not caused by an ASH function returning false, it's when it leaves Mafia in an error state. Plenty of user-defined functions return false without aborting execution, and ditto with built-in ("library") functions (have_skill() and boolean_modifier(), to list a couple that never do).

Note: Your patch does not affect the behavior of boolean-returning ASH functions called from JS code--they still throw instead of returning false. Changing this is not a high priority for me, though.

That was not the intention of the patch.

I think we fix this by making sure `.captureValue()` is run on every function call in JS

I don't agree with this.

Yes, you may dislike this behavior. But it's effectively an alternative for ASH's lack of robust exception handling. JS does have actual exceptions, so we have the option to throw some form of Exception, instead.

Now, if JavaScript had something like C++17's [[nodiscard]], then I'd suggest the use of that for the relevant ASH functions and continue on our way (so if you want to ignore exceptions, then you need to do that very explicitly, and if they fail, that's on you). But we don't. If we have a script that uses the bare

eat(1, Item.get("fortune cookie"));

as part of a sequence of commands when hunting for semirares, should we just proceed with the rest of the script as if nothing happened? Adventure in the wrong location?

"That's on the script writer!" you might insist. "If it's intended to be used as a precondition, well, there's a return value for a reason. Even in a Bash script, if a single line fails, it'll keep going unless it explicitly calls exit."

Here's the thing, though: there are multiple kinds of reasons why that above line can fail. The underlying retrieveItem could fail (not enough meat, NPC store not available). One or more underlying requests to the server could time out, generating an IOException. You could have incidentally queued up a consumption helper like a salad fork, and not had enough HP to survive. What if it's part of a broader sequence of actions?

eat(1, SALAD_FORK);
eat(1, EXTRA_GREASY_SLIDER);
useSkill(1, COCOON);
chew(1, INSTANT_KARMA);
adventure(1, NOOB_CAVE); // get pantsgiving fullness
eat(1, FORTUNE_COOKIE);

We don't have support for this level of nuance in ASH (wherein we can only capture that there was an error), but, in JS, we can do better. Some errors can be retried (after careful inspection of state to check whether the request actually made it to and was processed by the server); others cannot.

</rant>

 

[philmasterplus]

That wiki page is imprecise -- it's not caused by an ASH function returning false, it's when it leaves Mafia in an error state. Plenty of user-defined functions return false without aborting execution, and ditto with built-in ("library") functions (have_skill() and boolean_modifier(), to list a couple that never do).

Looks like the section was added a long time ago by @Bale. Is there a list of "assertive" functions that exhibit this behavior (i.e. sets interpreter state to RETURN)? Or would I have to dive into LibraryFunctions.java and check each method?

I appreciate your idea, but I believe exceptions make sense only if they're good. Previously, ASH runtime library functions threw a string "Script interrupted.". It wasn't even a proper Error object. That is no better than returning false, since exceptions cannot be readily composed with boolean algebra.

Even if the functions threw proper exceptions, they wouldn't be very useful. JavaScript is known for its anemic exception classes (c.f. Python). Other than the error message, exceptions provide very little context on what spawned them. Also, Rhino does not store the stack trace on the exception object; we lose the original stack trace the moment we catch an exception. This makes it hard to locate the origin of the exception. As it is, exceptions are little better than fanciful abort()s.

Also, I'm not sure if most scripters want to go that deep into error handling. Many functions do more than what they say on the tin: drink() will check your status and issue appropriate warnings for overdrinking, Ode, mafia ring, etc. We generally assume that KoLmafia is smart enough to "just work" most of the time (and it does, thanks to everyone working tirelessly on it!). Where it fails, we assume human intervention is necessary. Resilience is a lofty goal when the underlying game is >15 year old and constantly evolving.

I believe exceptions need a lot of work to get right. I'd be impressed if someone actually put in that work.

 

[heeheehee]

Every return statement sets the state to RETURN. It's just usually cleared up via the next unit of execution in FunctionCall.

What you really want to look for is the EXIT state. In particular, we currently have poor support for propagating errors, I agree. Complicating the situation, generally, library functions do not set the EXIT state themselves (with the exception of abort); rather, that's a check of Mafia state (KoLmafia.permitsContinue()) at various points, most notably in parsetree.BasicScope.

An oversimplification is that any function that performs IO can encounter errors. Not all of those are from IO, though -- for instance, retrieve_item can fail without actually making any server requests if it decides it can't satisfy your request no matter where it looks.

Where it fails, we assume human intervention is necessary.

This reads as an argument for "unhandled errors should be propagated, not ignored".

And again, if we had a [[nodiscard]] analogue, I'd be all for that approach, as it visibly shifts the burden onto the scriptwriter and serves as an additional check that can prevent bugs.