Fix the problem with deriving Debug with other attributes #15
Conversation
|
Fixed
…On Mon, Oct 23, 2023 at 8:55 PM Alyssa Haroldsen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In derive/src/lib.rs
<#15 (comment)>:
> @@ -135,8 +135,8 @@ fn open_enum_impl(
// To make `match` seamless, derive(PartialEq, Eq) if they aren't already.
let mut our_derives = HashSet::new();
- our_derives.insert("PartialEq");
- our_derives.insert("Eq");
+ our_derives.insert("PartialEq".to_string());
Prefer .to_owned() over .to_string()
—
Reply to this email directly, view it on GitHub
<#15 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5NEOFKU6S2JEOEQ5A7S7DYA2D65AVCNFSM6AAAAAA6IR5F2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOJSHA3TEMZYGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Thanks,
Jinank Jain
|
derive/src/lib.rs
Outdated
| } else if derive.is_ident("Eq") { | ||
| our_derives.remove("Eq"); | ||
| } | ||
| let ident_str = derive.get_ident().unwrap().to_string(); |
There was a problem hiding this comment.
This won't work for derives that aren't a single identifier. Example: derive(zerocopy::AsBytes).
There was a problem hiding this comment.
Can you add zerocopy = { version = "0.7.11", features = ["derive"] } to dev-dependencies and ensure that derive(zerocopy::AsBytes, zerocopy::FromBytes) works in both the inner open enum and the outer struct? Without a use, so a full path derive can be tested.
The fact that derive(::Debug) or derive(core::fmt::Debug) does not work is a known issue, but it should work for cases like this. The order in which the derive is placed does matter.
There was a problem hiding this comment.
I have added a test for core::fmt::Debug. Is that good enough or should I add zerocopy as well?
There was a problem hiding this comment.
I'd also like a test with another derive that should not be intercepted
should I add zerocopy as well
yes, please
jinankjain
force-pushed
the
main
branch
2 times, most recently
from
7b84ef1 to
5ce12d1
Compare
... in case we are emitting custom Debug impl trait implementation. Otherwise it would conflict with the default Debug implementation. Signed-off-by: Jinank Jain <[email protected]>
There are two new things added: 1. OpenEnum which mimics the failing build scenario 2. Struct which embeds that enum inside it and also derive Debug on top of it. By doing so we verify first of all the code compiles with multiple derive attributes and secondly the embedded debug derives works as expected. Signed-off-by: Jinank Jain <[email protected]>
|
@kupiakos Can you please take a look now? I have tried to address your concern around single identifier. |
derive/src/lib.rs
Outdated
| for nested_meta in nested.iter() { | ||
| if let NestedMeta::Meta(inner_meta) = nested_meta { | ||
| let inner_meta_str = format!("{}", quote::quote!(#inner_meta)); | ||
| if inner_meta_str.contains("Debug") && !allow_alias { |
There was a problem hiding this comment.
This would break a derive some_crate::DeriveThatHasDebugInTheName.
Can you instead have it only "capture" a Debug derive if its path is exactly one of these:
Debugfmt::Debug::core::fmt::Debugcore::fmt::Debug::std::fmt::Debugstd::fmt::Debug
It's unlikely (albeit possible) that these would conflict with other derives
I could not think of way of removing the intercept without parsing the derives actually... Is there something else that you could recommend? |
It will now check against a (module, Name) pair to determine how to intercept PartialEq, Eq, and Debug. Fixes kupiakos#14.
I added a new function to check against prelude derive paths which should work in all desired cases. |
This pull request tries to fix the problem pointed out in #14 and along with it also add a test to make sure that this does not happen in future.