JelteF/derive_more

`derive(Into)` lost the ability to convert into particular fields

tyranron opened this issue · 3 comments

Problem description

In our codebase we have the following code:

#[derive(AsRef, Clone, Debug, Default, Eq, Into)]
pub struct Data<Id> {
    id: OnceCell<Id>,
    #[as_ref]
    #[into]
    raw: String,
}

It worked totally fine on 0.99.* derive_more versions. But after upgrading to 1.0.0-beta.3 it stopped to compile, complaining about #[into] attribute being incorrect:

error: expected attribute arguments in parentheses: #[into(...)]
   --> whatever/paths/src/foo.rs:118:7
    |
118 |     #[into]
    |       ^^^^

Even if we change it as:

#[derive(AsRef, Clone, Debug, Default, Eq, Into)]
pub struct Data<Id> {
    id: OnceCell<Id>,
    #[as_ref]
    #[into(String)]
    raw: String,
}

It still doesn't work:

error: expected `skip`, found: `String`
   --> whatever/paths/src/foo.rs:118:7
    |
118 |     #[into(String)]
    |  

Seems like this ability was totally overlooked in #248 reimplementation and was never mentioned in documentation.

Proposed solution

Support #[into] and #[into(<types>)] attributes on field-level for derive(Into) macro. Mention it in docs, and cover with tests.

Design

Some notes about how it should interact with existing derive(Into) features:

#[derive(Into)]
struct Foo {
   a: u8,
   b: String,
}
// Generated impls:
// - impl From<Foo> for (u8, String)


#[derive(Into)]
struct Foo {
   a: u8,
   #[into]
   b: String,
}
// Generated impls:
// - impl From<Foo> for String


#[derive(Into)]
struct Foo {
   a: u8,
   #[into(Box<str>)]
   b: String,
}
// Generated impls:
// - impl From<Foo> for Box<str>


#[derive(Into)]
struct Foo {
   #[into(owned(u64), ref)]
   a: u8,
   b: String,
}
// Generated impls:
// - impl From<Foo> for u64
// - impl From<&Foo> for &u8


#[derive(Into)]
#[into(owned, ref((u8, str)), ref_mut)]
struct Foo {
   #[into(owned(u64), ref)]
   a: u8,
   b: String,
   #[into(skip)]
   _c: PhantomData<Whatever>, 
}
// Generated impls:
// - impl From<Foo> for (u8, String)
// - impl From<&Foo> for (&u8, &str)
// - impl From<&mut Foo> for (&mut u8, &mut String)
// - impl From<Foo> for u64
// - impl From<&Foo> for &u8

And, maybe, it has sense to allow this case:

#[derive(Into)]
#[into]
struct Foo {
   #[into(owned(u64), ref)]
   a: u8,
   #[into]
   b: String,
   #[into(skip)]
   _c: PhantomData<Whatever>, 
}
// Generated impls:
// - impl From<Foo> for (u8, String)
// - impl From<Foo> for u64
// - impl From<&Foo> for &u8
// - impl From<Foo> for String

So, we have the parity of how #[into(<types>)] attribute works on top-level and field-level.

@JelteF what would you say on this?

@JelteF I've also been thinking about slightly changed semantics, but unsure whether it has sense:

At the moment, top-level #[into] attribute reflects either conversion into a value directly (in case one field is used), or into a tuple of values (in case of multiple fields). This makes behaving this

#[derive(Into)]
#[into(u16)]
struct Foo {
   a: u8,
}

and this

#[derive(Into)]
struct Foo {
   #[into(u16)]
   a: u8,
}

are effectively the same

// Generated impls:
// - impl From<Foo> for u16

The proposal is to make the top-level attribute always responsible for tuples, so this

#[derive(Into)]
#[into(u16)]
struct Foo {
   a: u8,
}

will generate this (single-field tuple)

// Generated impls:
// - impl From<Foo> for (u16,)

While the field-level attribute is always responsible for direct type conversion

#[derive(Into)]
struct Foo {
   #[into(u16)]
   a: u8,
}
// Generated impls:
// - impl From<Foo> for u16

And, following this logic, let's describe the whole list of combinations:

#[derive(Into)]
struct Foo {
   a: u8,
}
// Generated impls:
// - impl From<Foo> for u8


#[derive(Into)]
#[into]
struct Foo {
   a: u8,
}
// Generated impls:
// - impl From<Foo> for (u8,)


#[derive(Into)]
#[into(owned(u16), ref)]
struct Foo {
   a: u8,
}
// Generated impls:
// - impl From<Foo> for (u16,)
// - impl From<&Foo> for (&u8,)


#[derive(Into)]
#[into]
struct Foo {
   #[into(owned(u16), ref)]
   a: u8,
}
// Generated impls:
// - impl From<Foo> for (u8,)
// - impl From<Foo> for u16
// - impl From<&Foo> for &u8

#[derive(Into)]
struct Foo {
   a: u8,
   b: String,
}
// Generated impls:
// - impl From<Foo> for (u8, String)

#[derive(Into)]
#[into]
struct Foo {
   a: u8,
   #[into]
   b: String,
}
// Generated impls:
// - impl From<Foo> for (u8, String)
// - impl From<Foo> for String

#[derive(Into)]
#[into(owned, ref)]
struct Foo {
   #[into(owned(u64), ref)]
   a: u8,
   #[into(Box<str>)]
   b: String,
   #[into(skip)]
   _c: PhantomData<Whatever>, 
}
// Generated impls:
// - impl From<Foo> for (u8, String)
// - impl From<&Foo> for (&u8, &String)
// - impl From<Foo> for u64
// - impl From<&Foo> for &u8
// - impl From<Foo> for Box<str>

This removes ambiguity from the macro API and extends its use-cases.

JelteF commented

tl;dr Sounds like a good plan. Let's add it as a breaking change to the changelog. Let's not do the single element tuple suggestion.

Okay, I read the problem and agree that this feature was missed in #248. For context, how it worked before is that adding #[into] to a field was the same as adding #[into(skip)] to all fields that did not have an #[into] attribute. So the newly suggested approach acts differently than the previous approach in this case:

#[derive(Into)]
struct Foo {
   #[into]
   a: u8,
   #[into]
   b: String,
   _c: PhantomData<Whatever>, 
   _d: PhantomData<Whatever>, 
   _e: PhantomData<Whatever>, 
}
// 0.99.x generated impls:
// - impl From<Foo> for (u8, String)

// newly generated impls:
// - impl From<Foo> for u8
// - impl From<Foo> for String

I'm wondering how common this type of usage is. It seems rather exotic, so I wouldn't mind causing this change in behaviour. Especially since it's possible to get the old behaviour by adding #[into(skip)] to the phantomdata fileds. But it should have a note in the "Breaking changes" section of the changelog.

I quite like the new syntax+behaviour you proposed. It's still unable to encode all Into derives that people might want. E.g. A struct with 3 fields cannot have an impl for a tuple with 3 items and one with 2, but that seems okay. I think the new behaviour handles at least 90% of the Into impls people would want to write.

Regarding your latest comment which discusses generating an impl for (u16,):

#[derive(Into)]
#[into(u16)]
struct Foo {
   a: u8,
}

I think we should not do that. I can't really think of a reason why someone would want to generate a single item tuple. So I'd rather have the thing that people likely want (just the item, no tuple) work with an attribute either on the field or on the full struct. Especially for newtypes it seems quite annoying to have to put the attribute on the field.

// this looks quite ugly imho
#[derive(Into)]
struct Foo(#[into(u16)] u8)

// this looks much nicer imho
#[derive(Into)]
#[into(u16)]
struct Foo(u8)