Bug - Fixed put_shop() coerces item quantity from float to int incorrectly

philmasterplus

philmasterplus

Active member
Update: This was not a JavaScript issue, but a problem with the ASH function itself. See reply #4 and below.
As such, I have renamed the thread title (previously "JavaScript: putShop() bug due to typing issues").

TL; DR: When calling putShop(), be sure to wrap the item quantity argument with toInt().

JavaScript's type system does not distinguish integers and floating-point numbers. Because of this, calling ASH functions (including KoLmafia's library functions) from JavaScript can lead to surprising behavior.

For example, putShop() will behave inconsistently if you pass a computed (i.e. non-literal) value as the item quantity:

JavaScript: 
// Passing a literal 1 as the quantity
putShop(0, 0, 1, Item.get('spices'));
// Works as intended

// Passing a variable as the quantity
let qty = 1;
putShop(0, 0, qty, Item.get('spices'));
// KoLmafia prints "Adding spices to the store...", but does not actually transfer any items

// Passing a variable as the quantity, but batched
batchOpen();
putShop(0, 0, qty, Item.get('spices'));
batchClose();
// KoLmafia transfers ALL of your spices to the store
// It also prints something like "4621819117588971520 is out of range, returning 0"

(I used a variable for demonstration purposes. In reality, any non-trivial expression will trigger the bug.)

In the last example, the error message is printed from line 828 of StringUtilities.java. I traced this back to line 4240 of RuntimeLibrary.java, which reads the contentLong field from the Value object. This issue might be solvable if we called intValue() here instead, but I haven't tested this idea.

Fortunately, this issue can be worked around by wrapping the quantity with toInt(), which seems to "sanitize" the quantity to an integer:
JavaScript: 
// Puts 1 spices in the store
putShop(0, 0, toInt(qty), Item.get('spices'));

// Puts 1 spices in the store
batchOpen();
putShop(0, 0, toInt(qty), Item.get('spices'));
batchClose();

There may be more typing issues like this, but this one could be particularly damaging.

Edit: It seems that the ASH function put_shop() does not reject floating numbers for the quantity, either:
Code: 
put_shop(0, 0, 1.5, $item[ spices ]);
// Prints "Adding spices to store..." and returns true, but does not transfer any items
 
Last edited:
Veracity

Veracity

Developer
Staff member
ASH autocoerces floats to ints in argument lists if the parameter is an int.

That is intentional.
 
philmasterplus

philmasterplus

Active member
Veracity said:
ASH autocoerces floats to ints in argument lists if the parameter is an int.

That is intentional.
Click to expand...

In that case, shouldn't the function call below cast the 1.5 to 1? It seems that KoLmafia is casting 1.5 to 0.

Code: 
put_shop(0, 0, 1.5, $item[ spices ]);
// Prints "Adding spices to store..." and returns true,
// but does not transfer any items
 
MCroft

MCroft

Developer
Staff member
philmasterplus said:
In that case, shouldn't the function call below cast the 1.5 to 1? It seems that KoLmafia is casting 1.5 to 0.

Code: 
put_shop(0, 0, 1.5, $item[ spices ]);
// Prints "Adding spices to store..." and returns true,
// but does not transfer any items
Click to expand...

Weirdly, it's casting "1.5" to 4609434218613702656
Code: 
qtyValue = {Value@7750} "1.5"
type = {Type@7759} "float"
contentLong = 4609434218613702656
contentString = null
content = null

Which is the bit representation of a double in a long. But not a rounded float. Either something may be funky in Value.java or contentLong doesn't mean what we expect it to.
Java: 
public Value( final double value )
{
       this.type = DataTypes.FLOAT_TYPE;
       this.contentLong = Double.doubleToRawLongBits( value );
}
 
Last edited:
Veracity

Veracity

Developer
Staff member
Sometimes it autoCoerces:

Code: 
> ash int foo ( float param ) { return param; } foo( 1.5 );

Returned: 1

> ash int foo ( int param ) { return param; } foo( 1.5 );

Returned: 1
Is it an issue with put_shop()?

Code: 
        params = new Type[] { DataTypes.INT_TYPE, DataTypes.INT_TYPE, DataTypes.INT_TYPE, DataTypes.ITEM_TYPE };
        functions.add( new LibraryFunction( "put_shop", DataTypes.BOOLEAN_TYPE, params ) );

    public static Value put_shop( ScriptRuntime controller, final Value priceValue, final Value limitValue, final Value qtyValue, final Value itemValue )
    {
        return put_shop( controller, priceValue, limitValue, qtyValue.contentLong, itemValue, false );
    }
It is declared to have an int and it assumes it has an int (actually a long).

Everywhere else in RuntimeLibrary, when it wants to get an int from a Value, instead of going straight to contentLong, it does something like this:

Code: 
        return new Value( value.intValue() );
(That is what to_int() does with anything other than a string.)
 
MCroft

MCroft

Developer
Staff member
I think I understand this as "Value.contentLong is NOT the Value as a long, but rather Float Value as represented as a Long. So it's still 1.5, but in the form of a stream of bits stored in a Long.

So if we need a Long, we need to use Value.intValue().
Here's what I did in RuntimeLibrary
Code: 
return put_shop( controller, priceValue, limitValue, qtyValue.contentLong, itemValue, false );
return put_shop( controller, priceValue, limitValue, qtyValue.intValue(), itemValue, false );

put_shop(0, 0, 1.5, $item[ whalebone corset ])

put one whalebone corset in the shop

What other type coercions should we test before committing this?

That works for floats, but breaks for strings.
Rich (BB code): 
> ash put_shop(0, 0, "1", $item[ whalebone corset ]) ;

Function 'put_shop( int, int, string, item )' undefined. This script may require a more recent version of KoLmafia and/or its supporting scripts. ()
 
Last edited:
MCroft

MCroft

Developer
Staff member
So, I reverted it and you get the same the message if you send put_shop(int, int, string, int) right now.

I tried other types-- "1" as a string, false, etc. and I'm reasonably happy with changing qtyValue.contentLong to qtyValue.intValue().

Is it worth putting string-to-number logic in there or should we just let (int, int, string, int) be a "Doctor it hurts when I go like this" error?

e.g.

Java: 
if ( itemValue.getType().equals( DataTypes.STRING_TYPE ) )
{
       itemVal = Integer.parseInt( itemValue.contentString  );
}

We also use .contentLong in mall_prices and session_logs. Those could also do nasty things if given floats instead of ints.
 
Veracity

Veracity

Developer
Staff member
You can make a version of the RuntimeLibrary function definition which allows a string and use param.toIntValue().intValue() which will do the string to int conversion. Or you can require the script to use to_int().

I vote for the latter.
 
philmasterplus

philmasterplus

Active member
I, too, am against "smart" string-to-int conversion. It can hide programming mistakes (e.g. two string and int variables with similar names). This is one "feature" I dislike about JavaScript.
 
MCroft

MCroft

Developer
Staff member
that was my thought as well. I don't see a advantage in using an arbitrary value when we can return an error.

It can come in a separate change request, with a reason that I haven't thought of yet.

R20632
 
philmasterplus

philmasterplus

Active member
Verified that put_shop() works correctly in r20632. Thank you!

Code: 
 > inv spices
spices (2)

 > ash put_shop(0, 0, 0.5, $item[spices])
Returned: true

 > ash put_shop(0, 0, 1.5, $item[spices])
Adding spices to store...
1 spices added to your store.
Returned: true

 > inv spices
spices

 > ash get_revision()
Returned: 20632
 
You must log in or register to reply here.
Top