Bug - Fixed Stdlib functions are non-configurable/writable when executing JS code via gCLI

[philmasterplus]

As of r20555, the gCLI command js injects KoLmafia's stdlib functions (e.g. print(), myAdventures()) into the global scope before execution. These functions are defined as non-configurable and non-writable properties (see initRuntimeLibrary() in net/sourceforge/kolmafia/textui/javascript/JavaScriptRuntime.java).

This causes a problem with JavaScript code that imports stdlib functions directly into the global scope:

// globals-test.js
const { print } = require("kolmafia");
module.exports.usefulFunction = () => { print("Hello, world!"); };

This script works flawlessly when executed with gCLI's call command. However, importing this inside a js command like this...

js require("globals-test.js").usefulFunction();

...results in an error:

JavaScript error: TypeError: redeclaration of const print.
at command line:1

Can this be solved if we make the stdlib functions configurable and enumerable? Well...

---

Digging deeper, it seems that Rhino does not allow re-declaration of a property if either the current or the new property is "const-ish". See org.mozilla.javascript.ScriptableObject.redefineProperty().

What is "const-ish"? In the same file, ScriptableObject.isConst() checks if the property is both PERMANENT and READONLY, which translates to "non-configurable and non-writable" in JavaScript. So a property is "const-ish" if it is both non-configurable and non-writable.

This behavior deviates from that of some popular browsers. I checked this JSFiddle on Chrome 87.0.4280.88, Firefox 83.0, and MS Edge 87.0.664.57. None of them had any qualms about me declaring const Number = 1, even though the built-in Number is configurable and enumerable.

Takeaway: Even if we make the stdlib functions configurable and enumerable, const { print } = require("kolmafia"); will still fail.

---

My proposed solution is twofold:

1. In JavaScript code, use var or let to import stdlib functions.
2. In KoLmafia, remove either the PERMANENT or READONLY flags on the stdlib functions.

We don't have to drop both; I verified that dropping either makes the following code work:

// globals-test-modified.js
// You can call this from the gCLI with:
//    js require("globals-test-modified.js").usefulFunction();
let { print } = require("kolmafia");
module.exports.usefulFunction = () => { print("Hello, world!"); };

However, having a property that is configurable but non-writable (or vice versa) doesn't make much sense. I therefore propose that we make them both configurable and writable. Since nearly all JavaScript built-ins (Number, Array, etc.) are configurable and writable anyway, I see little harm in following suit.

A complete, foolproof solution would also involve not injecting the stdlib functions into the scopes of require()-ed scripts. Unfortunately, this is beyond my expertise.

TL;DR: Make haveItem(), visitUrl, etc. configurable and writable, just like Number, Array, etc.

I'm posting a patch file with the proposed change.

 

[fronobulax]

r20557

I simultaneously appreciate and don't understand the level of detail

 

[philmasterplus]

Thank you. And it's fine, I don't understand half of it either. Modern JavaScript is weird.

 

[fronobulax]

Thank you. And it's fine, I don't understand half of it either. Modern JavaScript is weird.

My biggest concern was it sounded like the sandbox was being broken or given porous walls. But looking at the patch made that seem unlikely so...

 

[philmasterplus]

No worries--this issue has nothing to do with sandbox breaching. It involves the question: should a script be able to shoot itself in the foot by overriding a global function? Historically, JavaScript said yes, and we've come to accept it. This patch tries to conform to that behavior.

Fortunately, each script gets its own scope. If a script overrides a global function, only it sees the new function. One can shoot only oneself in the foot.