Alternative fallback fields implementation

[LoopyAshy]

This is an alternative implementation to #440. This implementation is notably different in that it is more controllable by the implementor and feels more like a first class feature. The main changes are the addition of types deriving Any to state the priority of dynamic fields vs actual fields. The options are as follows:

• First - Will try to find a dynamic field first before checking static fields.
• Last - Will try to find a static field first before checking dynamic fields.
• Never - Will completely disregard any dynamic fields and exclusively use static fields. (Same behavior as prior to this implementation and the default)
• Only - Will completely disregard static fields and exclusively use dynamic fields.

All of these can be applied to any type using the Derive(Any) macro by applying the attribute #[rune(dynamic_fields="never|only|first|last")] to the struct itself.

Dynamic fields functionality is basically the same as the #440 however due to the fact First and Only exist means there needed to be a change to their implementation. DYNAMIC_FIELD_GET now handles Options slightly differently: if you return None it will presume there was no field, Some(None) is still a option if desired. The value will be unwrapped from the Option on the callers side so doesn't need to be worried about by the script writer. DYNAMIC_FIELD_SET is basically the same as DYNAMIC_FIELD_GET however instead of a Option you can choose to return false which will be treat as a failure to set the field, allowing it to try and set the static field next if the type is First or simply return a error otherwise as expected with static fields. For both of the above changes to DYNAMIC_FIELD_SET and DYNAMIC_FIELD_GET: you can return any other type (including () of course) and it will not treat it any differently to before.

Examples

A struct with dynamic fields which are prioritised over static fields:

#[derive(Any, Clone)]
#[rune(dynamic_fields="first")]
struct TestStruct {
    values: HashMap<String, u32>,
    b: u32, 
    c: u32
}

implementing fallible DYNAMIC_FIELD_GET and DYNAMIC_FIELD_SET

let mut module = Module::new();

module.ty::<TestStruct>()?;
module.inst_fn(
    Protocol::DYNAMIC_FIELD_GET,
    |this: &TestStruct, key: String| {
        if let Some(v) = this.values.get(&key) {
            Some(*v)
        } else {
	    None
	}
    },
)?;
module.inst_fn(
    Protocol::DYNAMIC_FIELD_SET,
    |this: &mut TestStruct, key: String, value: u32| {
        if key == "nerd" {
            false
        } else {
            this.values.insert(key, value);
            true
	}
    },
)?;

Reasoning

Sometimes it is desired for user readability to pretend a specific custom type has fields and instead accessing the data from elsewhere such as from a HashMap or another collection. possible use cases is having types with dynamic layouts while having rules to their usage such as locking the layout once created. it also allows custom types to be dynamically extended by the scripter while keeping the benefits of being a static Shared<AnyObj> but not seeming like a collection which [] accesses give the impression of.

Benchmarks

Note Due to the tiny footprints of the functions, the differences are very hard to see if any and can be improved within my benchmarks with a faster hashmap regardless but honestly values these small are easily within margin of error.

Before

test fields::actual_field_get                    ... bench:         418 ns/iter (+/- 88)
test fields::actual_field_set                    ... bench:         434 ns/iter (+/- 161)

test fields::string_key_classic_index_get        ... bench:         523 ns/iter (+/- 52)
test fields::static_string_key_classic_index_get ... bench:         486 ns/iter (+/- 80)

After

test meta_fields::actual_field_get_first             ... bench:         614 ns/iter (+/- 18)
test meta_fields::actual_field_get_last              ... bench:         420 ns/iter (+/- 46)
test meta_fields::actual_field_get_never             ... bench:         414 ns/iter (+/- 27)
test meta_fields::actual_field_set_first             ... bench:         514 ns/iter (+/- 63)
test meta_fields::actual_field_set_last              ... bench:         427 ns/iter (+/- 14)
test meta_fields::actual_field_set_never             ... bench:         408 ns/iter (+/- 28)
test meta_fields::actual_field_set_only              ... bench:         515 ns/iter (+/- 16)
test meta_fields::static_string_index_get_first      ... bench:         467 ns/iter (+/- 14)
test meta_fields::static_string_index_get_last       ... bench:         471 ns/iter (+/- 35)
test meta_fields::static_string_index_get_never      ... bench:         465 ns/iter (+/- 43)
test meta_fields::static_string_index_get_only       ... bench:         459 ns/iter (+/- 17)
test meta_fields::static_string_meta_field_get_first ... bench:         531 ns/iter (+/- 64)
test meta_fields::static_string_meta_field_get_last  ... bench:         546 ns/iter (+/- 31)
test meta_fields::static_string_meta_field_get_only  ... bench:         518 ns/iter (+/- 35)
test meta_fields::static_string_meta_field_set_first ... bench:         525 ns/iter (+/- 231)
test meta_fields::static_string_meta_field_set_last  ... bench:         545 ns/iter (+/- 42)
test meta_fields::static_string_meta_field_set_only  ... bench:         515 ns/iter (+/- 34)
test meta_fields::string_index_get_first             ... bench:         504 ns/iter (+/- 24)
test meta_fields::string_index_get_last              ... bench:         505 ns/iter (+/- 42)
test meta_fields::string_index_get_never             ... bench:         504 ns/iter (+/- 17)
test meta_fields::string_index_get_only              ... bench:         501 ns/iter (+/- 20)
test meta_fields::string_meta_field_get_first        ... bench:         552 ns/iter (+/- 21)
test meta_fields::string_meta_field_get_last         ... bench:         595 ns/iter (+/- 26)
test meta_fields::string_meta_field_get_only         ... bench:         558 ns/iter (+/- 26)
test meta_fields::string_meta_field_set_first        ... bench:         508 ns/iter (+/- 23)
test meta_fields::string_meta_field_set_last         ... bench:         553 ns/iter (+/- 39)
test meta_fields::string_meta_field_set_only         ... bench:         510 ns/iter (+/- 23)

Closes

#381 #263

[udoprog]

I'm gonna have to think this over. It's not a style choice that I would choose, so if I include it I'd like to see the following changes done:

• Introduce your feature as extra Vm instructions which are feature gated. So instead of rewriting the ObjectSlotIndexGet instruction, add a new one which uses the new field loading convention.
• Feature gate the extra plumbing in AnyObj which allows for dynamic index get conventions.
• Add a compiler option which changes how index gets are performed so that they make use of the new feature. This would allow you to use the new feature, but it wouldn't be enabled by default.

That way the feature can be tested and refined in parallel.

Concerns

1. Dynamically deciding on the field loading convention in use is a form of metaprogramming which doesn't obviously mesh well with a future gradual type system. 2. Having dynamic field loading conventions might make future compile time optimizations harder or not possible.

Both of these are future potentials and not current blockers, which is why I wouldn't mind including the feature on an experimental basis to get a feel for it. Maybe improve it and mainline include in the future.

Thank you!

[LoopyAshy]

I'm gonna have to think this over. It's not a style choice that I would choose, so if I include it I'd like to see the following changes done:

• Introduce your feature as extra Vm instructions which are feature gated. So instead of rewriting the ObjectSlotIndexGet instruction, add a new one which uses the new field loading convention.
• Feature gate the extra plumbing in AnyObj which allows for dynamic index get conventions.
• Add a compiler option which changes how index gets are performed so that they make use of the new feature. This would allow you to use the new feature, but it wouldn't be enabled by default.

That way the feature can be tested and refined in parallel.

Concerns

1. Dynamically deciding on the field loading convention in use is a form of metaprogramming which doesn't obviously mesh well with a future gradual type system. 2) Having dynamic field loading conventions might make future compile time optimizations harder or not possible.

Both of these are future potentials and not current blockers, which is why I wouldn't mind including the feature on an experimental basis to get a feel for it. Maybe improve it and mainline include in the future.

Thank you!

Thank you for your reply. I am happy to make those changes and will do so when I am free.

Regarding you concerns: I do believe it shouldn't make it completely impossible but I can see the possibility of increasing the complexity of the compilation, if it does come to that I am however more than happy to try and assist in the ease of implementation as this is a QoL feature that I know will be very useful in quite a few of my projects for example in one of my projects the types are generated at load time and stored as templates and generated dynamically at runtime when requested and are optimised around these facts. Reason for this is so the project doesn't need recompiled as it relies on third party data types which are very often change but the layouts are available, however there layouts are statically typed so if I gave a float instead of a int it would be incorrect, so I have to check the typing and field name validity etc.

When I was using Lua for this project I simply overwrote the meta function for getting and setting fields and it worked like any other type and looked no different, it was seamless and effective. When moving to rune due to it having many many preferred design decisions over lua, it was only possible by having dedicated instance functions or using INDEX_* which means using ["field_name"] which makes it look strange to the script writers who think of these types are true types not HashMaps or collections.

[LoopyAshy]

I have added the requested changes along with making the naming more consistent (renaming the feature to dynamic fields from meta fields) Let me know what you think and if anything requires changes or clean up to meet the desired structure. I have run the previous benchmarks and no changes have been found with the previous implementation.

I want to note it was actually fun to apply these changes as it allowed me to further understand the source and its flow.

[udoprog]

The AnyObj runtime support (just nice to avoid the vtable increase if it's not used but not super important).

Actually, no this can't be done. AnyObjVtable is public API! So it would suffer the same issues as the Any trait. The field needs to be unconditionally included.

[LoopyAshy]

I have applied the requested changes, did take me a while due to how long I been coding today lol so sorry if I made any silly mistakes but all the tests passed and the benches are stable.

[udoprog]

Don't sweat it! And don't feel like you have to make requested changes immediately. I don't have any plans to change things in Rune for at least a week or two ;).

[LoopyAshy]

Don't sweat it! And don't feel like you have to make requested changes immediately. I don't have any plans to change things in Rune for at least a week or two ;).

Oh it is not for that reason lol, I just like getting stuff sorted, and when I get stuck in I usually stay focused on the task for good while. I am just glad it is all going smoothly; this is the first public repo I ever made a PR for due to confidence issues honestly, private is another thing all together though :D.

[LoopyAshy]

I'll add the requested changes when I get back later as I am busy today, thank you for the review <3

[LoopyAshy]

I been busy past few days but I added the requested changes and forgot to mention it lol, I also added a tiny idea for the clean up but admittedly I ain't the most fond of it and I am happy to revert that change if you feel it is too out of place, honestly though it is quite fiddly to clean up admittedly.

I have been using these changes on one of my projects to test the usability and ease of implementation etc, and honestly it has been a very smooth experience and I haven't ran into any problems yet, which is a wonderful sentence to type lol.

[LoopyAshy]

My bad, I genuinely completely forgot to push my changes to tests. I am surprised I didn't notice sooner as I had them ready but I ran the tests myself and completely blanked out lol.

Fixed now <3

Honestly though I should really clean the tests up later as they are horrible to read and refactor.

[udoprog]

I'll be back after this weekend and then I'll take a look!

Cheers!

[LoopyAshy]

I have updated the PR to the latest version of rune however I have questions before we decide to add this. mostly in regards to naming; what name for this feature would be most appropriate for it? Fallback_Set/Get, Meta_Set/Get, Dynamic_Set/Get, Dynamic_Field_Set/Get, Metafield_Set/Get, etc, etc. there is many options and I feel it should be consistent throughout but I was unable to decide a name I was completely satisfied with.

[udoprog]

Naming can always be punted on, initially this will be released as a --cfg flag, because enabling it as a feature could end up changing the behavior of the project in unrelated places. Features either have to be adopted wholesale and define the semantics of the language or optionally opted into like that.

That being said, after digging into this now I'm prone to cutting down this feature by only supporting DYNAMIC_FIELD_GET / DYNAMIC_FIELD_SET. If any of these protocols are registered it indicates that the implementer takes over full control of field getting and setting.

I find the semantics as it currently stands with the different modes to be more confusing to describe than such a method, and both would ultimately support the same set of use cases. E.g. If first is set and DYNAMIC_FIELD_GET returns None then it falls back to GET.

There's also a lack of reflexivity in the features. Only DYNAMIC_FIELD_GET supports falling back to default GET, DYNAMIC_FIELD_SET does not because that would involve some kind of sentinel value indicating whether the field was set or not.

So this is an instance where I think it's better to define the behavior in code, rather than metadata since it both leads to clearer semantics, and a smaller change. To this end I've pushed a suggestion I'd like you to take for a spin.

[LoopyAshy]

I am fine with this. I must ask though: Do you basically think that instead of modes existing just simply have a flag and treat all DYNAMIC_FIELD_GET's and DYNAMIC_FIELD_SET's as First? or do you think Only would be more appropriate, the reasoning being that I had Last, First etc was purely for times when you feel the type will be more likely used with DYNAMIC_FIELD_GET vs GET. I could if First becomes only option also potentially make DYNAMIC_FIELD_SET handle returning Option (same as DYNAMIC_FIELD_GET) to fallback to SET which to be honest I thought I had implemented regardless. Just checked my DYNAMIC_FIELD_SET implementation and it does allow fallback but it relies on a bool (false means fallback) but I can change that to be clearer if you feel it would be appropriate.

[udoprog]

What I intended was that if DYNAMIC_FIELD_GET is defined for a type, it's responsible for returning the field. It will not internally fall back to GET.

Just checked my DYNAMIC_FIELD_SET implementation and it does allow fallback but it relies on a bool (false means fallback) but I can change that to be clearer if you feel it would be appropriate.

I missed this! But the broader point would still be that the semantics are bit messy. If an implementor wants to fall back to e.g. calling GET themselves I think it's cleaner to give them access to the equivalent of Python's getattr rather than complicating the language implementation.

[LoopyAshy]

That is all good I will just have to think of how to implement the function so it's easily known to the users and works properly. Only reason I had it implemented the way I did was it didn't require any extra thought by the implementer but I can see the desire for it to be more explicit and customizable.

My main concern which was the rationale behind my implementation choices was to avoid paying for what you don't use, aka if a type has no dynamic fields then checking if it does by going through a lookup etc would obviously come with a cost which would be pointless and potentially cache dirtying, especially when custom field logic is arguably rare in when it is needed.

[jlewallen]

Hey there all... found this trying to see if rune supported this particular scenario, any chance that this or similar behavior will land soon?