Boa: Value::to_json destroy the order of items in an Array

[sele9]

Describe the bug
The order of array items is sometimes mixed after the conversion from boa::Value to serde_json::Value.

To Reproduce
I've added a few lines of code here:

https://github.com/boa-dev/boa/blob/edfafc4e0324da1548f672e6c1edc51a6e1337b5/boa/src/value/mod.rs#L242

let mut keys: Vec<u32> = Vec::new();
    for k in obj.borrow().keys() {
        match k {
            PropertyKey::Index(i) => {
                keys.push(i);
            },
            _ => {},
       }
}

The debugger shows me the following keys:

This JavaScript code reproduces the issue:
where $t is a globally registered function implemented in rust and self the equivalent of globalThis

function tableItems(items) {
    return items.map((item, i) => {
        if (i === 0) {
            item.isFirst = true;
        }
        if (i === items.length - 1) {
            item.isLast = true;
        }
        return item;
    });
}

self.context = {
    bevollmaechtigterItems() {
        const geschlechtF = (self.vollmacht_geschlecht === 'm' || self.vollmacht_geschlecht === 'w' || self.vollmacht_geschlecht === 'd') ? self.$t('geschlecht.' + self.vollmacht_geschlecht) : '';
        return tableItems([
            { label: self.$t('pdf.labels.vorname'), value: self.vollmacht_vorname },
            { label: self.$t('pdf.labels.nachname'), value: self.vollmacht_nachname },
            { label: self.$t('pdf.labels.geschlecht'), value: geschlechtF },
            { label: self.$t('pdf.labels.strasse'), value: self.vollmacht_strasse },
            { label: self.$t('pdf.labels.hausnummer'), value: self.vollmacht_hausnummer },
            { label: self.$t('pdf.labels.postleitzahl'), value: self.vollmacht_postleitzahl },
            { label: self.$t('pdf.labels.ort'), value: self.vollmacht_ort },
            { label: self.$t('pdf.labels.email'), value: self.vollmacht_email },
            { label: self.$t('pdf.labels.telefonnummer'), value: self.vollmacht_telefonnummer },
        ]);
    }
};

Expected behavior
It should return in the same Order as defined in javascript.

Build environment (please complete the following information):

• OS: Ubuntu Linux
• Version: 20.04.2 LTS
• Target triple: x86_64-unknown-linux-gnu
• Rustc version: 1.48.0-nightly

Additional context
I replaced these lines:
https://github.com/boa-dev/boa/blob/edfafc4e0324da1548f672e6c1edc51a6e1337b5/boa/src/value/mod.rs#L240-L253
with:

                if obj.borrow().is_array() {
                    let mut arr: Vec<JSONValue> = Vec::new();
                    let mut keys: Vec<u32> = Vec::new();
                    for k in obj.borrow().keys() {
                        match k {
                            PropertyKey::Index(i) => {
                                keys.push(i);
                            },
                            _ => {},
                        }
                    }
                    keys.sort();
                    for key in keys {
                        let k = PropertyKey::Index(key);
                        let value = self.get_field(k.to_string());
                        if value.is_undefined() || value.is_function() || value.is_symbol() {
                            arr.push(JSONValue::Null);
                        } else {
                            arr.push(self.get_field(k.to_string()).to_json(interpreter)?);
                        }
                    }
                    Ok(JSONValue::Array(arr))
                }

it works as expected but maybe there is a more performant way?

[HalidOdat]

it works as expected but maybe there is a more performant way?

Iterating through the indexed keys is better so we should have:

                if obj.borrow().is_array() {
                    let keys: Vec<u32> = obj.borrow().index_property_keys().collect().sort();
                    let mut array: Vec<JSONValue> = Vec::with_capacity(keys.len());
                    for key in keys {
                        let value = self.get_field(key);
                        if value.is_undefined() || value.is_function() || value.is_symbol() {
                            array.push(JSONValue::Null);
                        } else {
                            array.push(value).to_json(interpreter)?);
                        }
                    }
                    Ok(JSONValue::Array(array))
                }

we also dont do .get_field() twice, we only iterate though the properties that have a valid array index (this is faster since we store it in a separate space), we also don't call .to_string().

What do you think? @sele9

[sele9]

I think the "sort" is not really needed. If it is an Array the order should be from 0 .. length currently i'am using:

let len = obj
    .borrow()
    .keys()
    .into_iter()
    .filter(|k| match k {
        PropertyKey::Index(_) => true,
        _ => false,
    })
    .count();
let mut arr: Vec<JSONValue> = Vec::with_capacity(len);
for key in 0..len {
    let k = PropertyKey::Index(key as u32);
    let value = self.get_field(k);
    if value.is_undefined() || value.is_function() || value.is_symbol()
    {
        arr.push(JSONValue::Null);
    } else {
        arr.push(value.to_json(interpreter)?);
    }
}
Ok(JSONValue::Array(arr))

[HalidOdat]

I think the "sort" is not really needed. If it is an Array the order should be from 0 .. length currently i'am using:

This is how its implemented in the spec, but there is a problem with this approach, with sparse arrays for example:

let x = [];
x[4294967294] = 1;
JSON.stringify(x)

with this implementation it will iterate through from 0 to u32::MAX (which takes around ~1 minute) while the first implementation is less space efficient (Vec<u32> and sort) will take a couple milliseconds.

[sele9]

okay i'll update the PR

[sele9]

The compiler complains about the type, because sort does not return a value.

obj.borrow().index_property_keys().collect().sort();

241 | let keys : Vec = obj.borrow().index_property_keys().collect().sort();
| ^^^^^^^ cannot infer type for type parameter B declared on the associated function collect

[sele9]

let mut keys : Vec<u32> = obj.borrow().index_property_keys().cloned().collect();
keys.sort();

would work