kswoll / WootzJs

C# to Javascript Compiler implemented via Roslyn
MIT License
110 stars 21 forks source link

Bug in multicast delegate handling #17

Closed Danielku15 closed 9 years ago

Danielku15 commented 9 years ago

The Delegate.Combine method compiles to this:

if (a == null)
    return b;
else if (b == null)
    return a;
else if (System.MulticastDelegate.$GetType().IsInstanceOfType(a))
    return System.MulticastDelegate.prototype.Add.call(($cast(System.MulticastDelegate(), a)), b);
else
    return (function() {
        var $invocationList = $arrayinit([a, b], System.Delegate);
        var $delegate$ = function() {
            for (var $i = 0; $i < $invocationList.length; $i++)
                $invocationList[$i](arguments);
        };
        $delegate$.prototype = new System.MulticastDelegate();
        System.Object.$TypeInitializer($delegate$, $delegate$);
        System.Delegate.$TypeInitializer($delegate$, $delegate$);
        System.MulticastDelegate.$TypeInitializer($delegate$, $delegate$);
        $arrayinit([a, b], System.Delegate)[0].$type.$TypeInitializer($delegate$, $delegate$);
        System.MulticastDelegate.prototype.$ctor$1.call($delegate$, a.get_Target(), $invocationList);
        $delegate$.$type = $arrayinit([a, b], System.Delegate)[0].$type;
        return $delegate$;
    }).call(this);

The problematic part is $invocationList[$i](arguments);. This will call the delegate function:

var delegateFunc = null;
    delegateFunc = function() {
        return lambda.apply(delegateFunc.get_Target(), arguments);
    };

This will result in a wrong argument passing to the actual lambda. The lambda will be called with an arguments object which contains the real arguments list as first argument. So basically an argument array where the first element is the actual argument array. The solution is to change the $invocationList[$i](arguments); to$invocationList[$i].call(null, arguments);`.

I'm not quite sure why the return new MulticastDelegate(a.Target, new[] { a, b }); code gets expanded to this heavy this-scoped object creation but it breaks the correct delegate invocation.

kswoll commented 9 years ago

The reason it's so verbose is related to the reason that the CreateDelegate function ($delegate) is so verbose. It's because we're trying to ensure that an adhoc Javascript function has the type information required to participate in type operations such as casts, etc. In particular, MulticastDelegate has to be built in such a way that it is possible to cast it to the original delegate type. Perhaps there could be a better design.

Also, the bug should be fixed now. Added a test to the bottom of DelegateTests that repro'd the issue.