From 2412bec28d50bb809a9de737bdb385aa0d861fd3 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 16 Jun 2021 03:07:27 -0700 Subject: [PATCH] avoid requiring struct/enum definitions to constrain any type params - add doctest and FAQ - clarify that if fields impl Debug, they don't need to impl Display - make pedantic spelling change - add note on how destructuring is used for format strings - clarify the mechanism of adding Display only when otherwise unconstrained and add TODO --- src/attr.rs | 2 +- src/expand.rs | 274 +++++++++++++++++++++++++++++++++++++++++++++++++- src/lib.rs | 82 +++++++++++++-- 3 files changed, 345 insertions(+), 13 deletions(-) diff --git a/src/attr.rs b/src/attr.rs index e145935..23f1ef6 100644 --- a/src/attr.rs +++ b/src/attr.rs @@ -89,7 +89,7 @@ impl AttrsHelper { _ => unimplemented!(), }; - // Make an attempt and cleaning up multiline doc comments + // Make an attempt at cleaning up multiline doc comments. let doc_str = lit .value() .lines() diff --git a/src/expand.rs b/src/expand.rs index 02e683b..6b3cffc 100644 --- a/src/expand.rs +++ b/src/expand.rs @@ -1,7 +1,15 @@ use super::attr::AttrsHelper; -use proc_macro2::TokenStream; +use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote}; -use syn::{Data, DataEnum, DataStruct, DeriveInput, Error, Fields, Result}; +use syn::{ + punctuated::Punctuated, + token::{Add, Colon, Colon2, Comma, Where}, + Data, DataEnum, DataStruct, DeriveInput, Error, Fields, Generics, Ident, Path, PathArguments, + PathSegment, PredicateType, Result, TraitBound, TraitBoundModifier, Type, TypeParam, + TypeParamBound, TypePath, WhereClause, WherePredicate, +}; + +use std::collections::HashMap; pub(crate) fn derive(input: &DeriveInput) -> Result { let impls = match &input.data { @@ -66,6 +74,7 @@ fn specialization() -> TokenStream { fn impl_struct(input: &DeriveInput, data: &DataStruct) -> Result { let ty = &input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); + let where_clause = generate_where_clause(&input.generics, where_clause); let helper = AttrsHelper::new(&input.attrs); @@ -84,6 +93,10 @@ fn impl_struct(input: &DeriveInput, data: &DataStruct) -> Result { quote! { impl #impl_generics core::fmt::Display for #ty #ty_generics #where_clause { fn fmt(&self, formatter: &mut core::fmt::Formatter) -> core::fmt::Result { + // NB: This destructures the fields of `self` into named variables (for unnamed + // fields, it uses _0, _1, etc as above). The `#[allow(unused_variables)]` + // section means it doesn't have to parse the individual field references out of + // the docstring. #[allow(unused_variables)] let #pat = self; #display @@ -95,9 +108,266 @@ fn impl_struct(input: &DeriveInput, data: &DataStruct) -> Result { Ok(quote! { #display }) } +/// Create a `where` predicate for `ident`, without any [bound][TypeParamBound]s yet. +fn new_empty_where_type_predicate(ident: Ident) -> PredicateType { + let mut path_segments = Punctuated::::new(); + path_segments.push_value(PathSegment { + ident, + arguments: PathArguments::None, + }); + PredicateType { + lifetimes: None, + bounded_ty: Type::Path(TypePath { + qself: None, + path: Path { + leading_colon: None, + segments: path_segments, + }, + }), + colon_token: Colon { + spans: [Span::call_site()], + }, + bounds: Punctuated::::new(), + } +} + +/// Create a `where` clause that we can add [WherePredicate]s to. +fn new_empty_where_clause() -> WhereClause { + WhereClause { + where_token: Where { + span: Span::call_site(), + }, + predicates: Punctuated::::new(), + } +} + +enum UseGlobalPrefix { + LeadingColon, + #[allow(dead_code)] + NoLeadingColon, +} + +/// Create a path with segments composed of [Idents] *without* any [PathArguments]. +fn join_paths(name_segments: &[&str], use_global_prefix: UseGlobalPrefix) -> Path { + let mut segments = Punctuated::::new(); + assert!(name_segments.len() >= 1); + segments.push_value(PathSegment { + ident: Ident::new(name_segments[0], Span::call_site()), + arguments: PathArguments::None, + }); + for name in name_segments[1..].iter() { + segments.push_punct(Colon2 { + spans: [Span::call_site(), Span::mixed_site()], + }); + segments.push_value(PathSegment { + ident: Ident::new(name, Span::call_site()), + arguments: PathArguments::None, + }); + } + Path { + leading_colon: match use_global_prefix { + UseGlobalPrefix::LeadingColon => Some(Colon2 { + spans: [Span::call_site(), Span::mixed_site()], + }), + UseGlobalPrefix::NoLeadingColon => None, + }, + segments, + } +} + +/// Push `new_type_predicate` onto the end of `where_clause`. +fn append_where_clause_type_predicate( + where_clause: &mut WhereClause, + new_type_predicate: PredicateType, +) { + // Push a comma at the end if there are already any `where` predicates. + if !where_clause.predicates.is_empty() { + where_clause.predicates.push_punct(Comma { + spans: [Span::call_site()], + }); + } + where_clause + .predicates + .push_value(WherePredicate::Type(new_type_predicate)); +} + +/// Add a requirement for [core::fmt::Display] to a `where` predicate for some type. +fn add_display_constraint_to_type_predicate( + predicate_that_needs_a_display_impl: &mut PredicateType, +) { + // Create a `Path` of `::core::fmt::Display`. + let display_path = join_paths(&["core", "fmt", "Display"], UseGlobalPrefix::LeadingColon); + + let display_bound = TypeParamBound::Trait(TraitBound { + paren_token: None, + modifier: TraitBoundModifier::None, + lifetimes: None, + path: display_path, + }); + if !predicate_that_needs_a_display_impl.bounds.is_empty() { + predicate_that_needs_a_display_impl.bounds.push_punct(Add { + spans: [Span::call_site()], + }); + } + + predicate_that_needs_a_display_impl + .bounds + .push_value(display_bound); +} + +/// Map each declared generic type parameter to the set of all trait boundaries declared on it. +/// +/// These boundaries may come from the declaration site: +/// pub enum E { ... } +/// or a `where` clause after the parameter declarations: +/// pub enum E where T: MyTrait { ... } +/// This method will return the boundaries from both of those cases. +fn extract_trait_constraints_from_source( + where_clause: &WhereClause, + type_params: &[&TypeParam], +) -> HashMap> { + // Add trait bounds provided at the declaration site of type parameters for the struct/enum. + let mut param_constraint_mapping: HashMap> = type_params + .iter() + .map(|type_param| { + let trait_bounds: Vec = type_param + .bounds + .iter() + .flat_map(|bound| match bound { + TypeParamBound::Trait(trait_bound) => Some(trait_bound), + _ => None, + }) + .cloned() + .collect(); + (type_param.ident.clone(), trait_bounds) + }) + .collect(); + + // Add trait bounds from `where` clauses, which may be type parameters or types containing + // those parameters. + for predicate in where_clause.predicates.iter() { + match predicate { + WherePredicate::Type(ref pred_ty) => { + let ident = match &pred_ty.bounded_ty { + Type::Path(TypePath { path, qself: None }) => match path.get_ident() { + None => continue, + Some(ident) => ident, + }, + _ => continue, + }; + // We ignore any type constraints that aren't direct references to type + // parameters of the current enum of struct definition. No types can be + // constrained in a `where` clause unless they are a type parameter or a generic + // type instantiated with one of the type parameters, so by only allowing single + // identifiers, we can be sure that the constrained type is a type parameter + // that is contained in `param_constraint_mapping`. + if let Some((_, ref mut known_bounds)) = param_constraint_mapping + .iter_mut() + .find(|(id, _)| *id == ident) + { + for bound in pred_ty.bounds.iter() { + match bound { + TypeParamBound::Trait(ref bound) => { + known_bounds.push(bound.clone()); + } + // We only care about trait bounds here. + _ => (), + } + } + } + } + // We only care about type and not lifetime constraints here. + _ => (), + } + } + + param_constraint_mapping +} + +/// Hygienically add `where _: Display` to the set of [TypeParamBound]s for `ident`, creating such +/// a set if necessary. +fn ensure_display_in_where_clause_for_type(where_clause: &mut WhereClause, ident: Ident) { + for pred_ty in where_clause + .predicates + .iter_mut() + // Find the `where` predicate constraining the current type param, if it exists. + .flat_map(|predicate| match predicate { + WherePredicate::Type(pred_ty) => Some(pred_ty), + // We're looking through type constraints, not lifetime constraints. + _ => None, + }) + { + // Do a complicated destructuring in order to check if the type being constrained in this + // `where` clause is the type we're looking for, so we can use the mutable reference to + // `pred_ty` if so. + let matches_desired_type = match &pred_ty.bounded_ty { + Type::Path(TypePath { path, .. }) if Some(&ident) == path.get_ident() => true, + _ => false, + }; + if matches_desired_type { + add_display_constraint_to_type_predicate(pred_ty); + return; + } + } + + // If there is no `where` predicate for the current type param, we will construct one. + let mut new_type_predicate = new_empty_where_type_predicate(ident.clone()); + add_display_constraint_to_type_predicate(&mut new_type_predicate); + append_where_clause_type_predicate(where_clause, new_type_predicate); +} + +/// Given all declared type parameters, add an `impl` for a [core::fmt::Display]-like constraint if +/// the type parameter is not otherwise constrained. +fn ensure_where_clause_has_display_for_all_unconstrained_members( + where_clause: &mut WhereClause, + type_params: &[&TypeParam], +) { + let param_constraint_mapping = extract_trait_constraints_from_source(where_clause, type_params); + + for (ident, known_bounds) in param_constraint_mapping.into_iter() { + // If the type parameter has any constraints already, we don't want to touch it, to avoid + // breaking use cases where a type parameter only needs to impl `Debug`, for example. + if known_bounds.is_empty() { + ensure_display_in_where_clause_for_type(where_clause, ident); + } + } +} + +/// Generate a `where` clause that ensures all generic type parameters `impl` +/// [core::fmt::Display] unless already constrained. +/// +/// This approach allows struct/enum definitions deriving [crate::Display] to avoid hardcoding +/// a [core::fmt::Display] constraint into every type parameter. +/// +/// If the type parameter isn't already constrained, we add a `where _: Display` clause to our +/// display implementation to expect to be able to format every enum case or struct member. +/// +/// # TODO +/// This is possibly a little too coarse of a solution, since it's possible that a type +/// parameter may not be used in a way that affects whether the enum cases impl `Display`. It +/// would be nice to be able to ask rustc "does this type argument impl (e.g.) +/// `core::fmt::Display`?". +/// +/// In fact, we would preferably only require `where _: Display` or `where _: Debug` where the +/// format string actually requires it. However, while [`std::fmt` defines a formal syntax for +/// `format!()`][format syntax], it *doesn't* expose the actual logic to parse the format string, +/// which appears to live in [`rustc_parse_format`]. While we use the [`syn`] crate to parse rust +/// syntax, it also doesn't currently provide any method to introspect a `format!()` string. It +/// would be nice to contribute this upstream in [`syn`]. +/// +/// [format syntax]: std::fmt#syntax +/// [`rustc_parse_format`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_parse_format/index.html +fn generate_where_clause(generics: &Generics, where_clause: Option<&WhereClause>) -> WhereClause { + let mut where_clause = where_clause.cloned().unwrap_or_else(new_empty_where_clause); + let type_params: Vec<&TypeParam> = generics.type_params().collect(); + ensure_where_clause_has_display_for_all_unconstrained_members(&mut where_clause, &type_params); + where_clause +} + fn impl_enum(input: &DeriveInput, data: &DataEnum) -> Result { let ty = &input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); + let where_clause = generate_where_clause(&input.generics, where_clause); let helper = AttrsHelper::new(&input.attrs); diff --git a/src/lib.rs b/src/lib.rs index 285b01b..e8af005 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,6 +14,8 @@ //! //! ## Example //! +//! *Demonstration alongside the [`Error`][std::error::Error] derive macro from [`thiserror`](https://docs.rs/thiserror/1.0.25/thiserror/index.html), +//! to propagate source locations from [`io::Error`][std::io::Error] with the `#[source]` attribute:* //! ```rust //! use std::io; //! use displaydoc::Display; @@ -21,7 +23,7 @@ //! //! #[derive(Display, Error, Debug)] //! pub enum DataStoreError { -//! /// data store disconnected +//! /// data store disconnected: {0:?} //! Disconnect(#[source] io::Error), //! /// the data for key `{0}` is not available //! Redaction(String), @@ -33,22 +35,33 @@ //! /// unknown data store error //! Unknown, //! } +//! +//! let error = DataStoreError::Redaction("CLASSIFIED CONTENT".to_string()); +//! assert!("the data for key `CLASSIFIED CONTENT` is not available" == &format!("{}", error)); //! ``` //! //!
//! //! ## Details //! -//! - A `Display` impl is generated for your type if you provide doc comment -//! messages on the struct or each variant of your enum, as shown above in the -//! example. -//! -//! The messages support a shorthand for interpolating fields from the error. -//! +//! - An `fmt::Display` impl is generated for your enum if you provide +//! a docstring comment on each variant as shown above in the example. The +//! `Display` derive macro supports a shorthand for interpolating fields from +//! the error: //! - `/// {var}` ⟶ `write!("{}", self.var)` //! - `/// {0}` ⟶ `write!("{}", self.0)` //! - `/// {var:?}` ⟶ `write!("{:?}", self.var)` //! - `/// {0:?}` ⟶ `write!("{:?}", self.0)` +//! - This also works with structs and [generic types][crate::Display#generic-type-parameters]: +//! ```rust +//! # use displaydoc::Display; +//! /// oh no, an error: {0} +//! #[derive(Display)] +//! pub struct Error(pub E); +//! +//! let error: Error<&str> = Error("muahaha i am an error"); +//! assert!("oh no, an error: muahaha i am an error" == &format!("{}", error)); +//! ``` //! //! - Two optional attributes can be added to your types next to the derive: //! @@ -71,10 +84,10 @@ //! ## FAQ //! //! 1. **Is this crate `no_std` compatible?** -//! * Yes! This crate implements the `core::fmt::Display` trait not the `std::fmt::Display` trait so it should work in `std` and `no_std` environments. Just add `default-features = false`. +//! * Yes! This crate implements the [`core::fmt::Display`] trait, not the [`std::fmt::Display`] trait, so it should work in `std` and `no_std` environments. Just add `default-features = false`. //! //! 2. **Does this crate work with `Path` and `PathBuf` via the `Display` trait?** -//! * Yuuup. This crate uses @dtolnay's [autoref specialization technique](https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md) to add a special trait for types to get the display impl, it then specializes for `Path` and `PathBuf` and when either of these types are found it calls `self.display()` to get a `std::path::Display<'_>` type which can be used with the Display format specifier! +//! * Yuuup. This crate uses @dtolnay's [autoref specialization technique](https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md) to add a special trait for types to get the display impl. It then specializes for `Path` and `PathBuf`, and when either of these types are found, it calls `self.display()` to get a `std::path::Display<'_>` type which can be used with the `Display` format specifier! #![doc(html_root_url = "https://docs.rs/displaydoc/0.2.3")] #![cfg_attr(docsrs, feature(doc_cfg))] #![warn( @@ -109,7 +122,56 @@ mod fmt; use proc_macro::TokenStream; use syn::{parse_macro_input, DeriveInput}; -/// Derive macro for implementing `Display` via doc comment attributes +/// [Custom `#[derive(...)]` macro](https://doc.rust-lang.org/edition-guide/rust-2018/macros/custom-derive.html) +/// for implementing [`fmt::Display`][core::fmt::Display] via doc comment attributes. +/// +/// ### Generic Type Parameters +/// +/// Type parameters to an enum or struct using this macro should *not* need to +/// have an explicit `Display` constraint at the struct or enum definition +/// site. A `Display` implementation for the `derive`d struct or enum is +/// generated assuming each type parameter implements `Display`, but that should +/// be possible without adding the constraint to the struct definition itself: +/// ```rust +/// use displaydoc::Display; +/// +/// /// oh no, an error: {0} +/// #[derive(Display)] +/// pub struct Error(pub E); +/// +/// // No need to require `E: Display`, since `displaydoc::Display` adds that implicitly. +/// fn generate_error(e: E) -> Error { Error(e) } +/// +/// assert!("oh no, an error: muahaha" == &format!("{}", generate_error("muahaha"))); +/// ``` +/// +/// ### Using [`Debug`][core::fmt::Debug] Implementations with Type Parameters +/// However, if a type parameter must instead be constrained with the +/// [`Debug`][core::fmt::Debug] trait so that some field may be printed with +/// `{:?}`, that constraint must currently still also be specified redundantly +/// at the struct or enum definition site. If a struct or enum field is being +/// formatted with `{:?}` via [`displaydoc`][crate], and a generic type +/// parameter must implement `Debug` to do that, then that struct or enum +/// definition will need to propagate the `Debug` constraint to every type +/// parameter it's instantiated with: +/// ```rust +/// use core::fmt::Debug; +/// use displaydoc::Display; +/// +/// /// oh no, an error: {0:?} +/// #[derive(Display)] +/// pub struct Error(pub E); +/// +/// // `E: Debug` now has to propagate to callers. +/// fn generate_error(e: E) -> Error { Error(e) } +/// +/// assert!("oh no, an error: \"cool\"" == &format!("{}", generate_error("cool"))); +/// +/// // Try this with a struct that doesn't impl `Display` at all, unlike `str`. +/// #[derive(Debug)] +/// pub struct Oh; +/// assert!("oh no, an error: Oh" == &format!("{}", generate_error(Oh))); +/// ``` #[proc_macro_derive( Display, attributes(ignore_extra_doc_attributes, prefix_enum_doc_attributes, displaydoc)