Derive From for Enum has strange defaults or might be a regression

[Robbepop]

When using derive_more v0.15 I had the following on the enum with its derive_more::From:

/// Errors that can be encountered while accessing the contract's environment.
#[derive(Debug, Clone, From)]
pub enum Error {
    Call(CallError),
    Create(CreateError),
    Codec(scale::Error),
    InvalidStorageKey,
    InvalidStorageRead,
    InvalidContractCall,
    InvalidContractCallReturn,
    InvalidContractInstantiation,
    InvalidContractInstantiationReturn,
    InvalidRandomSeed,
}

Now with derive_more v0.99 I had to use plenty #[from(ignore)] attribute markers. To me this feels like a serious regression since those enums are kind of common. The problem is that the new derive_more::From implementation tries to generate a From<()> for each of the C-like variants.

/// Errors that can be encountered while accessing the contract's environment.
#[derive(Debug, Clone, From)]
pub enum Error {
    Call(CallError),
    Create(CreateError),
    Codec(scale::Error),
    #[from(ignore)] InvalidStorageKey,
    #[from(ignore)] InvalidStorageRead,
    #[from(ignore)] InvalidContractCall,
    #[from(ignore)] InvalidContractCallReturn,
    #[from(ignore)] InvalidContractInstantiation,
    #[from(ignore)] InvalidContractInstantiationReturn,
    #[from(ignore)] InvalidRandomSeed,
}

Question: Is this intended behaviour? If yes: Why? If no: Candidate for fix? :)

[JelteF]

I agree that this is indeed undesirable. This happened, because I removed the code that ignored both variants when the type was the same. The reason for this was that it would be confusing if you put #[from] on two variants and if they had the same type it would silently not add those implementations (instead of giving an error). I think it makes sense to fix this case though, where nothing is explicitly enabled or disabled.

For now a slightly shorter solution would be to enable the implementations for variants that you want explicitly, instead of ignoring the ones you don't. You can do this by putting a bare #[from] attribute on the top three variants in your example.

[ozkriff]

The new behavior doesn't work well for a few of my enums too:

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, derive_more::From)]
pub enum Ability {
    #[from(ignore)]
    Knockback,
    #[from(ignore)]
    Club,
    Jump(Jump),
    #[from(ignore)]
    Poison,
    #[from(ignore)]
    ExplodePush,
    #[from(ignore)]
    ExplodeDamage,
    #[from(ignore)]
    ExplodeFire,
    #[from(ignore)]
    ExplodePoison,
    Bomb(Bomb),
    BombPush(BombPush),
    BombFire(BombFire),
    BombPoison(BombPoison),
    BombDemonic(BombDemonic),
    #[from(ignore)]
    Summon,
    #[from(ignore)]
    Vanish,
    #[from(ignore)]
    Dash,
    Rage(Rage),
    Heal(Heal),
    #[from(ignore)]
    Bloodlust,
}

upd: it looks slightly better with bare #[from]s, but I'd still prefer to not add any explicit attributes for this case.

[JelteF]

I pushed a fix to master, can you check if that indeed fixes the issue?

[Robbepop]

It worked for me from what I can tell. So the C-like variants no longer seem to produce a From<()> impl which is as expected.

[JelteF]

Well, they still do. Just not when you have multiple C-like variants in the same enum.

[Robbepop]

I am not sure if I would create them at all. To me this is not what I would expect them to do and besides that typing Foo::Bar is not worse than From::<()> to me. So the gains are not worth the confusion that might result from these From<()> impls in my honest opinion. Is there a design rational somewhere about this design?

[JelteF]

I think that makes sense. I think they were simply originally there for consistency. I think I'll release this hotfix tonight and rethink generating them at all by default for 1.0.

[JelteF]

What do you think should be doen for non C-like variants that have the same types?
e.g.

#[derive(From)]
enum Ints {
    Int1(i32)
    Int2(i32)
    Uint(u32)
}

Should it throw an error that Int1 and Int2 both generate the same From implementation, or should it silently generate none of them? I'm leaning towards the first.

[Robbepop]

Error is always better than implied behaviour in such cases imo.

[JelteF]

Just published 0.99.1 that fixes this issue for variants without any members so, Int, Int() and Int{}. If there's multiple of these in an enum it will generate no from implementation for (). Not deriving it at all for these variants is tracked in #106

[Robbepop]

Thanks a lot! :)