Fix transparency check on shared attribute formatting on enums (#377)

[tyranron]

Follows #377

Synopsis

At the moment, the following example:

#[derive(Debug, Display)]
#[display("{self:?}")]
enum Enum {
    #[display("A {_0}")]
    A(i32),
    #[display("B {}", field)]
    B { field: i32 },
    C,
}

expand incorrectly as:

#[automatically_derived]
impl derive_more::Display for Enum {
    fn fmt(&self, __derive_more_f: &mut derive_more::core::fmt::Formatter<'_>) -> derive_more::core::fmt::Result {
        match self {
            Self::A(_0) => { derive_more::core::write!(__derive_more_f, "A {_0}", _0 = *_0) }
            Self::B {
                field
            } => { derive_more::core::write!(__derive_more_f, "B {}", field, ) }
            Self::C => { __derive_more_f.write_str("C") }
        } 
    } 
}

This is because transparency check of the shared #[display(...)] formatting attribute doesn't consider the actual trait, called transparently, in correspondence with the implemented trait.

Solution

Instead of

let has_shared_attr = self
    .shared_attr
    .map_or(false, |a| a.transparent_call().is_none());

do this

let has_shared_attr = self.shared_attr.map_or(false, |a| {
    a.transparent_call()
        .map_or(true, |(_, called_trait)| &called_trait != self.trait_ident)
});

Checklist

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

[JelteF]

I feel like we should disallow this instead. i.e. if you use a display attribute on a variant without having _variant in the display attribute on the enum.

[JelteF]

Or maybe it should just use the display tag on the enum as the default. And if a variant has one set then the one from the variant overrides the one from the enum, unless there is a _variant inside the one on the enum.

[tyranron]

@JelteF I would rather fix this in a separate PR, because it's somewhat orthogonal to the issue being fixed here.

Or maybe it should just use the display tag on the enum as the default. And if a variant has one set then the one from the variant overrides the one from the enum, unless there is a _variant inside the one on the enum.

We negotiated about the top-level attribute acting as the default one back in #142. There could be some unobvious subtleties about such behavior. Need to re-read all the thread.

I propose we better forbid this for now (in a separate PR), and negotiate the exact defaulting behavior in #142 (by re-opening it) or in new issue.

[tyranron]

@JelteF ping.

[JelteF]

The behaviour that we currently have on the master branch is that the top-level attribute acts as the default when no _variant is present. This seems at least somewhat reasonable behaviour and multiple posts in #142 suggest this behaviour too, and I didn't see strong arguments against it. Changing this behaviour now to be an error, or to do something differently would be a backward compatibility issue now that we're 1.0.0.

The behaviour proposed in this PR we definitely don't want IMHO. It's super confusing that the #[display] attributes on the variants are completely ignored.

I think we should make a PR that add the tests from this PR and makes sure we don't change the current behaviour. And also we should update the documentation to note that this is what happens.

[tyranron]

@JelteF

The behaviour that we currently have on the master branch is that the top-level attribute acts as the default when no _variant is present.

It seems not to be like that. At least we have the test showing the opposite overwriting behavior. And it had been introduced in #377 by your commit before I altered the desugaring. Maybe I miss something, but "acts as the default when no _variant is present" was never a thing.

The behaviour proposed in this PR we definitely don't want IMHO. It's super confusing that the #[display] attributes on the variants are completely ignored.

This PR has nothing to do with such behavior. It doesn't alter any behavior, only makes transparency check for shared attribute formatting more clever, fixing some cases overlooked in #377.

This seems at least somewhat reasonable behaviour and multiple posts in #142 suggest this behaviour too, and I didn't see strong arguments against it.

I'm not against it. It just seems to be a theme for a separate PR.

Changing this behaviour now to be an error, or to do something differently would be a backward compatibility issue now that we're 1.0.0.

Considering that we've just released, it's unlikely that people have widely adopted it. I agree that it's super confusing one. So, yanking 1.0.0 and releasing 1.0.1 with treating this case more correctly is reasonable a good way to go here, as for me.

[JelteF]

This PR definitely does change behaviour: i.e. if you remove your code changes the new test that you added with #[display("{_0}")] returns the "acts as the default" result.

After looking closer I do realize that that seems mostly by accident though. In all other cases it seems to do the "overwrite" approach, which imho is the worst possible behaviour.

So yeah let's make this test a hard error for now and release 1.0.1 quickly: https://github.com/JelteF/derive_more/blob/v1.0.0/tests/display.rs#L1404-L1427

[tyranron]

@JelteF

This PR definitely does change behaviour: i.e. if you remove your code changes the new test that you added with #[display("{_0}")] returns the "acts as the default" result.

After looking closer I do realize that that seems mostly by accident though. In all other cases it seems to do the "overwrite" approach, which imho is the worst possible behaviour.

I still don't see it or do misunderstand it. Given the test from the PR:

#[derive(Display)]
#[display("{_0}")]
enum Enum<T> {
    #[display("A")]
    A(i32),
    #[display("B")]
    B(&'static str),
    #[display("C")]
    C(T),
}

#[test]
fn assert() {
assert_eq!(Enum::<u8>::A(1).to_string(), "1");  // "acts as the default" should return "A" here
assert_eq!(Enum::<u8>::B("abc").to_string(), "abc");  // "acts as the default" should return "B" here
assert_eq!(Enum::<u8>::C(9).to_string(), "9");  // "acts as the default" should return "C" here
}

So yeah let's make this test a hard error for now and release 1.0.1 quickly: https://github.com/JelteF/derive_more/blob/v1.0.0/tests/display.rs#L1404-L1427

👍

But as I've proposed above, let me do it as a separate PR.

[tyranron]

@JelteF considering the #405 discussion, we may introduce here a breaking behavior to set up things right.

I do still propose to merge this PR "as is", because it doesn't alter any interaction between top-level and variant-level #[display()] attributes, but only makes the transparency check more clever to not consider cases it shouldn't to.

[JelteF]

I still don't see it or do misunderstand it. Given the test from the PR:

I feel like you made a mistake in testing, because the example you shared very clearly fails on 1.0.0 (because 1.0.0 has the "acts as the default" behaviour in that specific case). Try running this cargo-script and see how it errors:

#!/usr/bin/env run-cargo-script
//! ```cargo
//! [dependencies]
//! derive_more = { version = "1", features = ["display"] }
//! ```
extern crate derive_more;
use derive_more::Display;

#[derive(Display)]
#[display("{_0}")]
enum Enum<T> {
    #[display("A")]
    A(i32),
    #[display("B")]
    B(&'static str),
    #[display("C")]
    C(T),
}

fn main() {
    assert_eq!(Enum::<u8>::A(1).to_string(), "1"); // "acts as the default" should return "A" here
    assert_eq!(Enum::<u8>::B("abc").to_string(), "abc"); // "acts as the default" should return "B" here
    assert_eq!(Enum::<u8>::C(9).to_string(), "9"); // "acts as the default" should return "C" here
}

To make it pass you need to change it to this:

#!/usr/bin/env run-cargo-script
//! ```cargo
//! [dependencies]
//! derive_more = { version = "1", features = ["display"] }
//! ```
extern crate derive_more;
use derive_more::Display;

#[derive(Display)]
#[display("{_0}")]
enum Enum<T> {
    #[display("A")]
    A(i32),
    #[display("B")]
    B(&'static str),
    #[display("C")]
    C(T),
}

fn main() {
    assert_eq!(Enum::<u8>::A(1).to_string(), "A"); // "acts as the default" should return "A" here
    assert_eq!(Enum::<u8>::B("abc").to_string(), "B"); // "acts as the default" should return "B" here
    assert_eq!(Enum::<u8>::C(9).to_string(), "C"); // "acts as the default" should return "C" here
}