464312 - NativeArrays.get(*,Scriptable), and getIds/getAllIds don't function properly

Status: RESOLVED INACTIVE

(Rhino Graveyard :: Core, defect)

Description

[David Parks]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: 1.7R1

Summary of issues:
1) get(String, Scriptable) method does not work with String values when the scope object (Scriptable) is a NativeArray.
2) get(int, Scriptable) requires different input depending on whether the scope object is a NativeArray or another object (index values for NativeArray, hash values for other Objects).
3) get(int, Scriptable) does not work at all (except for NativeArrays using index values) unless a bug in ScriptableObject is corrected.
4) If the bug in #3 is fixed then it is not clear in the documentation that you must provide the string hash key to get(int, Scriptable) rather than the same index position of the key when getIds() was called.
5) NativeArray overrides the method getIds() to include its elements, however it does not override getAllIds(), so a call to getAllIds() fails to include the array elements.

____________________________________________________________________________
Detailed description of issues
============================================================================

get(String, Scriptable)

Works for everything but Native Array objects, NativeArrays require that you use get(int, Scriptable)

Using get(String, Scriptable) on a NativeArray, example: get("1", scope), will not yield the same result as get(1, scope). However a call to getIds() on a NativeArray will reuturn an array of strings such as ["0", "1", "2"], so it should be allowed to call get(String, scope) rather than having to convert the string value into an int whenever the scope object is an instance of NativeArray (very inelegant).

_________________________________________________________________________

get(int, Scriptable)

Only work for Native Arrays if [int] is the index position of the array element. Does not work for anything else. 

If the following bug is corrected in ScriptableObject then it works when int is the hash value of the String returned by getIds().

//***** Bug correction added by David Parks *****
//Original line:
//                    if (sname != null) {
//Replaced with the following line:
                      if (name != null) {
//************ End bug correction ***************

___________________________________________________________________________

getAllIds() works well for objects other than NativeArray. NativeArray overrides the getIds() function, but does not override getAllIds(), so when getIds() is called the NativeArray returns all elements including its array items, however when getAllIds() is called on a NativeArray it defaults to NativeArrays super class ScriptableObject for the getAllIds() function which does not know about the NativeArrays elements.

Also, NativeArray does not override get(String, scope), it only overrides get(int, scope). 

Therefore, if you get a reference to (Scriptable)myNativeArray, you will get a different result than you would if you got a reference to (Scriptable)myNativeObject. And in order to be consistent you would have to check what type of Scriptable instance you have - this is clearly a violation of the Object Oriented Methodology.

Basically all derivative functions (e.g. NativeArray.get(String, scope)) should require the same input and yield similarly formatted output. In this case it does not. If handling NativeArrays this way was a design requirement then the get(*, *) methods should not have been overriden, rather the user of a NativeElement should have been forced to use custom methods of that class (such as NativeElement.getNativeElement(*,*))

____________________________________________________________________________


The Solution:
==========================
I have applied the following patches to my environment and have it working. 3 things were done:

   1) NativeArray.getAllIds() was implemented (of primary importantce to note is that the key value for the array objects in dense are returned in String format rather than Integer format).
   2) NativeArray.get(String, Scriptable) was implemented (minor int/string conversion changes were made to the original get(int, Scriptable) to facilitate this. 
   3) The bug in ScriptableObject was corrected [if (sname != null)] was changed to [if (name != null)].


Limitations to this fix:
===========================
	Basically this fix allows the get(String, Scriptable) method to work on all Objects derived from ScriptableObject in the same way (at least all of the ones I've tested so far). However it makes get(int, Scriptable) redundant, because get(int, Scriptable) requires the hash value of the String, so you might as well just use get(String, Scriptable) since you need to either pass myString or myString.hashCode().

	A better solution might be to fix get(int, Scriptable) so that it obtains an object using the same index number (0 to N) as is returned by getIds() (currently this is not the case for any objects, however the documentation vaguely suggests that this would be the correct useage).


Updated Code:
===========================
	The 3 blocks of code that were changed are listed below. Each one has the original code commented out for reference, everything else was then added.


============================================================================

[[[ScriptableObject.java]]]
//***** Bug correction added by David Parks *****
//Original line:
//                    if (sname != null) {
//Replaced with the following single line:
                      if (name != null) {
//************ End bug correction ***************


============================================================================

//******* Begin patch created by David Parks *****************

    public Object[] getIds(){
        return getIdsImpl(false);
    }

    public Object[] getAllIds(){        //Added an override for getAllIds()
        return getIdsImpl(true);
    }

    private Object[] getIdsImpl(boolean getAll)
    {
        Object[] superIds = (getAll) ? super.getAllIds() : super.getIds();
        if (dense == null) { return superIds; }
        int N = dense.length;
        long currentLength = length;
        if (N > currentLength) {
            N = (int)currentLength;
        }
        if (N == 0) { return superIds; }
        int superLength = superIds.length;
        Object[] ids = new Object[N + superLength];

        int presentCount = 0;
        for (int i = 0; i != N; ++i) {
            // Replace existing elements by their indexes
            if (dense[i] != NOT_FOUND) {
                ids[presentCount] = "" + i;     //Changed the key from an Integer to a String to be consistent with other sibling objects that drive from the same base class
                ++presentCount;
            }
        }
        if (presentCount != N) {
            // dense contains deleted elems, need to shrink the result
            Object[] tmp = new Object[presentCount + superLength];
            System.arraycopy(ids, 0, tmp, 0, presentCount);
            ids = tmp;
        }
        System.arraycopy(superIds, 0, ids, presentCount, superLength);
        return ids;
    }

//******** Original function, replaced as shown above **********
//    private Object[] getIdsImpl()
//    {
//        Object[] superIds = super.getIds();
//        if (dense == null) { return superIds; }
//        int N = dense.length;
//        long currentLength = length;
//        if (N > currentLength) {
//            N = (int)currentLength;
//        }
//        if (N == 0) { return superIds; }
//        int superLength = superIds.length;
//        Object[] ids = new Object[N + superLength];
//
//        int presentCount = 0;
//        for (int i = 0; i != N; ++i) {
//            // Replace existing elements by their indexes
//            if (dense[i] != NOT_FOUND) {
//                ids[presentCount] = new Integer(i);
//                ++presentCount;
//            }
//        }
//        if (presentCount != N) {
//            // dense contains deleted elems, need to shrink the result
//            Object[] tmp = new Object[presentCount + superLength];
//            System.arraycopy(ids, 0, tmp, 0, presentCount);
//            ids = tmp;
//        }
//        System.arraycopy(superIds, 0, ids, presentCount, superLength);
//        return ids;
//    }
//************** End patched code ********************************


============================================================================


//******* Begin patch created by David Parks *****************

    public Object get(int index, Scriptable start){
        return getImpl(""+index, start);
    }

    public Object get(String index, Scriptable start){
        return getImpl(index, start);
    }

    public Object getImpl(String index, Scriptable start)
    {
        //Convert the string index to an int, if it is not an int get value from super
        int ixValue;
        try{
            ixValue = Integer.parseInt(index);
        }catch(NumberFormatException e){
            return super.get(index, start);
        }

        if (!denseOnly && isGetterOrSetter(null, ixValue, false))
            return super.get(index, start);
        if (dense != null && 0 <= ixValue && ixValue < dense.length)
            return dense[ixValue];
        return super.get(index, start);
    }

//******** Original function, replaced as shown above **********
//    public Object get(String index, Scriptable start)
//    {
//        if (!denseOnly && isGetterOrSetter(null, index, false))
//            return super.get(index, start);
//        if (dense != null && 0 <= index && index < dense.length)
//            return dense[index];
//        return super.get(index, start);
//    }
//************** End patched code ********************************



Reproducible: Always

Steps to Reproduce:
Execute a simple script in debug mode (I have created a simple example of an embedded debugger for my tests).

Call Scriptable.getAllIds() after initializing your scope.

Execute some javascript such as instantiating an array and other variables.

at a line break try to write out all elements in the scope, that is, all elements that are returned by scope.getAllIds(). Recurse into any object that is instanceof Scriptable, writing out all elements that each object returns from getAllIds().




Actual Results:  
See summary of issues above. 

To explain it in one line: you will find that Scriptable.get(String, Scriptable) and Scriptable.get(int, Scriptable) have significantly different and arbitrary behaviors depending on what subclass the Scriptable object is.

Expected Results:  
See summary and solution section

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Closing. Bug management is now done here:
https://github.com/mozilla/rhino