Skip to content
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

Add an enum_properties_lazy macro that allows arbitrary values. #6

Closed
wants to merge 1 commit into from

Conversation

Cryptjar
Copy link

The current enum_properties macro seems to require that the properties struct can be const initialized. This excludes properties that contain String or Vec or any other non-trivially initialized types. Otherwise, some error "returns a reference to data owned by the current function" is encountered.

This PR adds the enum_properties_lazy macro that support runtime initialized properties structs by utilizing lazy_static. lazy_static allows to initialize static data at runtime, lifting the const initialization constraint while still allowing to obtain a reference with static lifetime (which is needed in the defer function).

Since this introduces a new dependency, it is gated behind a crate feature, i.e. lazy_static is an optional dependency and enum_properties_lazy will only exist if it was enabled.

@cofinite
Copy link
Owner

Hey! Thank you for your interest in the crate. My chief concern with this PR is that the crate highly encourages one-by-one access of properties, and the introduction of a load operation on a synchronization primitive is not conducive to this pattern. Such loads are better performed without the syntactic sugar provided by this crate.

My second concern is that the two types you expressed interest in using within a static property, Vec and String, both already have const equivalents: &'static [] and &'static str. Each new example you provided initializes each Vec and String with a value that could've been used to initialize a &'static [] or &'static str instead. The PR does not demonstrate a compelling need for non-const initialization of static properties.

For these reasons, I am closing the PR.

@cofinite cofinite closed this Jan 15, 2022
@Cryptjar
Copy link
Author

Well, it's clear that lazy_static does add additional overhead, but it allows using more types. Admittedly, Vec and String were rather silly examples, they were just the first std types that came to my mind. I guess HashMap would be a better example. However, the type I'm actually interested in is EnumMap, just clarify.

If you stick to your opinion, I guess I'll just start a new crate, I think my use-case justifies it.

@cofinite
Copy link
Owner

cofinite commented Jan 15, 2022

It is strange that EnumMap is not a const-initializable type, considering it performs no heap allocation. Of course, in a perfect world, even heap allocating types would be able to be const-initialized, and none of the runtime overhead of synchronization primitives would be necessary here.

To clarify what I was talking about in the first paragraph of my last post, the syntactic sugar provided by this crate encourages not holding onto the reference to the properties struct, but rather reacquiring it every single time any property needs to be accessed. This pattern makes less sense the higher the cost of acquiring the reference to the properties struct.

Another issue is that wrapping the entire properties struct in a synchronization primitive means that accesses to properties which normally do not have the additional overhead now have it too.

These issues could be jointly fixed by separating the lazy properties from the const properties, and making access of the lazy properties explicit rather than automatic via Deref. What are your thoughts on this?

@cofinite cofinite reopened this Jan 15, 2022
@Cryptjar
Copy link
Author

Of course, in a perfect world, even heap allocating types would be able to be const-initialized, and none of the runtime overhead of synchronization primitives would be necessary here.

So true, I'm personally eager to see const_heap getting implemented, but I think that very long shoot.

To clarify what I was talking about in the first paragraph of my last post, the syntactic sugar provided by this crate encourages not holding onto the reference to the properties struct, but rather reacquiring it every single time any property needs to be accessed. This pattern makes less sense the higher the cost of acquiring the reference to the properties struct.

Sure thing, I do agree to the general premise, but I want to point out two things:

First, as you already indicated with by using comparative, this not a binary situation, it's more gradually, and the additional cost essentially boils down to a single atomic usize load, and once initialized, that memory is not going to change anymore meaning the contention on it will be low.

Second, it also depends on the use-case how much overhead is acceptable, e.g. some sparse accesses vs a lot in a tight loop. Thus, I already proposed to keep those two approaches separate, so one is free to choose whether one stays restricted to more efficient const data or pays to allow more general types.

Tho, I think, it could be worth to point out these implications/differences in the documentation.

Another issue is that wrapping the entire properties struct in a synchronization primitive means that accesses to properties which normally do not have the additional overhead now have it too.

Well, this is unfortunate.

These issues could be jointly fixed by separating the lazy properties from the const properties, and making access of the lazy properties explicit rather than automatic via Deref. What are your thoughts on this?

So, I assume you mean that the user can implement two property structs on the same enum, one const initialized would be accessed via deref and the other lazy_static one via, say, a custom get method. It sounds interesting, but at first it seems a bit hacky, I mean how would you even put this into the syntax of the current macro? (In my personal opinion, it is already a bit convoluted)


On the off chance of making this too long, I think, it is a good point to tell about another idea that I had. I got inspired to this via EnumDiscriminants, which takes an enum that might have additional data on its variants and creates a new enum that only has the variant names without any variant data, i.e. a C-like enum. I though, if I want to combine this with enum_properties, with the current macro, I could only add properties to the original enum with variant data. This is rather sad, because it means, I would need to get some valid variant data only to have access to the properties.

However, I think, we can improve on that: the current enum_properties macro really does only implement Deref on an enum. Thus, it seems quite logical to me to separate the enum definition from the Deref impl, e.g. see this playground (tho, admittedly the ergonomics seem to suffer a bit, I mean this way it looks close enough to a manual impl).

Adding to that I could image instead of simply implementing Deref, this crate could instead define some trait e.g.:

pub trait EnumProp<Prop> {
    fn get(&self) -> &'static Prop;
}

Allowing multiple property types on the same struct. Then one of them can be redirected to Deref while the others can only be access via the trait (tho, one could easily add inherent convenience methods redirecting to the corresponding trait method to avoid call ambiguity).

Having this trait, would allow to have a more unified macro independent on which impl gets used as Deref. Then seems more logical to me to define lazyness on such a per impl basis.

But of course these are just ideas.

@Cryptjar
Copy link
Author

I experimented a bit with my idea to separate the impls from the enum definition: see playground

The basic syntax so far would be like this:

enum Foo {
    Alpha,
    Beta(u8),
}

struct PropsConst {
    name: &'static str,
}
// a const impl & Deref
props!{
    const deref props PropsConst for crate::Foo {
        Self::Alpha => {
            name: "Alpha"
        }
        Self::Beta(42) => {
            name: "Best Beta"
        }
        Self::Beta(_) => {
            name: "Normal Beta"
        }
    }
}

struct PropsLazy {
    name: String,
}
// a lazy impl
props!{
    lazy props PropsLazy for crate::Foo {
        Self::Alpha => {
            name: [1,2,3].iter().map(ToString::to_string).collect()
        }
        Self::Beta(_) => {
            name: "Just Beta".into()
        }
    }
}

fn main() {
    assert_eq!(Foo::Alpha.name, "Alpha");
    // Using universal function call
    assert_eq!(EnumProp::<PropsLazy>::get(&Foo::Alpha).name, "123");
}

This just a draft at that point. It is still missing some features of the current macro, most notably default values, and there are still some open questions (apart from universal syntax bikeshedding, I'm also open for alternatives here):

  • Should the macro (automatically or optionally) implement an inherent method on the enum for non-deref impls to prevent the fallback to universal function call and how to specify its name?
  • Should the macro really allow arbitrary matches (such as the silly Beta(42))?
  • Should the macro also support static besides const? (also see the comments in the playground)
  • Maybe we could add a rule to map the old syntax to const deref props for backwards compatibility, if needed/wanted?
  • How about cfg-ing the lazy rules? (I suppose an alternative could be to just omit lazy_static from this crate's dependencies and use whatever lazy_static is in scope where the macro is expanded; works but is a bit fragile; obviously a down-stream crate that does not use any lazy rule will not need lazy_static in its dependencies)

@cofinite
Copy link
Owner

cofinite commented Jan 16, 2022

However, I think, we can improve on that: the current enum_properties macro really does only implement Deref on an enum. Thus, it seems quite logical to me to separate the enum definition from the Deref impl, e.g. see this playground (tho, admittedly the ergonomics seem to suffer a bit, I mean this way it looks close enough to a manual impl).

A pretty huge part of this crate is just making things as simple and succinct as possible, which is why everything is defined at the same time as the enum itself (you never have to write the name of a variant twice). So that kind of design I'd rule as being out of scope of this project, but I can see value in a separate crate providing something along those lines.

Regarding the non-const initialization problem, I'm investigating the possibility of pre-main initialization in the style of static_init. This kind of solution would, in my opinion, be ideal as it incurs no performance hit nor would it require any substantial changes in API.

@Cryptjar
Copy link
Author

A pretty huge part of this crate is just making things as simple and succinct as possible, which is why everything is defined at the same time as the enum itself (you never have to write the name of a variant twice). So that kind of design I'd rule as being out of scope of this project, but I can see value in a separate crate providing something along those lines.

Well, I see the appeal of the brevity of the current macro. But it comes with its limits, whereas my proposal gives a more general macro. However, I don't see them necessarily as mutually exclusive, I mean, they both could coexist. Thus, letting the user choose between the two variants. Additionally, I think they can also be composed together, for example using the current macro to concisely add a const-init deref property and then the more general version to just add another (e.g. lazy) property as well to the same enum.

However, if you insist, I could just spin-up a new crate.

Regarding the non-const initialization problem, I'm investigating the possibility of pre-main initialization in the style of static_init. This kind of solution would, in my opinion, be ideal as it incurs no performance hit nor would it require any substantial changes in API.

static_init sounds definitely interesting, however, I'm afraid it is less portable than the lazy_static approach, i.e. it seems it uses some OS specific features, thus I guess it wouldn't work on bare-metal or even in WebAssembly. However, it is imaginable to support both at the user's choice (i.e. having both as optional dependencies and using whatever has been enabled).

@cofinite
Copy link
Owner

However, if you insist, I could just spin-up a new crate.

I think your props macro is sufficiently different to be its own crate, and you have my blessing if you'd like to make it so. Especially since enum_properties and props would still work together.

@Cryptjar
Copy link
Author

Closing this PR, because I implement the macro that I proposed here in my own spin-off crate, I call it:

enumeraties

Also, see this example, where I show how both crates can cooperate.

I'm still working on it and plan then to release it on crates.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants