Decouple Object from Value

[HalidOdat]

What is the problem with the current implementation?
The problem is that we treat Value as an Object which is not always the case. for example Value has a .set_field() which sets an object field, this does not make sense for a couple of reasons:

• what if Value is not an object should calling .set_field panic?, should it ignore it? should it return an option or result? or something else? (we ignore non object values which can become a really hard bug to debug), as we can see it does not make sense for Value to have Object behaviour
• This also forces us to do checks if the value is an object.

If we need object behaviour we can call as_object method.

Changes:

• Move all object behaivour from Value to Object
• Places were we need (or places were the type is only an object for example: the global object) should be GcObjects
• function like .to_object should return a GcObject not Value done

Doing these thing should remove unnecessary checks and make the API more intuitive

Any suggestions on how to improve this?

[RageKnify]

Hey @HalidOdat I found this interesting and am trying to tackle it.
To ensure I'm following your "vision", methods from builtins like Array would stop receiving &Value as this and instead receive &Object/&mut Object, right?

Ps: Also, functions like Array::new_array would change to return Result<GcObject> or Result<Object> rather than Result<Value>, or no?

[jasonwilliams]

@HalidOdat how are you handling boxing, unboxing of primitive values? I think I left set_field and get_field on value so things like “hello world ”.trim() would still work

[HalidOdat]

@HalidOdat how are you handling boxing, unboxing of primitive values? I think I left set_field and get_field on value so things like “hello world ”.trim() would still work

We store some primitives on the stack (number, boolean, undefined, null) and string, bigint, symbol on the heap with Rcs (since they can't have cycles, less work for the gc) and object with Gc. we discussed this in #465 .

Why would removing .set_field and .get_field cause “hello world ”.trim() to break?

[jasonwilliams]

Ok sounds good.

Why would removing .set_field and .get_field cause “hello world ”.trim() to break?

I think originally the unboxing happened inside of get_field() where the value was checked and upgraded to an object of it was a primitive. .trim() would have called get_field(“trim”)

I think we refactored quite a few times since this though so you can ignore this.

[HalidOdat]

Hey @HalidOdat I found this interesting and am trying to tackle it.
To ensure I'm following your "vision", methods from builtins like Array would stop receiving &Value as this and instead receive &Object/&mut Object, right?

Changing this from &Value to anything else wont work, since they would not match the NativeFunction signature, and we should not change the NativeFunction to take object since this can be some value that is not an object.

We need to the check as described in the spec for when this is not a object for example: Array.prototype.push

1. Let O be ? ToObject(this value).
2. Let len be ? LengthOfArrayLike(O).

It calls .to_object on this which converts it to a object (if it is not already), so we handle the case for when this is not an object but .set_field and .get_field does not and just ignore it if it isn't an object, this is not spec compliant.
There is also another problem that to_object returns a Result<Value> instead of Result<GcObject>, there are many places where this happens and that's why is why it's hard to remove them directly (generates ~1000 errors). Many PRs I did were to make this transition easier. So the way I would suggest on doing this, would be in small incremental changes, doing it all at once is very hard. For example making a PR so .to_object returns a Result<GcObject> and another PR fixing a something else and so on.

Ps: Also, functions like Array::new_array would change to return Result<GcObject> or Result<Object> rather than Result<Value>, or no?

If it is a helper function then, yes it should.

[RageKnify]

Yeah, I started at .to_object, I'll try to make it in small stages like you said.

One thing I was wondering, would the global_obj in Realm become an Object/GcObject or would it remain a Value?
And if it does change, which would be more aproppriate?

[HalidOdat]

One thing I was wondering, would the global_obj in Realm become an Object/GcObject or would it remain a Value?
And if it does change, which would be more aproppriate?

It should be GcObject since it can be referenced, GcObject is like Rc<...> (but it handles cyclic references) and the global object can be referenced form anywhere so we need to store it as such.

If it is referenced it should be GcObject, but if it is owned it's better to store it as Object

[RageKnify]

I have created my PR, #712, I'll leave a note here as to how this should also eventually be done since I did it for new_object_with_prototype:

• Value::new_object -> Object::new_object

[RageKnify]

Hey @HalidOdat any idea on what I should try to change next? I was trying to change Value::new_object to return GcObject, which should probably also move to Object, but it would require a lot of changes in other places. So I wonder if there's anything smaller you have in mind.

[HalidOdat]

Hey @HalidOdat any idea on what I should try to change next? I was trying to change Value::new_object to return GcObject, which should probably also move to Object, but it would require a lot of changes in other places. So I wonder if there's anything smaller you have in mind.

I think it would be a good idea to wait until #722 lands since it also tackles this issue in a way and will generate a lot of merge conflicts.

[jasonwilliams]

@HalidOdat from my work on #854 it would be good to stack-allocate Values, but as some of them hold RCObjects its not possible. Should there be some pointer which through a method gives back the object?

[HalidOdat]

@HalidOdat from my work on #854 it would be good to stack-allocate Values, but as some of them hold RCObjects its not possible.

There is a problem with objects being stack allocated, many operations (eq, some object methods, object internal methods) require to know the exact location in memory which means we can move the objects once there created (say for example pop two and push one in the case of #854), if we do something will break.

Should there be some pointer which through a method gives back the object?

Sorry, could you explain a bit more?

[RageKnify]

Hey @HalidOdat I got started on changing the global object to a GcObject rather than Value. One thing I ran into is that there are some methods of GcObject that receive &mut self, I think it stayed like that from when they were methods of Object, do you think we should keep it to signal mutability or change it to just &self?

[sphinxc0re]

In Firefox, setting a field on a Number is just ignored.

[RageKnify]

In Firefox, setting a field on a Number is just ignored.

boa seems to match that behaviour for me, was there a situation where it seemed to not match @sphinxc0re ?

[sphinxc0re]

No, I was just commenting on the "should set_field panic" part. It shouldn't

[Razican]

I don't think this will be ready for 0.14, so I'm moving it out.

[HalidOdat]

Many refactors/changes have been done since this issue was created and they have been resolved. So closing this :)