idanarye / rust-smart-default

Rust macro for automatically generating default
MIT License
133 stars 7 forks source link

Better support for String #5

Closed MightyPork closed 5 years ago

MightyPork commented 5 years ago

It would be nice if the library was a bit smarter about owned string fields.

#[derive(Serialize, Deserialize, Debug, SmartDefault)]
#[serde(default)]
pub struct Config {
    // doesn't work, but it could - just add .to_string(). a quickfix for this common case
    #[default = "localhost"]
    pub host: String,
    // what i thought would work. doesn't
    #[default = "localhost".to_string()]
    pub host: String,
    // using the special syntax, really surprised me this doesn't work
    #[default("localhost".to_string())]
    pub host: String,

    // this one finally works
    #[default(String::from("localhost"))]
    pub host: String,

    #[default=8000]
    pub port: u16,
}
idanarye commented 5 years ago

I can probably just strap an into() on every default literal, but we need to consider special usecases like:

trait Foo<T> {
    fn foo() -> T;
}

struct Bar<T>(T);

impl Foo<u32> for Bar<u32> {
    fn foo() -> u32 {
        1
    }
}
impl Foo<u16> for Bar<u16> {
    fn foo() -> u16 {
        2
    }
}

#[derive(smart_default::SmartDefault)]
struct Baz(
    #[default(Bar::foo())]
    u32,
);

This currently works, but if I strap .into() everywhere I'll get a default of Bar::foo().into() which can be either Bar::<u32>::foo().into() or Bar::<u16>::foo().into(). So I'm going to need to add an escape hatch - maybe:

#[derive(smart_default::SmartDefault)]
struct Baz(
    #[default(Bar::foo(), into=False)]
    u32,
);
MightyPork commented 5 years ago

If you want to be conservative, I think adding .into() only to &str would be enough for this case without breaking other things like your generics example. I would have tried to help, but I'm only just beginning with macro_rules!, this is way beyond my level.

idanarye commented 5 years ago

Maybe I have no choice. The case I showed is not common enough to justify limiting the .into() to &str, but I just checked and found a much more common case that gets broken by .into():

#[derive(smart_default::SmartDefault)]
struct Baz(
    #[default = 1]
    u32,
);

With Baz(1) Rust will know that 1 is u32, but with Baz(1.into()) Rust will assume that i is a i32 and then complain that i32 cannot be converted to u32.

So yea... I'll have to only add it to strings...

MightyPork commented 5 years ago

wanted to try this, but got an error:

error[E0658]: multiple patterns in `if let` and `while let` are unstable (see issue #48215)                                                                                                                                      
  --> /home/ondra/.cargo/registry/src/github.com-1ecc6299db9ec823/smart-default-0.5.0/src/default_attr.rs:81:9                                                                                                                   
   |                                                                                                                                                                                                                             
81 | /         if let Ok(syn::Lit::Str(_)) | Ok(syn::Lit::ByteStr(_)) = syn::parse::<syn::Lit>(code.clone().into()) {                                                                                                            
82 | |             // A string literal - so we need a conversion in case we need to make it a `String`                                                                                                                           
83 | |             return ConversionStrategy::Into;                                                                                                                                                                              
84 | |         }                                                                                                                                                                                                                 
   | |_________^ 

could you split it to two if-let's, or use a match?

edit: nevermind, I see 1.33 came out yesterday and it's working now. thanks!

idanarye commented 5 years ago

I think I'll still convert it to a match. No point to raise the version requirement for this...