Redesign `From` derive macro #241

ilslv · 2023-02-17T09:03:07Z

Synopsis

This PR is a part of replacing all attributes having syn::Meta syntax to custom parsing.

Solution

Replace #[from(types(i32, "&str"))] with #[from(i32, &str)] and add support for deriving multi-field structs and enum variants with #[from((<tuple>), (<tuple>), ...)].

Checklist

Documentation is updated (if required)
Tests are added/updated (if required)
CHANGELOG entry is added (if required)

…haustive`

…ature

ilslv · 2023-02-17T09:45:20Z

@tyranron @JelteF as I was reworking From attributes to accept non-string expressions, I've found some design decisions that seem suboptimal to me.

Enum #[from(forward)] and #[from(types)] automatically ignores all other fields:

#[derive(From)]
enum Value {
    #[from(forward)]
    Int(i32),
    String(String),
}

// Expands to
#[automatically_derived]
impl<__FromT0> ::core::convert::From<(__FromT0)> for Value
where
    i32: ::core::convert::From<__FromT0>,
{
    #[inline]
    fn from(original: (__FromT0)) -> Value {
        Value::Int(<i32 as ::core::convert::From<__FromT0>>::from(original))
    }
}

#[derive(From)]
enum Value {
    #[from(types(i8))]
    Int(i32),
    String(String),
}

// Expands to
#[automatically_derived]
impl ::core::convert::From<(i32)> for Value {
    #[inline]
    fn from(original: (i32)) -> Value {
        Value::Int(original)
    }
}

#[automatically_derived]
impl ::core::convert::From<(i8)> for Value {
    #[inline]
    fn from(original: (i8)) -> Value {
        Value::Int(<i32 as ::core::convert::From<i8>>::from(original))
    }
}

It's possible to manually specify other fields with #[from] or #[from(forward)], but it seems to contradict with behaviour without those attributes

#[derive(From)]
enum Value {
    #[from(forward)]
    Int(i32),
    #[from]
    String(String),
}

#[derive(From)]
enum Value {
    #[from(forward)]
    Int(i32),
    #[from(forward)]
    String(String),
}

from(types(...)) automatically adds field type itself

#[derive(From)]
#[from(types(u8))]
struct Int(u64);

#[automatically_derived]
impl ::core::convert::From<(u64)> for Int {
    #[inline]
    fn from(original: (u64)) -> Int {
        Int(original)
    }
}

#[automatically_derived]
impl ::core::convert::From<(u8)> for Int {
    #[inline]
    fn from(original: (u8)) -> Int {
        Int(<u64 as ::core::convert::From<u8>>::from(original))
    }
}

#[from(types(...))] for multi-field structs automatically uses single each attribute field for all structs fields

#[derive(From)]
#[from(types(i8))]
struct Point(i32, i32);

#[automatically_derived]
impl ::core::convert::From<(i32, i32)> for Point {
    #[inline]
    fn from(original: (i32, i32)) -> Point {
        Point(original.0, original.1)
    }
}

#[automatically_derived]
impl ::core::convert::From<(i8, i8)> for Point {
    #[inline]
    fn from(original: (i8, i8)) -> Point {
        Point(
            <i32 as ::core::convert::From<i8>>::from(original.0),
            <i32 as ::core::convert::From<i8>>::from(original.1),
        )
    }
}

tyranron · 2023-02-17T11:02:34Z

@ilslv

Enum #[from(forward)] and #[from(types)] automatically ignores all other fields:

#[from(forward)] does this, I guess, because it produces a blanket impl, which inevitably will conflict with other impls.

#[from(types)], I guess, does this by mistake, because was made out of #[from(forward)] once.

from(types(...)) automatically adds field type itself

This is intended behavior. Do you have better suggestions?

#[from(types(...))] for multi-field structs automatically uses single each attribute field for all structs fields

The same here. It was in the initial design to work with tuples.

ilslv · 2023-02-17T11:11:56Z

@tyranron

#[from(types)], I guess, does this by mistake, because was made out of #[from(forward)] once.

👍

from(types(...)) automatically adds field type itself

This is intended behavior. Do you have better suggestions?

Explicitly including field type

#[from(types(...))] for multi-field structs automatically uses single each attribute field for all structs fields

The same here. It was in the initial design to work with tuples.

Looks very counterintuitive and niche to me, I would prefer something like:

#[derive(From)]
#[from(types((i8, i8), (i16, i16)))]
struct Point(i32, i32);

tyranron · 2023-02-17T11:42:19Z

@ilslv

Explicitly including field type

This may have sense, since makes things more straightforward and allows more (excluding direct underlying type from impls).

Looks very counterintuitive and niche to me, I would prefer something like:
#[derive(From)]
#[from(types((i8, i8), (i16, i16)))]
struct Point(i32, i32);

But here I'm not sure what it gives except making ergonomics worse.

Anyway, we need @JelteF's opinion on that.

JelteF · 2023-02-20T23:40:37Z

Enum #[from(forward)] and #[from(types)] automatically ignores all other fields:

Thinking back, I think my original reasoning was that I liked it that you could easily specify variants explicitly by specifying the #[from] attribute (without any arguments). The fact that forward almost certainly conflicts might have played a role too. I don't think the types really came in to that thought. I think I'd prefer keeping it as is. Having no attributes at all being a special case, where automatically all variants are enabled. But once you specify attributes, you should specify them all. Or you should specify a default attribute for all variants like:

#[derive(From)]
#[from]
enum Value {
    #[from(forward)]
    Int(i32),
    String(String),
}

from(types(...)) automatically adds field type itself

I like that making it explicit you can choose not to derive the From for the actual type. But I'm strongly wondering how often that's applicable in practice. I think most people will want to derive additive for types. No-one has opened an issue so far that they couldn't do it. And maybe the better way to do so would be to allow #[from(ignore, types(...)) to ignore the default derive.

#[from(types(...))] for multi-field structs automatically uses single each attribute field for all structs fields

This one I actually think makes sense to change. There's many types you currently cannot specify. And strangely it doesn't even error if you currently pass a tuple. It simply doesn't derive anything and completely ignores the tuple. One example that I'd like to be able to derive is:

#[derive(From)]
#[from(types(i8, &str))]
struct SomeMixedType(i32, String);

To summarize: I'd keep 1 and 2 as is, but 3 I think makes sense to change.

ilslv · 2023-02-21T06:34:40Z

@JelteF

And maybe the better way to do so would be to allow #[from(ignore, types(...)) to ignore the default derive.

I think that combination of types and ignore may be quite confusing:

#[derive(From)]
enum Value {
    #[from(ignore, types(i8))]
    Int(i32),
    #[from(ignore)]
    String(String),
}

And even if user understands whats going on here, discovering that on their own is quite unlikely.

tyranron · 2023-02-21T12:42:29Z

@JelteF @ilslv to sum up:

All agreed that #[from(forward)] should imply ignoring other non-marked fields.
All agreed on the #[from(types((i8, i8), (i16, i16)))] syntax for tuple structs, since it makes things less magic and allows expressing more patterns.
The last questions are about #[from(types(...)] design:
1. Should #[from(types(...)] imply ignoring other non-marked fields the same way as #[from(forward)]?
2. Should it implicitly imply the underlying type with disabling allowance via ignore, or rather implicitly disable the underlying type, requiring to specify it explicitly if necessary?

JelteF · 2023-02-23T22:31:09Z

The last questions are about #[from(types(...)] design:

Should #[from(types(...)] imply ignoring other non-marked fields the same way as #[from(forward)]?

Should it implicitly imply the underlying type with disabling allowance via ignore, or rather implicitly disable the underlying type, requiring to specify it explicitly if necessary?

For 3.i, I don't have a real strong preference either way. But I'd lean towards keeping the current, behaviour simply because we haven't had complaints about it.
For 3.ii, I feel quite strongly that implicitly disabling the derive for the actual types seems like it's pretty much never what you want. I don't think a solution for this edge case needs to be part of this PR. If people create an issue with a usecase for it, then we can figure out some nice API then. (I agree that my ignore API is not really nice) But I think spending time on such an edge case usecase is not worth the effort at the moment.

tyranron · 2023-02-28T14:54:16Z

@JelteF @ilslv

3.i. Should #[from(types(...)] imply ignoring other non-marked fields the same way as #[from(forward)]?

I would prefer to not have such implying. It definitely makes sense for forward due to the blanket impl, but for types it could be quite annoying during refactorings that when you add more types, some other impls disappear, while there is no real impls conflict for that to happen.

3.ii. Should it implicitly imply the underlying type with disabling allowance via ignore, or rather implicitly disable the underlying type, requiring to specify it explicitly if necessary?

I'd like to go with "implicitly disable the underlying type, requiring to specify it explicitly if necessary". Seems like a quite intuitive and unambiguous thing: either we default to the underlying type, or enumerate all the needed types explicitly.

It also frees our hands to go forward and make more simplifications like this (removing the types/owned/ref/ref_mut completely):

#[derive(From)]
enum Value {
    #[from(i8, &i32)]
    Int(i32),
    #[from(&str, &mut str, &String, String)]
    String(String),
}

#[derive(Into)]
#[into(i64, &i64, &mut i64)]
struct MyInt64(i64);

ilslv · 2023-03-03T11:08:21Z

@tyranron @JelteF

I would prefer to not have such implying. It definitely makes sense for forward due to the blanket impl, but for types it could be quite annoying during refactorings that when you add more types, some other impls disappear, while there is no real impls conflict for that to happen.

I'd like to go with "implicitly disable the underlying type, requiring to specify it explicitly if necessary". Seems like a quite intuitive and unambiguous thing: either we default to the underlying type, or enumerate all the needed types explicitly.

I agree with both points.

It also frees our hands to go forward and make more simplifications like this (removing the types/owned/ref/ref_mut completely):

From actually doesn't have any types/owned/ref/ref_mut shenanigans, only Into does, but agree, those attributes are annoying to me.

tyranron · 2023-03-03T12:55:57Z

@JelteF @ilslv

From actually doesn't have any types/owned/ref/ref_mut shenanigans, only Into does, but agree, those attributes are annoying to me.

Yes, I've thought about it a bit more, and have concluded that we cannot actually throw owned/ref/ref_mut out, because they control Self type representation, and I see no way to auto-deduce it unambiguously.

However, nothing bad happens if we strip types out completely.

ilslv · 2023-03-10T14:52:28Z

This PR is ready for review

JelteF · 2023-03-18T19:21:18Z

CHANGELOG.md

+- The `From` derive now uses `#[from(<types>)]` instead of `#[from(types(<types>))]`
+ and ignores field type itself.


Hmm, I didn't realize this changed until I read this changelog entry. Sorry for not noticing earlier, because it's indeed in the PR description. But I don't think we should remove the types wrapper itself. This will make it hard for us to add new sub-attributes to the from attribute in a backwards compatible manner in the future, because although unlikely they might conflict with type names.

To be clear this comment was about removing the types wrapper. "Ignoring the field type itself" I'm also not a huge fan of, but I think that's fine.

JelteF

Overall I like this change a lot! Thanks for writing all the detailed tests too! Left one comment on the final syntax.

JelteF · 2023-04-23T12:17:53Z

@ilslv small ping. I think this can be merged after the types(...) wrapper is added back (and the merge conflict in the changelog is resolved).

JelteF · 2023-06-12T16:53:01Z

@ilslv There's a few tiny merge conflicts on this PR now that #251 is merged. Could you resolve those?

tyranron · 2023-06-13T17:17:58Z

@JelteF I'll pick @ilslv's work up and finish it in next few days, since @ilslv is having vacations this week 🏝

But before doing so, I'd like to argue a little bit more about types() restoring. I want to make some points regarding the current solution which seem to be not so obvious. Don't have time to write them up today, unfortunately, but will definitely do tomorrow.

JelteF · 2023-07-01T12:29:49Z

@tyranron I'm definitely interested in this:

But before doing so, I'd like to argue a little bit more about types() restoring. I want to make some points regarding the current solution which seem to be not so obvious. Don't have time to write them up today, unfortunately, but will definitely do tomorrow.

(I guess you were busy with other things, which is totally fine, but just a kind reminder)

tyranron · 2023-07-03T16:48:39Z

@JelteF yes, sorry for such delaying, and thank you for the patience!

Now, to the topic...

As you've mentioned before:

But I don't think we should remove the types wrapper itself. This will make it hard for us to add new sub-attributes to the from attribute in a backwards compatible manner in the future, because although unlikely they might conflict with type names.

Yes, since Rust types are PascalCased (modulo primitives) and attributes are snake_cased, we very-very-very unlikely will ever have those conflicts.

But more to that (my main concern):
Since we've changed semantics to "implicitly disable the underlying type, requiring to specify it explicitly if necessary" when the from attribute with types is specified, we leave no space, in practice, for different sub-attributes to collide. They are now mutually exclusive: either skip, or forward, or types, but never together.
With this in mind, it just feels quite odd and redundant for the library user to write everywhere #[from(types(...))] instead of just #[from(...)]: why should I always write the types( part if there is nothing to disambiguate with?

#[derive(From)]
enum Value {
    #[from(i8, i32)]
    Int(i32),
    #[from(f32, f64)]
    Float(f64),
    #[from(MyString, String)]
    String(String),
    #[from(skip)]
    Ignored(String),
}

#[derive(From)]
enum Value {
    #[from(types(i8, i32))]
    Int(i32),
    #[from(types(f32, f64))]
    Float(f64),
    #[from(types(MyString, String))]
    String(String),
    #[from(skip)]
    Ignored(String),
}

JelteF · 2023-07-03T20:59:50Z

Yes, since Rust types are PascalCased (modulo primitives) and attributes are snake_cased, we very-very-very unlikely will ever have those conflicts.

But more to that (my main concern): Since we've changed semantics to "implicitly disable the underlying type, requiring to specify it explicitly if necessary" when the from attribute with types is specified, we leave no space, in practice, for different sub-attributes to collide. They are now mutually exclusive: either skip, or forward, or types, but never together. With this in mind, it just feels quite odd and redundant for the library user to write everywhere #[from(types(...))] instead of just #[from(...)]: why should I always write the types( part if there is nothing to disambiguate with?

Fair enough. Let's do it it without the types wrapper.

JelteF

If you fix the mere conflicts I think we can merge this.

# Conflicts: # CHANGELOG.md # impl/src/fmt/mod.rs

# Conflicts: # tests/display.rs # tests/from.rs

## Synopsis This PR is a part of replacing all attributes having `syn::Meta` syntax to custom parsing, similarly to #241, #248. Paves the way for #285, and possibly other enhancements such as an opt-in #123. ## Solution Implement custom attribute parsing without `utils::State`. Co-authored-by: Kai Ren <tyranron@gmail.com>

ilslv and others added 16 commits February 9, 2023 15:21

WIP

518c1b1

Add tests

99cb368

Add tests

a062ce1

Docs and clippy

7f24761

Fix unit tests

845558e

Docs

6211b1a

Port compile_fail tests from Display derive macro

e5ef86e

More compile_fail tests

8c8f9ca

CHANGELOG

6e82741

Reimplement core::fmt::DebugTuple to add support for `finish_non_ex…

7080be8

…haustive`

Docs fmt corrections

20a0cb0

Minor correction

8c58553

Minor correction

5a8cf3a

Minor corrections [skip ci]

558c2f0

Restructure the modules and guard Debug under a separate debug fe…

819454e

…ature

Implement syn::Type parsing

aa2d332

Simplify syn::Type parsing with assumptions

20d107e

Attributes parsing

ea8e31c

JelteF added this to the 1.0.0 milestone Feb 27, 2023

ilslv changed the title ~~Redesign From derive macro~~ Redesign From derive macro attributes Mar 10, 2023

ilslv changed the title ~~Redesign From derive macro attributes~~ Redesign From derive macro Mar 10, 2023

Corrections and changelog

1d74fdf

ilslv marked this pull request as ready for review March 10, 2023 14:25

Corrections

249a92c

JelteF reviewed Mar 18, 2023

View reviewed changes

JelteF mentioned this pull request Mar 18, 2023

Redesign Into derive macro #248

Merged

3 tasks

zohnannor mentioned this pull request Jun 21, 2023

Support multiple attribute occurrences #265

Closed

3 tasks

JelteF previously approved these changes Jul 3, 2023

View reviewed changes

JelteF assigned tyranron and ilslv Jul 4, 2023

ilslv added 2 commits July 5, 2023 13:50

Merge branch 'master' into from-attribute

4006a78

# Conflicts: # CHANGELOG.md # impl/src/fmt/mod.rs

Corrections

ebf41ed

ilslv dismissed JelteF’s stale review via ebf41ed July 5, 2023 12:33

ilslv requested a review from JelteF July 5, 2023 12:39

ilslv and others added 3 commits July 5, 2023 16:13

Merge branch 'master' into from-attribute

ea0cd04

# Conflicts: # tests/display.rs # tests/from.rs

Merge branch 'master' into from-attribute

a78f124

Minor corrections

ec48a37

tyranron approved these changes Jul 5, 2023

View reviewed changes

tyranron merged commit 60ed1e7 into JelteF:master Jul 5, 2023
16 checks passed

MegaBluejay mentioned this pull request Aug 9, 2023

Refactor AsRef and AsMut derives #286

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign `From` derive macro #241

Redesign `From` derive macro #241

ilslv commented Feb 17, 2023 •

edited

Loading

ilslv commented Feb 17, 2023

tyranron commented Feb 17, 2023

ilslv commented Feb 17, 2023

tyranron commented Feb 17, 2023

JelteF commented Feb 20, 2023 •

edited

Loading

ilslv commented Feb 21, 2023

tyranron commented Feb 21, 2023

JelteF commented Feb 23, 2023 •

edited

Loading

tyranron commented Feb 28, 2023

ilslv commented Mar 3, 2023

tyranron commented Mar 3, 2023

ilslv commented Mar 10, 2023

JelteF Mar 18, 2023

JelteF Mar 18, 2023

JelteF left a comment

JelteF commented Apr 23, 2023

JelteF commented Jun 12, 2023

tyranron commented Jun 13, 2023

JelteF commented Jul 1, 2023

tyranron commented Jul 3, 2023

JelteF commented Jul 3, 2023

JelteF left a comment

		- The `From` derive now uses `#[from(<types>)]` instead of `#[from(types(<types>))]`
		and ignores field type itself.

Redesign From derive macro #241

Redesign From derive macro #241

Conversation

ilslv commented Feb 17, 2023 • edited Loading

Synopsis

Solution

Checklist

ilslv commented Feb 17, 2023

tyranron commented Feb 17, 2023

ilslv commented Feb 17, 2023

tyranron commented Feb 17, 2023

JelteF commented Feb 20, 2023 • edited Loading

ilslv commented Feb 21, 2023

tyranron commented Feb 21, 2023

JelteF commented Feb 23, 2023 • edited Loading

tyranron commented Feb 28, 2023

ilslv commented Mar 3, 2023

tyranron commented Mar 3, 2023

ilslv commented Mar 10, 2023

JelteF Mar 18, 2023

Choose a reason for hiding this comment

JelteF Mar 18, 2023

Choose a reason for hiding this comment

JelteF left a comment

Choose a reason for hiding this comment

JelteF commented Apr 23, 2023

JelteF commented Jun 12, 2023

tyranron commented Jun 13, 2023

JelteF commented Jul 1, 2023

tyranron commented Jul 3, 2023

JelteF commented Jul 3, 2023

JelteF left a comment

Choose a reason for hiding this comment

Redesign `From` derive macro #241

Redesign `From` derive macro #241

ilslv commented Feb 17, 2023 •

edited

Loading

JelteF commented Feb 20, 2023 •

edited

Loading

JelteF commented Feb 23, 2023 •

edited

Loading