My integer++ suddenly becomes a string

[gjanvier]

I initially posted my bug in Promise github but I don’t think it is specific to that lib. And I am not sure it is visible there.

In a specific use-case (see initial post in that github), an integer gets incremented and suddenly becomes a string, raising an ERROR: indexing array with string.

function Promise_serial1(promises) {
    // Copy/paste of Promise.serial
    local i = 0;
    return Promise.loop(
        @() i < promises.len(),
        function () {
            return "function" == type(promises[i]) // you'll see that this i integer will become a string??
                 ? promises[i++]()
                 : promises[i++];
        }
    )
}

If you don’t believe me, the same code with a small change (which, in my mind, is exactly equivalent), solved the issue:

function Promise_serial2(promises) {
    // Just a tiny change in Promise.serial
    local i = 0;
    return Promise.loop(
        @() i < promises.len(),
        function () {
            // I don't know why but I had to change the order here
            local res = promises[i++];
            if ("function" == type(res)) {
                res = res();
            }
            return res;
        }
    )
}

As it does not make sense to me, I would really like to ask the squirel / imp VM experts.

Any idea?

[hugo]

Paging @peter … he knows the VM the best!

[peter]

Oh, that is a good one. That took me all morning to figure out. (It was a worry, because wrong types out of nowhere can be caused by stack imbalances, and stack imbalances can lead to sandbox escapes.)

But there is no bug here, just an extremely odd Squirrel feature. The error isn’t being raised from the lookup of “i++” in “promises”, it’s being raised when “currentCommandPromise” is being looked-up inside the function commandWrapper. “But what lookup? in what array?”, I hear you ask.

So. The Squirrel feature in question here is “automatic context-object setting”. In Squirrel, all function calls have an implicit context-object, which is usually the global table for freestanding functions, and the class instance for class-method functions. The context-object is available inside the function itself under the name “this”, and is usually implicitly passed on to any further functions called, but it can be overridden (made explicit) using bindenv().

If “p” is an instance of class Promise, and you write a method call such as “p.fail(…)”, then the “fail” function really needs to have “p” as its context-object in order to access member variables and functions of that particular Promise. So when you extract a function from an object and then immediately call that function, Squirrel automatically uses the very object it was extracted from, as that function call’s context-object.

So far so good. The extremely odd bit, is that Squirrel also does this for extracting a function from an array and then immediately calling it – the array is set as the context-object:

local promises = [
    function() {
        server.log("type(this)="+type(this));
    },
];
promises[0]();

This is unfortunate, because arrays do a very poor job of being context-objects – not least because non-local variables are first looked up in the current context-object by name, before the root table is checked!

So the short answer is, your Promise_serial1 and Promise_serial2 are not as equivalent as they look, because:

promises[i++]();

triggers automatic context-object setting, whereas:

local res = promises[i++];
res();

does not. Promise_serial and Promise_serial1 both end up calling onStartCallback with the “promises” array as its context-object; onStartCallback fortuitously accesses no non-local variables before it calls on to commandWrapper with the same context-object. But commandWrapper does access the non-local variable currentCommandPromise. And that’s where your error came from.

Our own Promise.serial, in Promise library 4.0.0, turns out to be a bit of a bear-trap due to its use of the former construct. I’ll raise an internal bug.

[gjanvier]

I knew it was not something I could understand by myself

So if I just want to improve my code with 1 general best-practice, we should avoid any code like:

 promises[i++]();

• The issue is not in the i++

• But in the promise[any_variable_here](), because the context resolution is not what we would naively expect

• To avoid any issue we should prefer those 2 lines of code

  local tmp = promises[any_variable_here];
  tmp();

Right?

[peter]

Right?

Exactly right, yes. In fact it’s array[any-expression-at-all]() that’s the problem.

Peter

[gjanvier]

Thank you for your help