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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
99a906d
Consider trait identification in shared attribute transparency
tyranron Aug 9, 2024
1a9aacc
Fix one more bug
tyranron Aug 9, 2024
30ccf11
Mention in CHANGELOG
tyranron Aug 9, 2024
e5d8804
Merge branch 'master' into fix-shared-transparency-check
tyranron Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
([#399](https://github.com/JelteF/derive_more/pull/399))
- `unreachable_code` warnings on generated code when `!` (never type) is used.
([#404](https://github.com/JelteF/derive_more/pull/404))
- A top level `#[display("...")]` attribute on an enum being incorrectly
treated as transparent.
([#395](https://github.com/JelteF/derive_more/pull/395))


## 1.0.0 - 2024-08-07
Expand Down
18 changes: 10 additions & 8 deletions impl/src/fmt/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,18 @@ impl<'a> Expansion<'a> {
fn generate_body(&self) -> syn::Result<TokenStream> {
let mut body = TokenStream::new();

// If `shared_attr` is a transparent call, then we consider it being absent.
let has_shared_attr = self
let shared_attr_contains_variant = self
.shared_attr
.map_or(false, |a| a.transparent_call().is_none());
.map_or(true, |a| a.contains_arg("_variant"));

if !has_shared_attr
|| self
.shared_attr
.map_or(true, |a| a.contains_arg("_variant"))
{
// If `shared_attr` is a transparent call to `_variant`, then we consider it being absent.
let has_shared_attr = self.shared_attr.map_or(false, |attr| {
attr.transparent_call().map_or(true, |(_, called_trait)| {
&called_trait != self.trait_ident || !shared_attr_contains_variant
})
});

if !has_shared_attr || shared_attr_contains_variant {
body = match &self.attrs.fmt {
Some(fmt) => {
if has_shared_attr {
Expand Down
45 changes: 45 additions & 0 deletions tests/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1499,6 +1499,28 @@ mod enums {
}
}

mod only_field {
use super::*;

#[derive(Display)]
#[display("{_0}")]
enum Enum<T> {
#[display("A")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@tyranron tyranron Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF ping.

Copy link
Owner

@JelteF JelteF Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner

@JelteF JelteF Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

A(i32),
#[display("B")]
B(&'static str),
#[display("C")]
C(T),
}

#[test]
fn assert() {
assert_eq!(Enum::<u8>::A(1).to_string(), "1");
assert_eq!(Enum::<u8>::B("abc").to_string(), "abc");
assert_eq!(Enum::<u8>::C(9).to_string(), "9");
}
}

mod use_field_and_variant {
use super::*;

Expand All @@ -1521,6 +1543,29 @@ mod enums {
}
}

mod as_debug {
use super::*;

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

#[test]
fn assert() {
assert_eq!(Enum::A(1).to_string(), "A(1)");
assert_eq!(Enum::B { field: 2 }.to_string(), "B { field: 2 }");
assert_eq!(Enum::C.to_string(), "C");
}
}

mod pointer {
use super::*;

Expand Down