Setting array length does not delete elements.

[benjaminflin]

Describe the bug
According to the spec, shortening array.length should delete elements in the array. Currently, array elements that are accessed beyond the array length are not necessarily undefined.

To Reproduce

let arr = [1, 2, 3, 4, 5];
arr.length = 3;
arr[4]; // 5

Expected behavior
arr[4] should be undefined but instead is 5.

Build environment

• OS: macOS Mojave
• Version: 10.14.3
• Target triple: x86_64-apple-darwin
• Rustc version: rustc 1.46.0-nightly (f455e46ea 2020-06-20)

Found in #555

[54k1]

Hey I'm new to this codebase and decided to take a look at this. Please correct me if I'm wrong.
I have seen how arrays are implemented. Basically it is a dictionary with array indices as properties (as inferred from this builtin).

Object properties are separate from ObjectData and the function responsible for setting properties directly mutates the dictionary (properties field of struct Object) regardless of ObjectData. I think here's where the problem lies. We might need more information while setting properties or perhaps delegate this action to the data (ObjectData) field of struct Object.

This would require perhaps a new trait which constraits objects (array, etc.) to have get/set properties (among others). The default impl of the set property could be the one that exists here. For special cases, this can be overridden.

Could you highlight the problems with this? Thanks.

[HalidOdat]

Hi @54k1

I think here's where the problem lies. We might need more information while setting properties or perhaps delegate this action to the data (ObjectData) field of struct Object.

This is a problem with Internal Object Methods , they currently are only set for ordinary objects, but not for array, string etc. Every object in the spec gets these internal methods an example is [[DefineOwnProperty]] for ordinary objects it calls OrdinaryDefineOwnProperty but for Array [[DefineOwnProperty]] is different when the property that is being defined is "length" it calles ArraySetLength this is what shrinks the array.

This would require perhaps a new trait which constraits objects (array, etc.) to have get/set properties (among others). The default impl of the set property could be the one that exists here. For special cases, this can be overridden.

We are trying to solve an inheritance problem, rust does not have a way to describe a base class where we have base class (with the the properties) and all the object (Array, etc) inherit from it. Also this would force dynamic dispatch this would slow down the code and it would not allow some compiler optimizations, we know the exact number of object types we don`t have do do this.

I was going to do this after #577 which is another architectural problem that needs to be fixed, before the internal object methods.

A hacky way of fixing this would be to check in Value::set_field and see if its a Array and if it is an the field is "length" then we shrink the array as described in ArraySetLength

[benjaminflin]

@HalidOdat I was wondering how you were going to go about overriding internal object methods for exotic objects in general, since at the moment doing something like exoticObj.specialProperty = value; will always call the ordinary object internal methods. I'm guessing that it would not be preferable to do a check for the object type in OrdinaryDefineOwnProperty but also it wouldn't make sense to do trait/dynamic dispatch for the reasons you mentioned. Would it make sense to do that check in the Executable implementation for GetField?

There might already be a solution offered in the codebase somewhere that I missed, so please let me know.

Perhaps a new issue should be made for the handling of exotic objects.

[HalidOdat]

@HalidOdat I was wondering how you were going to go about overriding internal object methods for exotic objects in general, since at the moment doing something like exoticObj.specialProperty = value; will always call the ordinary object internal methods. I'm guessing that it would not be preferable to do a check for the object type in OrdinaryDefineOwnProperty but also it wouldn't make sense to do trait/dynamic dispatch for the reasons you mentioned. Would it make sense to do that check in the Executable implementation for GetField?

There might already be a solution offered in the codebase somewhere that I missed, so please let me know.

This is how I intended to do it, we have the internal method for example [[set]], we also have the type definitions of what the object is Array, Map, etc. We have all we need to make this a static dispatch. So something like this:

impl Object {
	fn ordinary_set(&self /* , ... other parameters */) -> bool {
		// implementation
	}
	fn set(&mut self /* , ... other parameters */ ) -> bool {
		match self.data {
			ObjectData::Array => {
				// Arrays [[set]]
			}
			ObjectData::String(string) => {
				// Strings [[set]]
			}
			// This is for ordinay objects and for objects
			// that don't define a special implementation of [[set]]
			_ => self.ordinary_set(/* , ... args*/)
		}
	}
} 

This way of implementing allows the compiler to do some optimization that can't be done in dynamic dispatch.

If you have a better solution, or a way to improve it feel free to say so :)

Perhaps a new issue should be made for the handling of exotic objects.

I was going to create it after #577

[HalidOdat]

Perhaps a new issue should be made for the handling of exotic objects.

@benjaminflin. I created the issue we can discuss it there #591.

[tofpie]

This issue has been fixed in #1042.