JS bug in trunk with this in filteredEntities

[Eric Walch]

I recently had problems with the use of a this.xxx in a named function. I was sure it never throw errors with 1.73. I changed the code to let it work with trunk and forgot about it. But this weak, McLane's personalities oxp gave the similar error with trunk (not with 1.73 were McLane tested it). Today Thargoid came up with the same problem in a pm.

I am not capable of determining if it is an oolite bug or it is because of an intentional code change. Anyhow, I scripted a small example for a shipscript. this.test is called from the AI with a script message.

Code: Select all

this.test = function()
{
    this.preyLimit = 10;
    function isOffender(entity)
    {
        return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked
    }
    var nextTarget = system.filteredEntities(this, isOffender, this.ship, 25600)
    log("testship", "Testship found target: "+ nextTarget);
}

It is a stripped piece of code from McLane. When used under 1.73 I get a correct log with the closes target that has the bounty. But with current trunk I get:

Code: Select all

Warning (strict mode): reference to undefined property this.preyLimit
    Active script: "testShip" 1.01
    testShip.js, line 45:
            return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked

[Screet]

I am not capable of determining if it is an oolite bug or it is because of an intentional code change. Anyhow, I scripted a small example for a shipscript. this.test is called from the AI with a script message.

Maybe have a look at this to see how it can be fixed - I had a similar error and fixed it a few weeks ago:

Code: Select all

[script.javaScript.exception.noProperties]: ***** JavaScript exception ("drones_Pickup" 1.0): TypeError: this.ship has no properties
[script.javaScript.exception.noProperties]:       ../AddOns/A - OSE Main Data WiP V0.70.21.oxp/Scripts/drones_Pickup.js, line 29.

fixed by:

Code: Select all

this.findMastersHostiles = function()
{
	var master = this.ship.master;
    function isHostileToMaster(entity) 
    { 
       return (entity.isShip && entity.target && entity.target == master && entity.hasHostileTarget);
     }
    let targets = system.filteredEntities(this, isHostileToMaster, this.ship, 25600)
    if(targets.length > 0)
    {
        this.ship.target = targets[0];
        this.ship.reactToAIMessage("NEW_DRONE_TARGET");
        if(this.ship.master == player || this.ship.master == player.ship) player.commsMessage(this.ship.shipDescription+": Attacking "+targets[0].shipDisplayName)
    }
    else this.ship.reactToAIMessage("NOTHING_FOUND");
    
}

Screet

[Eric Walch]

Screet,

I know about that fix, I even suggested it to McLane. But that fix does not work with 1.73

I changed the code into:

Code: Select all

this.test = function()
{
    var preyLimit = 10;
    function isOffender(entity)
    {
        return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked
    }
    var nextTarget = system.filteredEntities(this, isOffender, this.ship, 25600)
    log("testship", "Testship found target: "+ nextTarget);
}

This works with trunk but with 1.73 i now get:

Code: Select all

Exception: ReferenceError: preyLimit is not defined
    Active script: "aquatics_platform" 1.0
    testShip.js, line 45:
            return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked

So I get confused to what is wrong JS code or what is a bug in Oolite. It even logs the wrong active script.

I like to fix this in a way it works in both Oolite versions.

[Thargoid]

The one above is exactly what I get (in essence, rather than specific code of course).

I'm currently setting up some functions globally (at the top of a script outside any function) and that works under 1.73.4 perfectly. If I move them inside the function that calls them I get the undefined error along the lines of the one above.

These functions use this.ship in them however (to judge distance between the ship in question and other ships including the player), and under 1.74 I get the error that this.ship isn't defined.

My presumption was that this was due to the proposed re-working of how ship-scripts were handled under 1.74 (changed functionality of this.startUp, depreciation of this.reset and reloading of scripts at game restart etc). But as Eric mentions, it is a little confusing and not one I can see a compatibility work-around for easily to have the script compatible with both versions.

[Kaks]

Screet,

I know about that fix, I even suggested it to McLane. But that fix does not work with 1.73

I changed the code into:

Code: Select all

this.test = function()
{
    var preyLimit = 10;
    function isOffender(entity)
    {
        return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked
    }
    var nextTarget = system.filteredEntities(this, isOffender, this.ship, 25600)
    log("testship", "Testship found target: "+ nextTarget);
}

This works with trunk but with 1.73 i now get:

Code: Select all

Exception: ReferenceError: preyLimit is not defined
    Active script: "aquatics_platform" 1.0
    testShip.js, line 45:
            return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked

So I get confused to what is wrong JS code or what is a bug in Oolite. It even logs the wrong active script.

I like to fix this in a way it works in both Oolite versions.

I think I know what's happening! It's an out of scope thing. You have a named function inside another function. For a very long time it was not possible to do just that, but apparently it's possible now. There's a mini-essay to be written on the subject, but I'll stop now.

Basically this inside isOffender refers to the isOffender function itself, which has no connection whatsoever with the rest of the function 'above' it. Instead of using a named function declaration, you're better off using an anonymous function declaration, which still allows for 'external' variables to be used inside its declaration.

If I'm right, either of the two following function declations should work:

the first one, not that much of an improvement:

Code: Select all

    function isOffender(entity)
    {
        this.preyLimit = 10;
        return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked;
    }

it's hardly worth the bother to create the preyLimit variable to begin with.

The second one, useful if you want to 'import' preyLimit from outside the function:

Code: Select all

    var preyLimit = 10;
    var isOffender = function(entity)
    {
        return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked
    }

the following declaration wouldn't work

Code: Select all

    this.preyLimit = 10;
    var isOffender = function(entity)
    {
        return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked
    }

because the this keyword outside the function should refer to 2 different things. Still don't quite know why the original code worked in 1.73.x I thought it wouldn't have worked at all to begin with...

[Eric Walch]

The second one, useful if you want to 'import' preyLimit from outside the function:

Code: Select all

    var preyLimit = 10;
    var isOffender = function(entity)
    {
        return entity.isShip && entity.bounty >= preyLimit && !entity.isCloaked
    }

the following declaration wouldn't work

Code: Select all

    this.preyLimit = 10;
    var isOffender = function(entity)
    {
        return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked
    }

because the this keyword outside the function should refer to 2 different things. Still don't quite know why the original code worked in 1.73.x I thought it wouldn't have worked at all to begin with...

My hope was completely on you it needs knowledge of the JS internals which I don't have.

I now repeated my tests with both variations in the anonymous function declaration as above. Same result. "var preyLimit = 10;" only works with trunk and "this.preyLimit = 10;" only working with 1.73 (Have not tested with 1.72)
this.preyLimit = 10 was just an example to make it more readable. In the original code it is "this.ship.scriptInfo.preyLimit;". So clearly a variable that is defined outside of the function.

And we are using the same spidermonky version as in 1.73, or not? The functions are called over the system.filteredEntities() so maybe there is a change?

[Thargoid]

To perhaps make it a little simpler I've just PM'd Kaks a link to the full WIP of my OXP, so he can see my "problem code" in action. That might make things a little clearer than reproducing snippets here?

[another_commander]

And we are using the same spidermonky version as in 1.73, or not? The functions are called over the system.filteredEntities() so maybe there is a change?

Yes, we are using the same Spidermonkey. I can confirm the inconsistency also on Windows, but I do not have in-depth knowledge of Spidermonkey to be able to tell which of the two behaviours is the correct one. All I can say is that comparing the OOJavaScriptEngine.m files from 1.73.4 and current trunk shows that there have been changes where (JSFunction *) pointers were substituted with jsvals and JS_CallFunction calls have been substituted with JS_CallFunctionValue ones. Still, I'd rather leave the analysis to someone more experienced with JS.

[Eric Walch]

Looked it up in the revision history. Comment for 2404 is:

Workaround for a nasty little SpiderMonkey bug/misfeature (supposedly fixed in 1.: converting a JSObject or jsval to a JSFunction discards its context information, so effectively callbacks implemented using JS_CallFunction() were not closures. The workaround is to use jsvalues and JS_CallFunctionValue() instead.

And when looking at the changes, the fix was in the filteredEntities() function. So basically, this.xx working at all was a bug, and the fix made it possible to use normal outside variables.

That clears the problem, but make writing code for both 1.73 and 1.74 difficult.

Looking at the working of the filter method at an old Mozzilla page I read:

Description
filter calls a provided callback function once for each element in an array, and constructs a new array of all the values for which callback returns a true value. callback is invoked only for indexes of the array which have assigned values; it is not invoked for indexes which have been deleted or which have never been assigned values. Array elements which do not pass the callback test are simply skipped, and are not included in the new array.

callback is invoked with three arguments: the value of the element, the index of the element, and the Array object being traversed.

If a thisObject parameter is provided to filter, it will be used as the this for each invocation of the callback. If it is not provided, or is null, the global object associated with callback is used instead.

filter does not mutate the array on which it is called.

The range of elements processed by filter is set before the first invocation of callback. Elements which are appended to the array after the call to filter begins will not be visited by callback. If existing elements of the array are changed, or deleted, their value as passed to callback will be the value at the time filter visits them; elements that are deleted are not visited.

So the way of calling determines to what this refers.

Basically this all is as clear for me as a Japanese text would be.

[Eric Walch]

Playing a bit with the code showed that:

Code: Select all

this.test = function()
{
    this.preyLimit = 10;
    var isShip = function (entity)
    {
        return entity.isShip;
    }
    var isOffender = function (entity)
    {
        return entity.bounty >= this.preyLimit && !entity.isCloaked;
    }
    var nextTarget = system.filteredEntities(this, isShip, this.ship, 25600).filter(isOffender, this);
    log("testship", "Testship found target: "+ nextTarget);
}

works with both 1.73 and 1.74. Basically I use now oolites filter function to find all ships in range and than use the JS filter method to select the offenders from it. Here I can pass the this. So probably there is now a bug in the this definition of the filteredEntities.

[Kaks]

In normal js the this keyword should only refer to the function we're in. So it should change meaning depending on the context. I have a possibly more future proof option.

Please try the following:

Code: Select all

    var PreyLimit=this.ship.scriptInfo.preyLimit;
    var isOffender = function(entity)
    {
        this.preyLimit = PreyLimit;
        return entity.isShip && entity.bounty >= this.preyLimit && !entity.isCloaked
    }
    var nextTarget = system.filteredEntities(this, isOffender, this.ship, 25600) 

It doesn't seem to cause any errors in either 1.73.x or trunk, and follows the language specifications too!

The important thing would be not to mix the various this(s), so you'd still need the var PreyLimit to act as a bridge between the outside & the inside of a function.

[JensAyton]

Basically this inside isOffender refers to the isOffender function itself, which has no connection whatsoever with the rest of the function 'above' it.

This should not be the case, since filteredEntities sets “this” explicitly, similarly to native JS Function.call() or Array.filter() (that’s what the first parameter to filteredEntities is for). I suspect that this broke when I fixed inheritance of variables (r2404). Presumably also applies to filteredSystems(). Bug #16439

[Kaks]

Looks like I forgot to mention this should be fixed in trunk now:

It should be fixed in trunk by now!