JsImplType.VerifyMethods does not handle base methods properly

[GoogleCodeExporter]

This is a bug in the source code.  See Type.cs lines 379 or so:

FillMethods(_JsType.definition);
FillMethods(_JsType.staticDefinition);
var baseType = BaseType;
if (baseType != null)
{
    var methods = baseType.GetMethods();
    foreach (var pe in methods)
    {
        if (_MethodsByName[pe._Name] == null)
        {
            _MethodsByName[pe._Name] = pe;
            _Methods.push(pe);
        }
    }
}

The problem is that _MethodsByName[key] is treated as a JsExtendedArray 
throughout the rest of the codebase.  However, in this one instance, you are 
storing a single-element into the dictionary, rather than an array.  This 
corrupts the _MethodsByName dictionary and results in the "GetMethods(name)" 
method failing to return the expected type (an array).  Instead, the code 
should be modified to be consistent with array usage like so:

            var newArray = new JsExtendedArray();
            newArray.Add(pe);
            _MethodsByName[pe._Name] = newArray;
            _Methods.push(pe);

Not sure if this breaks any real code, but in trying to improve the method 
GetAttributeTarget, it's failing to get the correct method due to this problem.

Original issue reported on code.google.com by kirk.w...@gmail.com on 29 Nov 2013 at 2:19

[GoogleCodeExporter]

I think that the original concept was to give out only the most derived methods 
(overrides), the 'name' should be unique (if I recall), since it's the extended 
method name. So, for any method by name you get only one method. Does it make 
sense? I might be wrong because I didn't check this and I'm speaking only from 
memory.

Original comment by DanelK...@gmail.com on 2 Dec 2013 at 6:15

[GoogleCodeExporter]

So regardless of whether _MethodsByName is intended to contain one element or 
an array, the fact is that the code is inconsistent with how it treats this 
field.  In Type.js:

https://code.google.com/p/sharpkit/source/browse/trunk/SharpKit.JsClr/System/Typ
e.cs

Line 251:
var methods = _MethodsByName[methodName].As<JsExtendedArray>();

Line 276:
var methods = _MethodsByName[name].As<JsExtendedArray>();

But on line 298:

_MethodsByName[pe._Name] = pe;

Where `pe` is defined as an element (from a foreach) of `baseType.GetMethods()` 
-- not an array.  Because of this, usages of this field that expect an array 
will blow up.  So even if this dictionary should only ever have one method per 
name, the other usages that try to treat it like an array will still be broken, 
so one side or the other needs to be fixed (either not cast to a 
JsExtendedArray, or always make sure it contains a JsExtendedArray).

Original comment by kirk.w...@gmail.com on 9 Dec 2013 at 9:40

[GoogleCodeExporter]

Thanks,

Transformed all code to use the generic typed version of JsObject - 
JsObject<JsArray<JsImplMethodInfo>>, and fixed the issue

Original comment by DanelK...@gmail.com on 12 Feb 2014 at 12:59

• Changed state: Fixed
• Added labels: v-5.3.6

[GoogleCodeExporter]

This issue was closed by revision r1492.

Original comment by DanelK...@gmail.com on 12 Feb 2014 at 12:59