Implemented Revamped Object Model

[mitsuhiko]

This changes the object interface to support both structs and sequences with the possibility to add extra behavior (eg: arbitrary key maps) at a future point. There are some challenges with this design however.

Issues:

These are probably something I need to accept:

• impl<'a, T: ArgType<'a, Output = T>> ArgType<'a> for Vec<T> cannot be implemented to support these new dynamic sequences because the lifetime constraints cannot be enabled. Restrict?

Things to resolve:

This is stuff that can be fixed:

• Similarly the slice syntax x[:5] first converts the entire value before slicing down.
• Decide on the names.
• The Value::as_slice API cannot return borrowed values for dynamic slices. Internally a as_cow_slice has been added. Potential solution is to retire as_slice entirely for SeqObject.
• Iteration over dynamic slices first makes a real slice out of it. This is mostly done because of lifetime issues that are annoying to work around.
• Consider adding SeqObject::iter

Refs #147

[mitsuhiko]

Other potential names I made up:

• Object::behavior -> Object::as_container
• ObjectBehavior -> AsContainer
• AsSeq -> AsSeqContainer
• AsStruct -> AsStructContainer

[SergioBenitez]

Why not just ObjectKind? Object::kind() -> ObjectKind, StructObject, SeqObject, as_seq_object, as_struct_object.

Will look into making use of this later on today and report back.

[SergioBenitez]

A few notes from making use of this:

• The naming of the get() methods introduced conflicts with {BTree/Hash}Map::get(). Perhaps get_field()?
• The name of len() in AsSeq and AsStruct is also cause for concern as it may conflict with slice/vec methods. Maybe num_fields()?
• Implementing both AsStruct or AsSeq and Object feels really redundant. One thing that could help resolve this are Value::from_struct_object and Value::from_seq_object methods so I don't need to implement Object when it provides no value. Or perhaps a macro that generates the impl.
• AsStruct::len() and AsStruct::is_empty() feel like they shouldn't be there. I'm mostly curious why they're there when they weren't there before and if I should try to optimize them. Edit: I see why they're there now, but I'm wondering where they're used and if it's worthwhile to try to optimize them.

[SergioBenitez]

Also, I haven't investigated further, but I believe there's a bug somewhere. I'm getting an undefined value error where I don't expect one. Specifically, I have the equivalent of the following:

19 >       foo.bar[0].baz
i                 ^^^^^^^ undefined value

From the call to bar[0], I return:

Object {
    "baz": String("..."),
},

And yet I still get the error.

Edit: Here's the bug: #148 (review).

[SergioBenitez]

General feedback.

[SergioBenitez]

Also, it would be simple to add a (implemented by default) iter() -> Iter<'_> method to AsSeq to iterate over any AsSeq. The struct would of course hold a reference to &self and internally call get().

[SergioBenitez]

Performance wise, I'm seeing about a ~5% speedup as a result of making use of this. Not as wild as using Object, but still a nice and easy win.

[mitsuhiko]

I agree that having to implement Object and AsSeq is a bit odd, but it's not entirely terrible. The slice situation is more annoying. Potentially the solution is to really say the engine at least internally starts using the AsSeq trait in most places so at least the code duplication goes away, but I'm not sure if it's really so appealing as a user to have to use a custom sequence trait but it's unclear.

In any case as_slice as it exists today cannot be satisfied with dynamic slices and either has to live with that restriction or go away entirely.

[mitsuhiko]

Still not particularly happy with this but it's going somewhere. as_slice is internal now but it has to hang around for borrowing of filter arguments. Unfortunately I cannot find a way to make these work with dynamic sequences since they do not have values that can be borrowed. I'm really not sure what could be done about that.

The APIs of SeqObject and StructObject are also quite ugly now with the new names (get_item, seq_len and get_field and struct_size) but I do agree that having the names clash with common other names is highly impractical.

I now also have a weird impl impl<'a> SeqObject for &'a [Value] which doesn't make too much sense. I would love to have one for [Value] instead but Rust doesn't allow that today. It only exists for a single case to implement the include logic. Wanna get rid of that.

[SergioBenitez]

I agree that having to implement Object and AsSeq is a bit odd, but it's not entirely terrible.

Yeah, it's absolutely not terrible. But, are you opposed to Value::from_seq_object() and Value::from_struct_object() constructor methods? Internally they would simply wrap in a T: Object. Their presence would mean you don't need to implement Object unless you need to dynamically decide between being a struct or seq.

[SergioBenitez]

Some more general commentary. This looks awesome.

[SergioBenitez]

Since as_slice() is gone, what is this referring to?

[mitsuhiko]

The underlying ArgTypes cannot be implemented still. See the remaining internal uses of as_slice. I'm unsure how this can be resolved due to limitations with how lifetimes are entangled with value. Basically you cannot have a filter taking foo(items: &[Value]) or foo(items: Vec<i32>) when the items value was a dynamic sequence.

[SergioBenitez]

Should this also return Some() when we have a ValueRepr::Map? That would make its functionality similar to as_seq(). And if so, maybe StructObject should be called MapObject.

[mitsuhiko]

At a later point I might introduce a MapObject which takes arbitrary keys. However that opens up some uncomfortable questions about how to represent keys which I don't want to go into yet.

[mitsuhiko]

I ended up adding Value::from_seq_object and Value::from_struct_object. For people who want to just impersonate basic struct type maps and sequences they also come with a matching default implementation for debug and stringification that look exactly like the built-in sequences. That however means that the Arc for these is hidden and that downcasting does not work since the internal wrappers are not accessible.