Feature - Implemented JavaScript: Remove custom valueOf() implementation for enumerated types

[philmasterplus]

Currently (r20617), there are three ways of retrieving the numeric ID of an enumerated data type (e.g. Item, Effect, Familiar):

1. Using the toInt() library function
2. Using the id property (only available for a few types)
3. Using the custom valueOf() method

The last method allows an enumerated type to be converted to a string or number, depending on context:

let item = Item.get('hair spray');
Number(item); // 744
+item;        // 744
String(item); // "hair spray"
print(item);  // Prints "hair spray"

It also allows types to be implicitly compared by ID:

Item.get('cold wad') > Item.get('hot wad'); // true
Item.get('cold wad') < Item.get('hot wad'); // false

While this may be convenient, it may cause surprising behaviors, now and in the future. Thus, I propose removing the custom valueOf() implementation altogether.

Why remove a good feature?

Because we are sitting on a bomb.

Consider this expression:

Item.get('pail') + someVar;

What should the item be converted to, a number or a string?

According to the ECMAScript 6 Language Specification†, the runtime must first convert both operands to a primitive value using a ToPrimitive() operation. How ToPrimitive() works deserves its own article, but the gist is:

1. Call obj.valueOf(). If it returns a primitive value, use it.
2. Otherwise, call obj.toString(). If it returns a primitive value, use it.
3. Throw a TypeError.

In step 1, the JS runtime should call item.valueOf() first. Since it returns a primitive value (the number 294), it does not proceed to step 2. Therfore, our pail must always evaluate to the number 294, regardless of the value of someVar. It should never evaluate to the string "pail".

But it just "works" in KoLmafia! What is going on?

This is caused by a bug in Rhino. To observe this bug, evalute this code in r20617:

Item.get('pail') + "foo";           // "pailfoo" (technically a bug, even though it's what we want)
Item.get('pail') + String("foo");   // "294foo"  (correct)
let someVar = "foo";
Item.get('pail') + someVar;         // "294foo"  (correct)

Since this behavior is not standards-compliant, it cannot be reproduced in other JavaScript runtimes. This undermines development and testing scripts in environments such as Node.js.

Meanwhile, a contributor has already implemented a fix. Should this make it into the next release of Rhino and we choose to integrate it, it would break all JS code that expects item-with-string concatenation to "just work".

This alone is a strong reason to ditch valueOf() as it stands today. If we delete it, the ToPrimitive() operation will always convert enumerated types to strings, thereby eliminating the potential for writing incorrect code that relies on accidentally-correct-but-not-guaranteed behavior.

† I also checked the ECMAScript 5.1 spec, which is worded differently, but the important parts are the same.

 

[gausie]

Thank you for the writeup. As much as I can follow this seems like the smart thing to do. I expect @ikzann (and now you) are the only ones familiar enough with the nitty-gritty of the implementation to really contribute meaningfully

 

[fronobulax]

Ok. I understand this, kind of.

I agree with the general idea - we allow something wonky because of someone else's bug and bad things will happen when they fix it so we should do something now

AFAIK this is only an issue for JavaScript users. Non-scripters and ash/cli only scripters will not notice the problem or the fix. If that is true then Go For It. In general if it only effects the JavaScript community then I trust that community to figure out what the right thing is and do it and I am inclined to support that.

So is the solution to delete public int valueOf() from EnumeratedWrapper, release the fix and then be nice when people complain it broke their scripts or is some other solution being proposed?

 

[philmasterplus]

Here's a patch that removes the custom valueOf() implementation for all enumerated data types. Since all enumerated types now inherit Object.prototype.valueOf(), they are always transformed to a string by the ToPrimitive() operation.

This patch breaks almost all JS code that relies on numeric conversion via Number(item) or +item. Such expressions will now always return NaN, unless its string representation happens to be a number. (Is there an item whose name is just a number?)

This also "fixes" the Rhino bug, since the Addition Operator will always perform a string concatenation when given an enumerated type.

What do we lose by this patch?

JS script authors can no longer use Number() to retrieve the ID of enumerated types. There are alternatives:

• 9 out of 16 enumerated types can be converted to their integer IDs via the toInt() library function. It's significantly slower than Number()--casual testing indicates a 10-15x reduction in speed.
  These include: Class, Effect, Familiar, Item, Monster, Servant, Thrall, Vykea.
  • Some of these types also expose an id property, which can be used instead of toInt(). Fortunately, these are about as fast as the old Number() conversions.
    They include: Monster, Servant, Skill, Thrall, Vykea
• 6 out of 16 enumerated types did not have meaningful integer IDs; valueOf() always returned 0 for them anyway, so no loss here.
  These include: Bounty, Coinmaster, Element, Location, Phylum, Stat
• Slot is an interesting case, as valueOf() previously provided access to integer IDs that we could not access from within ASH. Since these slot IDs were never used in ASH scripts, I doubt we're losing anything of value here.

Note: Should the function call overhead become a serious issue, one alternative is to add the IDs as properties on the underlying ASH types themselves, e.g. $item, $skill, $effect. Another choice is to simply add a toInt() method on the JS side only.

 

[fronobulax]

r20620