-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alternative fallback fields implementation #441
base: main
Are you sure you want to change the base?
Conversation
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:
That way the feature can be tested and refined in parallel. Concerns
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. 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. |
I have added the requested changes along with making the naming more consistent (renaming the feature to dynamic fields from meta fields) I want to note it was actually fun to apply these changes as it allowed me to further understand the source and its flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat, and thanks for the quick turnaround!
So most of the feature flagging can be avoided. All that really needs to be feature flagged is:
- The option that enables the use of the different field get/set instructions in
Options
(see comment). - The enum variants in
DynamicFieldMode
which are notNever
(see comment). - Use of those other variants when deriving
Any
inrune-macros
(and the appropriate error if the feature is not enabled). Thesee comment.AnyObj
runtime support (just nice to avoid the vtable increase if it's not used but not super important).
It is perfectly all right for the instructions to exist alongside other instructions in the Vm. As long as their use is behind an option.
crates/rune/src/any.rs
Outdated
@@ -18,7 +22,32 @@ pub use rune_macros::Any; | |||
/// name: String, | |||
/// } | |||
/// ``` | |||
pub trait Any: Named + MetaFieldMode + std::any::Any { | |||
#[cfg(not(feature = "dynamic_fields"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, avoid feature gating two incompatible traits of Any
. What can happen is that you have coherence issues under a scenario such as:
Crate a
defines a type that implements Any
without activating the dynamic_fields
feature, so they leave FIELD_MODE
undefined.
Crate b
defines a type that implements Any
while activating the dynamic_fields
feature, so they define FIELD_MODE
.
Crate c
depends on a
and b
, so the dynamic_fields
feature is activated, causing a
to fail to build.
Instead:
- Unconditionally include
FIELD_MODE
(and possibly rename toDYNAMIC_FIELD_MODE
:)). - Feature gate the other variants of
DynamicFieldMode
which are expected to cause alternate behaviors (First
,Last
, andOnly
). - Mark
DynamicFieldMode
as#[non_exhaustive]
. This prevents dependents which have not enabled the feature from using it in a manner which is incompatible with the feature being enabled.
crates/rune/src/compile/options.rs
Outdated
@@ -61,6 +65,10 @@ impl Options { | |||
Some("v2") => { | |||
self.v2 = it.next() != Some("false"); | |||
} | |||
#[cfg(feature = "dynamic_fields")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be confusing if it fails to parse despite being in the documentation.
Instead:
- Return a special parse error indicating that the option exists, but it's not supported without enabling a feature; or,
- Emit a warning through
Diagnostics
during a build if thedynamic_fields
option is used but the feature is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call.
crates/rune-macros/src/any.rs
Outdated
#[cfg(feature = "dynamic_fields")] | ||
let meta_fields = Some(quote! {Never}); | ||
#[cfg(not(feature = "dynamic_fields"))] | ||
let meta_fields = None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional simplifications in rune-macros
would be possible when we go for the other feature gating strategy I'm proposing. So I'm not gonna comment on them now ;).
Actually, no this can't be done. |
d6da63b
to
d0ccc8d
Compare
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good, a few nits!
crates/rune-cli/src/main.rs
Outdated
@@ -50,7 +50,7 @@ | |||
//! [rune]: https://github.com/rune-rs/rune | |||
|
|||
use anyhow::{anyhow, Result}; | |||
use rune::compile::ParseOptionError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to rename the error type, you've only turned it into an enum which is still one error (one of its variants). "Errors" gives the impression that it's a collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem I can change that easily.
crates/rune-macros/src/context.rs
Outdated
self.errors.push(syn::Error::new_spanned( | ||
s, | ||
"attempted to set dynamic_fields to `only` while the feature `dynamic_fields` is disabled." | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation here seems a bit off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnily enough it looked perfect until cargo fmt decided to make it look like that lol, I'll appease it shortly.
crates/rune-macros/src/context.rs
Outdated
attrs.meta_fields = Some(match s.value().as_str() { | ||
"never" => format_ident!("Never"), | ||
"only" => { | ||
if cfg!(feature = "dynamic_fields") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this kind of test is repeated three times it might be worth breaking out into a function.
crates/rune/src/compile/options.rs
Outdated
|
||
/// Allow use of META_FIELD_GET and META_FIELD_SET | ||
#[cfg(feature = "dynamic_fields")] | ||
pub dynamic_fields: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub(crate)
since it has a mutator method (else we can set it directly).
The mutator method can be feature gated. This field does not need to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking that but I wasn't 100%.
crates/rune/src/compile/options.rs
Outdated
required_feature: "dynamic_fields", | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cfg!(feature = "dynamic_fields") { /* */ } else { /* */ }
is probably cleaner here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cfg!(feature = "dynamic_fields") { /* */ } else { /* */ }
is probably cleaner here.
Yeah easily sorted; it is just like this due to ObjectIndexDynamicGet
originally being feature gated so it wouldn't compile with that if statement lol (kind of wish rust had a macro which mimicked if else cfg macro but actually ignored the invalid branch since its more sane to read.
}, | ||
span, | ||
); | ||
#[cfg(not(feature = "dynamic_fields"))] | ||
c.asm.push(Inst::ObjectIndexSet { slot }, span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving into a function which is feature gated with two different implementations instead?
Block-level feature gates are a bit awkward to deal with in my experience.
Something like:
push_field_lookup(c, slot);
}, | ||
span, | ||
); | ||
#[cfg(not(feature = "dynamic_fields"))] | ||
c.asm.push(Inst::ObjectIndexGet { slot }, span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
crates/rune/src/runtime/protocol.rs
Outdated
/// Search for a field by `String` | ||
pub const DYNAMIC_FIELD_GET: Protocol = Protocol { | ||
name: "dynamic_field_get", | ||
hash: Hash::new(0x6dda58b140dfeaf9), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how did you generate these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how did you generate these?
I used the Inst hasher and used a name which wouldn't be allowed as a field name.
crates/rune/src/runtime/vm.rs
Outdated
CallResult::Unsupported(target) => CallResult::Unsupported(target), | ||
} | ||
} | ||
#[cfg(feature = "dynamic_fields")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of these big featured off blocks but not much to be done about them either. Maybe consider if these sections can be cleaned up a bit (somehow) in the future. But I'm not sure how right now.
crates/rune/src/runtime/vm.rs
Outdated
} | ||
} else { | ||
CallResult::Ok(result) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preferred style in instances like thse would be this to save some of the indentation:
let result = match result {
Value::Option(result) => result,
_ => return Ok(CallResult::Ok(result)),
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I agree, was just bit tired during the coding session lol.
I'll add the requested changes when I get back later as I am busy today, thank you for the review <3 |
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. |
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. |
I'll be back after this weekend and then I'll take a look! Cheers! |
3fd1072
to
c9a61dd
Compare
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. |
Naming can always be punted on, initially this will be released as a That being said, after digging into this now I'm prone to cutting down this feature by only supporting 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. There's also a lack of reflexivity in the features. Only 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. |
fa8d2ff
to
8329943
Compare
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 |
What I intended was that if
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 |
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. |
Hey there all... found this trying to see if rune supported this particular scenario, any chance that this or similar behavior will land soon? |
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: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
andOnly
exist means there needed to be a change to their implementation.DYNAMIC_FIELD_GET
now handlesOption
s slightly differently: if you returnNone
it will presume there was no field,Some(None)
is still a option if desired. The value will be unwrapped from theOption
on the callers side so doesn't need to be worried about by the script writer.DYNAMIC_FIELD_SET
is basically the same asDYNAMIC_FIELD_GET
however instead of aOption
you can choose to returnfalse
which will be treat as a failure to set the field, allowing it to try and set the static field next if the type isFirst
or simply return a error otherwise as expected with static fields.For both of the above changes to
DYNAMIC_FIELD_SET
andDYNAMIC_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:
implementing fallible DYNAMIC_FIELD_GET and DYNAMIC_FIELD_SET
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
After
Closes
#381 #263