Async functions with variable number of args won't work

[aminmarashi]

Consider the following code example:

function init(interpreter, scope) {
    interpreter.setProperty(scope, 'delayedLog', this.createAsyncFunction(function() {
        var args = [].slice.call(arguments);
        var callback = args.pop();
        setTimeout(function() { callback(console.log.apply(null, args)) }, 1000)
    }));
}

(new Interpreter('delayedLog("hi")', init)).run();

If executed, the above code throws this error:

RangeError: Invalid array length

The reason seems to be this commit which forces the position of callback to be always the same as the function definition.

I can see the point of the above commit, but would be nice if we could somehow find a workaround for this issue as well.

I wrote a workaround for this issue, which is enough for my existing use-case:

var MAX_ARGS = 100;

Interpreter.prototype.createAsyncFunctionVarArgs = function(func) {
    var interpreter = this;
    var asyncWrapper = function asyncWrapper() {
        var args = [].slice.call(arguments);
        var callback = args.pop();
        var reversedArgs = args.slice().reverse();
        // Remove all extra undefined from end of the args list
        var firstDefinedItem = reversedArgs.findIndex(function(i) { return i !== undefined });
        var trimmedArgs = firstDefinedItem < 0
            ? []
            : reversedArgs.slice(firstDefinedItem).reverse();
        var nativeArgs = trimmedArgs.map(function(arg) {
            return interpreter.pseudoToNative(arg);
        }).concat(callback);
        func.apply(null, nativeArgs);
    }
    // All functions will have 100 arguments + 1 callback
    Object.defineProperty(asyncWrapper, 'length', { value: MAX_ARGS + 1 });
    return interpreter.createAsyncFunction(asyncWrapper);
};

function init(interpreter, scope) {
    interpreter.setProperty(scope, 'delayedLog', interpreter.createAsyncFunctionVarArgs(function() {
        var args = [].slice.call(arguments);
        var callback = args.pop();
        setTimeout(function() { callback(console.log.apply(null, args)) }, 1000)
    }));
}

(new Interpreter('delayedLog("hi")', init)).run();

The biggest issue with the above workaround is that it makes every function args size = 101, that is not a very nice and efficient solution.

---

Instead of the above solution, I would like to propose adding support for async functions that return Promise to the interpreter code, another advantage to this is that the Promise abstraction can also be used to distinguish failure and successful results.

If we use Promise, we don't need to pass the callback and therefore we can leave the arguments unchanged.

I think because the Promise object is created outside of the interpreter code, this means the existing code will still work on older browsers.

I would love to create a PR with whatever solution you think is best.

[cpcallen]

It's unclear what the best way to proceed is. Async implementations that accept variable numbers of arguments should previously have been using Array.prototype.pop.call(arguments) to obtain callback; they are currently broken as @aminmarashi-binary reports, but:

• Reverting 4772bcc would fix them but break any code that obtains callback via a named argument.
• Having stepCallExpression append any trailing arguments to argsWithCallback would make both named-arguments and varargs implementations work, with the proviso that it would be impossible for the implementation to detect if any named argument was omitted, and creating the horrible situation of having the callback be infixed amongst the user-supplied arguments.
• Having stepCallExpression put the callback first breaks all existing code but corrects a serious design flaw. The breakage should be difficult to abuse, since although the callback could end up (incorrectly) being saved in some user-accessible location, no user-supplied argument will be callable, so async function implementations will definitely crash when they call callback if their named parameters haven't been updated, preventing any further user code from being executed. This might be preferable to further hackery, since I suspect that many existing usages are probably broken in more subtle ways at the present time.

I should note that although async function callbacks work much like Promises, the latter are for various reasons not quite the right fit in this situation; I created PR #178 to provide a similar but more appropriate mechanism to allow async functions to throw.

[pakya-1909]

Hey, can i work on this issue.

[cpcallen]

@pakya-1909: Thanks for the offer. I think this issue is waiting for @NeilFraser to make a decision about whether he is willing to break backwards compatibility to fix varargs or not.

[Webifi]

How about adding an optional falsely attribute to createAsyncFunction, making it createAsyncFunction (asyncFunc, varArgs), setting func.varArgs = varArgs.

Then in stepCallExpression:

 if (func.varArgs) {
  // call with callback first
 } else {
  // call as before
}

Adds a little overhead for an additional if, but should be backwards compatible with existing code. (Would only break existing code if they were erroneously passing more than one argument to createAsyncFunction.)

[cpcallen]

How about adding an optional falsely attribute to createAsyncFunction, making it createAsyncFunction (asyncFunc, varArgs), setting func.varArgs = varArgs.

An excellent suggestion.

I do wonder if it might be preferable to subsume any new async function support machinery in the work you're doing to support calling interpreted code from native functions, though: see google/CodeCity#172 for what I have in mind.

It might be useful to get @NeilFraser to weigh in on this, when he has a moment.

[Webifi]

though: see google/CodeCity#172 for what I have in mind.

Sorry, I'm a little confused on what I should be looking at in that link.

[cpcallen]

Sorry, I'm a little confused on what I should be looking at in that link.

Mainly, as an example, the addition of FunctionResult.Block to allow any NativeFunction (not just AsyncFunctions) to block while awaiting a (natively async) callback. There is a good example of how this is used in commit 8f7c760.

It basically makes AsyncFunctions obsolete. It means that we wouldn't need to make any changes to the existing createAsyncFunction API to add new features like varargs support, because all such API changes could be confined to a new and improved NativeFunction API.