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

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

Closed
tyranron opened this issue Aug 9, 2023 · 3 comments · Fixed by #291
Closed

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

tyranron opened this issue Aug 9, 2023 · 3 comments · Fixed by #291
Milestone

Comments

@tyranron
Copy link
Collaborator

tyranron commented Aug 9, 2023

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.

@tyranron tyranron added this to the 1.0.0 milestone Aug 9, 2023
@tyranron
Copy link
Collaborator Author

tyranron commented Aug 9, 2023

@JelteF what would you say on this?

@tyranron
Copy link
Collaborator Author

tyranron commented Aug 10, 2023

@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
Copy link
Owner

JelteF commented Aug 13, 2023

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)

JelteF pushed a commit that referenced this issue Oct 19, 2023
Resolves #287 

Allow using the same attribute arguments on fields as on the whole
struct, generate `impl`s for each

Co-authored-by: tyranron <[email protected]>
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 a pull request may close this issue.

2 participants