-
Notifications
You must be signed in to change notification settings - Fork 2
[FEATURE]: Generics, lifetimes, where clauses and internal field variant formatting #5
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.
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
base: main
Are you sure you want to change the base?
Conversation
Simplified the code a bit. It was previously checking that the user actually referenced each field before emitting variables for them, but this adds unnecessary complexity. We can just emit all field variables and let the compiler decide if they're unused. |
Btw, my specific use case was this: #[derive(Debug, Clone, PartialEq, Eq, EnumDisplay)]
#[enum_display(case = "Lower")]
pub enum ParserErrorKind<I> {
NomError(ErrorKind),
#[variant_display(format = "{variant}: {0}")]
Keyword(Keyword),
Ident(Ident<I>),
...
} Where I wanted to provide a general impl of Display for most variants, but be specific for required Keywords. |
Thanks for submitting these PRs! Both the generics impl and this. I'll have time next weekend to actually look at this and critique. From the 1000 mile overview it looks like a solid addition to this library. It seems like the build pipeline is currently failing and I'll investigate that more as well. Is it possibly due to different version of Thanks again! |
Cheers! Glad you like it on the surface. The generics side of things was actually really simple (almost like the code equivalent of copy paste - you take the generics from I've had a quick look at the build issue and it's very weird. Does indeed look like maybe a version mismatch. Anyway, yeah, like you, I'll be able to spend more time on this next weekend. |
- Bump version number before publish - `gen` is a reserved keyword: https://doc.rust-lang.org/edition-guide/rust-2024/gen-keyword.html - Rename all functions named `gen` to `generate` - Allow for local testing via workspaces, however when publishing the local dep needs to be replaced with the published version - This allowed for the actual proc_macro code changes to propegate into the local build.
…the published version
@jngls I created a branch on top of your changes you pushed here and have got the CI/CD pipeline building and fixed a few other things here: #6. You can see the fixes that were needed. I'm unfortunately not a git master, so I think some of the history got borked with my changes being pushed upstream to my repo from your fork/branch. Feel free to add my commits to your PR and it works great if you want to properly preserve your impact on the codebase. Thanks again for the great code and suggestions here. I hope my fixes are good enough for you! |
Almost forgot, we should update the docs if there's anything missing and the |
- Rename `variant_display` to `display` - Support named argument `format=` and first argument for format.
Yo sorry, work was crazy lately. I finally have a day to myself, so I'll look at this now. Need to catch up on what you've done. |
Yeah, your changes look great to me. I was never happy with the long With the build issue, good fix. I remember now I had to locally change the dependency to I don't see anything borked in the git history. Just a bunch of autoformat, which is fine. Looks good to me. I'll merge your changes into my branch, test locally, and then start updating docs. |
Right. I've merged your changes into this PR and added a little doc example. I want to consider this a little before you merge and publish a version:
I'm not 100% sure what the right course of action is so need to read up. Since the macro might inject a |
Ok. There is no So, that means the formatting code path is only supported on
|
I've been unable to use:
This is because it mandates that the However, I have a plan. The It feels slightly hacky and I don't like the idea of exporting something the user isn't meant to see. But I'm not sure what else to do right now. Lemme know if you have any ideas. I'll push the code shortly. Maybe this can serve as a first iteration and if we find a better method we can switch to that. |
Actually, And my I need to head home. Will pick this up again when I can. |
I never had any intention of supporting |
I was able to "vibe-code" (lol) a simple diff --git a/enum-display-macro/src/lib.rs b/enum-display-macro/src/lib.rs
index 6483879..54628cb 100644
--- a/enum-display-macro/src/lib.rs
+++ b/enum-display-macro/src/lib.rs
@@ -150,9 +150,14 @@ impl NamedVariantIR {
let fields = self.fields;
match (any_has_format, attrs.format) {
(true, Some(fmt)) => {
- quote! { #ident { #(#fields),* } => { let variant = #ident_transformed; format!(#fmt) } }
+ quote! { #ident { #(#fields),* } => {
+ let variant = #ident_transformed;
+ ::core::write!(f, #fmt)
+ } }
+ }
+ (true, None) => {
+ quote! { #ident { .. } => ::core::fmt::Formatter::write_str(f, #ident_transformed), }
}
- (true, None) => quote! { #ident { .. } => String::from(#ident_transformed), },
(false, None) => quote! { #ident { .. } => #ident_transformed, },
_ => unreachable!(
"`any_has_format` should never be false when a variant has format string"
@@ -187,9 +192,14 @@ impl UnnamedVariantIR {
let fields = self.fields;
match (any_has_format, attrs.format) {
(true, Some(fmt)) => {
- quote! { #ident(#(#fields),*) => { let variant = #ident_transformed; format!(#fmt) } }
+ quote! { #ident(#(#fields),*) => {
+ let variant = #ident_transformed;
+ ::core::write!(f, #fmt)
+ } }
+ }
+ (true, None) => {
+ quote! { #ident(..) => ::core::fmt::Formatter::write_str(f, #ident_transformed), }
}
- (true, None) => quote! { #ident(..) => String::from(#ident_transformed), },
(false, None) => quote! { #ident(..) => #ident_transformed, },
_ => unreachable!(
"`any_has_format` should never be false when a variant has format string"
@@ -216,9 +226,14 @@ impl UnitVariantIR {
} = self.info;
match (any_has_format, attrs.format) {
(true, Some(fmt)) => {
- quote! { #ident => { let variant = #ident_transformed; format!(#fmt) } }
+ quote! { #ident => {
+ let variant = #ident_transformed;
+ ::core::write!(f, #fmt)
+ } }
+ }
+ (true, None) => {
+ quote! { #ident => ::core::fmt::Formatter::write_str(f, #ident_transformed), }
}
- (true, None) => quote! { #ident => String::from(#ident_transformed), },
(false, None) => quote! { #ident => #ident_transformed, },
_ => unreachable!(
"`any_has_format` should never be false when a variant has format string"
@@ -299,36 +314,44 @@ pub fn derive(input: TokenStream) -> TokenStream {
.map(|variant| VariantIR::from_variant(variant, &enum_attrs))
.collect();
- // If any variants have a format string, the output of all match arms must be String instead of &str
- // This is because we can't return a reference to the temporary output of format!()
+ // If any variants have a format string, we need to handle formatting differently
let any_has_format = intermediate_variants.iter().any(|v| v.has_format());
- let post_fix = if any_has_format {
- quote! { .as_str() }
- } else {
- quote! {}
- };
// Build the match arms
let variants = intermediate_variants
.into_iter()
.map(|v| v.generate(any_has_format));
- // #[allow(unused_qualifications)] is needed
- // due to https://github.com/SeedyROM/enum-display/issues/1
- // Possibly related to https://github.com/rust-lang/rust/issues/96698
- let output = quote! {
- #[automatically_derived]
- #[allow(unused_qualifications)]
- impl #impl_generics ::core::fmt::Display for #ident #ty_generics #where_clause {
- fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
- ::core::fmt::Formatter::write_str(
- f,
+ let output = if any_has_format {
+ // When format strings are present, we write directly to the formatter
+ quote! {
+ #[automatically_derived]
+ #[allow(unused_qualifications)]
+ impl #impl_generics ::core::fmt::Display for #ident #ty_generics #where_clause {
+ fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
match self {
#(Self::#variants)*
- }#post_fix
- )
+ }
+ }
+ }
+ }
+ } else {
+ // When no format strings, we can return &str directly
+ quote! {
+ #[automatically_derived]
+ #[allow(unused_qualifications)]
+ impl #impl_generics ::core::fmt::Display for #ident #ty_generics #where_clause {
+ fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
+ ::core::fmt::Formatter::write_str(
+ f,
+ match self {
+ #(Self::#variants)*
+ }
+ )
+ }
}
}
};
+
output.into()
} There's a few other configuration changes to the main lib tests: diff --git a/src/lib.rs b/src/lib.rs
index 62ea829..11379d3 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,5 +1,8 @@
//!
-//! enum-display is a crate for implementing [`std::fmt::Display`] on enum variants with macros.
+//! enum-display is a crate for implementing [`core::fmt::Display`] on enum variants with macros.
+//!
+//! This crate supports both `std` and `no_std` environments. In `no_std` mode, it works
+//! without allocation by writing directly to the formatter.
//!
//! # Simple Example
//!
@@ -31,8 +34,28 @@
//! HelloGreeting { name: String },
//! }
//!
+//! # #[cfg(feature = "std")]
//! assert_eq!(Message::HelloGreeting { name: "Alice".to_string() }.to_string(), "hello-greeting");
+//! ```
+//!
+//! # No-std Usage
+//!
+//! This crate works in `no_std` environments:
+//!
+//! ```rust
+//! # #![cfg_attr(not(feature = "std"), no_std)]
+//! use enum_display::EnumDisplay;
//!
+//! #[derive(EnumDisplay)]
+//! enum Status {
+//! Ready,
+//!
+//! #[display("Error: {code}")]
+//! Error { code: u32 },
+//! }
+//! ```
+
+#![cfg_attr(not(feature = "std"), no_std)]
pub use enum_display_macro::*;
@@ -40,6 +63,15 @@ pub use enum_display_macro::*;
mod tests {
use super::*;
+ #[cfg(feature = "std")]
+ use std::string::{String, ToString};
+
+ #[cfg(not(feature = "std"))]
+ extern crate alloc;
+
+ #[cfg(not(feature = "std"))]
+ use alloc::string::{String, ToString};
+
#[allow(dead_code)]
#[derive(EnumDisplay)]
enum TestEnum {
@@ -101,7 +133,7 @@ mod tests {
#[derive(EnumDisplay)]
enum TestEnumWithLifetimeAndGenerics<'a, T: Clone>
where
- T: std::fmt::Display,
+ T: core::fmt::Display,
{
Name,
Address { Same with added features in diff --git a/Cargo.toml b/Cargo.toml
index 1bf8a55..34c8250 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -14,3 +14,18 @@ repository = "https://github.com/SeedyROM/enum-display"
[dependencies]
enum-display-macro = { version = "0.1.5", path = "./enum-display-macro" }
+
+[features]
+default = ["std"]
+std = []
+
+[dev-dependencies]
+static_assertions = "1.1"
+
+[[test]]
+name = "no_std_integration"
+required-features = []
+
+[package.metadata.docs.rs]
+all-features = true
+rustdoc-args = ["--cfg", "docsrs"] Integration test:
I think the most important changes are in the initial change to the proc macro. It also solves alot of the changes you made in your experimental branch I think??? I am dumb on this stuff but the tests are passing locally for me. We'll need more tests for embedded targets etc if we really want to verify. I think the main difference between out setups is the layout of the proc macro change. The config looks very close? |
This is a considerable update of the proc macro which supports optional custom format strings per variant with full access to variant fields via the
#[variant_display(format = "xyz")]
attribute.I recommend reading the code in isolation rather than as a diff.
Summary
EnumAttrs
which handles parsing of the mainenum_display
attributeVariantAttrs
which handles parsing of the newvariant_display
field attributevariant_display
field attribute allows the user to customise each variantformat = "xyz"
argument defines a custom display format (see below for Format Behaviour) - if omitted, the original crate behaviour is usedVariantInfo
holds properties common to all variant typesNamedVariantIR
,UnnamedVariantIR
, andUnitVariantIR
are the intermediate representations of variant typesVariantIR
wraps all of thosesyn
tokens is moved to thefrom_*()
methods ofEnumAttrs
,VariantAttrs
, andVariantIR
VariantIR::gen()
methodFormat Behaviour
Format strings behave almost identically to using
format!()
in normal rust code so it should be very familiar to users. Besides a bit of translation of unnamed fields, the string specified by the user is the string that ends up as the parameter toformat!()
in each match arm.The following considerations apply:
{variant}
placeholderVariant { my_field: u32 }
, you use the{my_field}
placeholderVariant(u32)
, you use the{0}
placeholder (to access the first tuple element){my_field:?}
displaysmy_field
using itsDebug
implementation,{0:5}
displays the first unnamed field with a width of 5 - however - format spec options which depend on extra arguments passed intoformat!()
are not supportedAdditional Notes
format!()
- perhaps instead, we should extern alloc and hide it behind a feature flag?main
- let me know if you want them separated. Otherwise merging Implement generics, lifetimes, bounds and where clause support #4 first or discarding and just merging this would also work.Full examples