nikis05 / derive-visitor

MIT License
23 stars 6 forks source link

impl Drive for String and some others #3

Closed badicsalex closed 2 years ago

badicsalex commented 2 years ago

I'm trying to use derive(Drive) with types generated by https://github.com/badicsalex/peginator/.

Unfortunately, a lot of these types contain String fields (and ops::range::RangeInclusive, but maybe that could be skipped) in ways that's not easy to detect (there are a lot of type alias shenanigans going on). This causes an error when using the derive macro.

Would it be possible to impl Drive for some more standard types? Or should this be solved differently?

nikis05 commented 2 years ago

The reason why the crate doesn't implement Drive for more std types is to discourage vague visitor methods, e.g.

#[derive(Drive)]
struct User {
    username: String,
}

#[derive(Drive)]
struct File {
    content: String,
}

In this example, both field values would be passed to visit_string method of a visitor, even though their contents are very much semantically different, and likely need to be treated differently. Current design forces the user to make an explicit choice between:

  1. Skipping a field and treating its containing structure as a "leaf", rather than treating the field itself as a "leaf":
#[derive(Drive)]
struct User {
    #[drive(skip)]
    username: String
}
  1. Making a field a newtype and making the visitor method semantically meaningful:
#[derive(Drive)]
struct User {
    username: Username
}

#[derive(Drive)]
struct Username(#[drive(skip)] String);

I can add a macro attribute e.g. #[drive(skip_all)] or #[drive(leaf)] that would be applied to the container struct and produce the same result as marking each field with skip. Furthermore there could be a "whitelist mode", i.e. all fields are skipped by default, but select fields are visited. Would it solve your issue?

badicsalex commented 2 years ago

I would hope anyone writing an enter_string function knows it's not going to be very "semantic".

Then again, I understand your concern that it shouldn't be misusable by default. For my case I'd be happy with an empty implementation too, but I guess that's bad for other reasons. Maybe behind a config flag?

(My use-case is that I generate structs from a DSL, where each field refers to another struct. Sometimes not a struct is generated for a specific name, but a type alias. This is done statelessly during generation, so it's hard to know what the fields' actual types will be. Using type aliases instead of newtypes is a core design choice unfortunately. Otherwise I guess could do your no 2. solution or just use #[drive(with=...)] for primitive-typed fields, or generate that skip manually)

nikis05 commented 2 years ago

Feature flag sounds like a good compromise to me, what std types would you want to implement Drive then?

badicsalex commented 2 years ago

All simple primitive types and String would be enough.

I actually drafted something here: https://github.com/badicsalex/derive-visitor/commit/8e3063588e324522ce4a44c40d5aff8e815a013b

(That macro might not be the most elegant solution.)

nikis05 commented 2 years ago

Looks good to me, could you create a PR?

badicsalex commented 2 years ago

Thanks!