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

Automatic Generic-Derived Qualifiers #145

Closed
wants to merge 8 commits into from
Closed

Automatic Generic-Derived Qualifiers #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d2c7d4
Initial rough-out for enum generic handling
Dessix Aug 28, 2021
fedb020
Add visitor which marks provided generics by name in variants
Dessix Aug 28, 2021
64b2a0d
Support `#[from]` usage parsing using GenericUsageVisitor
Dessix Aug 29, 2021
f9ca456
Add support for generic structs
Dessix Aug 29, 2021
28751e0
Fully functional. Format literal parsing is present yet atrocious.
Dessix Aug 30, 2021
268352c
Merge remote-tracking branch 'origin/master' into thiserror-79-error-…
Dessix Aug 30, 2021
5de9589
Fix compatibility with rust 1.31.0 and 1.36.0
Dessix Aug 30, 2021
377de48
Apparently `matches` is newer than Rust 1.36.0
Dessix Aug 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion impl/Cargo.toml
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@ proc-macro = true
[dependencies]
proc-macro2 = "1.0"
quote = "1.0"
syn = "1.0.45"
syn = { version = "1.0.45", features = ["visit"] }

[package.metadata.docs.rs]
targets = ["x86_64-unknown-linux-gnu"]
302 changes: 299 additions & 3 deletions impl/src/expand.rs
View file
Open in desktop
@@ -1,8 +1,12 @@
use crate::ast::{Enum, Field, Input, Struct};
use crate::fmt::DisplayFormatMarking;
use proc_macro2::TokenStream;
use quote::{format_ident, quote, quote_spanned, ToTokens};
use syn::spanned::Spanned;
use syn::{Data, DeriveInput, GenericArgument, Member, PathArguments, Result, Type, Visibility};
use syn::{
parse_quote, Data, DeriveInput, GenericArgument, Member, PathArguments, Result, Type,
Visibility,
};

pub fn derive(node: &DeriveInput) -> Result<TokenStream> {
let input = Input::from_syn(node)?;
Expand Down Expand Up @@ -114,6 +118,48 @@ fn impl_struct(input: Struct) -> TokenStream {
None
};
let display_impl = display_body.map(|body| {
let mut extra_predicates = Vec::new();
for field in input
.attrs
.display
.iter()
.flat_map(|d| d.iter_fmt_types(input.fields.as_slice()))
{
Copy link
Owner

Choose a reason for hiding this comment

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

In general these changes in expand.rs are way more code than I would expect to need to add for this feature. Please see #148 for the approach I used instead.

Copy link
Author

Choose a reason for hiding this comment

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

Your Field.contains_generic approach combined with the scope abstraction seem to be the biggest contributors to code reduction, and InferredBounds is doing some serious work considering its length.

I'm curious, however, if there's some way to describe the "intent" for each bound as it's read, allowing a single InferredBounds-esque type to track the various usage forms to generalize the solution beyond error and display bounds.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes that is how https://github.com/dtolnay/reflect works.

Copy link
Author

Choose a reason for hiding this comment

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

I'll keep an eye out for opportunities to use that library- but I suspect it would be a bit much in the way of overhead for this task? Then again, being a proc macro, its dependencies (hopefully?) aren't compiled into the binary it helps to produce unless explicitly requested.

let (ty, bound, ast): (
&syn::Type,
syn::punctuated::Punctuated<syn::TypeParamBound, _>,
&syn::Field,
);
match field {
DisplayFormatMarking::Debug(f) => {
ty = f.ty;
bound = parse_quote! { ::std::fmt::Debug };
ast = f.original;
}
DisplayFormatMarking::Display(f) => {
ty = f.ty;
bound = parse_quote! { ::std::fmt::Display };
ast = f.original;
}
}
// If a generic is at all present, a constraint will be applied to the field type
// This may create redundant `AlwaysDebug<T>: Debug` scenarios, but covers T: Debug and &T: Debug cleanly
let mut usages = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
syn::visit::visit_field(&mut usages, ast);
if usages.iter_marked().next().is_some() {
extra_predicates.push(syn::WherePredicate::Type(syn::PredicateType {
bounded_ty: ty.clone(),
colon_token: syn::token::Colon::default(),
bounds: bound,
lifetimes: None,
}));
}
}

let where_clause = augment_where_clause(where_clause, extra_predicates);

quote! {
#[allow(unused_qualifications)]
impl #impl_generics std::fmt::Display for #ty #ty_generics #where_clause {
Expand Down Expand Up @@ -144,8 +190,46 @@ fn impl_struct(input: Struct) -> TokenStream {
}
});

let error_trait = spanned_error_trait(input.original);
let (generic_field_types, generics_in_from_types): (Vec<&syn::Type>, Vec<proc_macro2::Ident>) = {
let mut generics_in_from_types = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
let mut generic_field_types = Vec::new();
if let Some(from_field) = input.from_field() {
let mut generics_in_this_field = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
syn::visit::visit_type(&mut generics_in_this_field, &from_field.original.ty);
if generics_in_from_types.mark_from(generics_in_this_field.iter_marked()) {
generic_field_types.push(from_field.ty);
}
}
(
generic_field_types,
generics_in_from_types
.generics
.into_iter()
.map(|(k, _v)| k)
.collect(),
)
};

let extra_predicates =
std::iter::once(parse_quote! { Self: ::std::fmt::Display + ::std::fmt::Debug })
.chain(
generics_in_from_types
.into_iter()
.map(|generic| parse_quote! { #generic: 'static }),
)
.chain(
generic_field_types
.into_iter()
.map(|ty| parse_quote! { #ty: ::std::error::Error }),
);

let where_clause = augment_where_clause(where_clause, extra_predicates);

let error_trait = spanned_error_trait(input.original);
quote! {
#[allow(unused_qualifications)]
impl #impl_generics #error_trait for #ty #ty_generics #where_clause {
Expand Down Expand Up @@ -308,6 +392,47 @@ fn impl_enum(input: Enum) -> TokenStream {
} else {
None
};

let mut extra_predicates = Vec::new();
for field in input.variants.iter().flat_map(|v| {
v.attrs
.display
.iter()
.flat_map(move |d| d.iter_fmt_types(&v.fields))
}) {
let (ty, bound, ast): (
&syn::Type,
syn::punctuated::Punctuated<syn::TypeParamBound, _>,
&syn::Field,
);
match field {
DisplayFormatMarking::Debug(f) => {
ty = f.ty;
bound = parse_quote! { ::std::fmt::Debug };
ast = f.original;
}
DisplayFormatMarking::Display(f) => {
ty = f.ty;
bound = parse_quote! { ::std::fmt::Display };
ast = f.original;
}
}
// If a generic is at all present, a constraint will be applied to the field type
// This may create redundant `AlwaysDebug<T>: Debug` scenarios, but covers T: Debug and &T: Debug cleanly
let mut usages = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
syn::visit::visit_field(&mut usages, ast);
if usages.iter_marked().next().is_some() {
extra_predicates.push(syn::WherePredicate::Type(syn::PredicateType {
bounded_ty: ty.clone(),
colon_token: syn::token::Colon::default(),
bounds: bound,
lifetimes: None,
}));
}
}

let arms = input.variants.iter().map(|variant| {
let display = match &variant.attrs.display {
Some(display) => display.to_token_stream(),
Expand All @@ -325,6 +450,9 @@ fn impl_enum(input: Enum) -> TokenStream {
#ty::#ident #pat => #display
}
});

let where_clause = augment_where_clause(where_clause, extra_predicates);

Some(quote! {
#[allow(unused_qualifications)]
impl #impl_generics std::fmt::Display for #ty #ty_generics #where_clause {
Expand Down Expand Up @@ -364,8 +492,50 @@ fn impl_enum(input: Enum) -> TokenStream {
})
});

let error_trait = spanned_error_trait(input.original);
let (generic_field_types, generics_in_from_types): (Vec<&syn::Type>, Vec<proc_macro2::Ident>) = {
let mut generics_in_from_types = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
let mut generic_field_types = Vec::new();
for from_field in input
.variants
.iter()
.filter_map(|variant| variant.from_field())
{
let mut generics_in_this_field = GenericUsageVisitor::new_unmarked(
input.generics.type_params().map(|p| p.ident.clone()),
);
syn::visit::visit_type(&mut generics_in_this_field, &from_field.original.ty);
if generics_in_from_types.mark_from(generics_in_this_field.iter_marked()) {
generic_field_types.push(from_field.ty);
}
}
(
generic_field_types,
generics_in_from_types
.generics
.into_iter()
.map(|(k, _v)| k)
.collect(),
)
};

let extra_predicates =
std::iter::once(parse_quote! { Self: ::std::fmt::Display + ::std::fmt::Debug })
.chain(
generics_in_from_types
.into_iter()
.map(|generic| parse_quote! { #generic: 'static }),
)
.chain(
generic_field_types
.into_iter()
.map(|ty| parse_quote! { #ty: ::std::error::Error }),
);

let where_clause = augment_where_clause(where_clause, extra_predicates);

let error_trait = spanned_error_trait(input.original);
quote! {
#[allow(unused_qualifications)]
impl #impl_generics #error_trait for #ty #ty_generics #where_clause {
Expand All @@ -377,6 +547,132 @@ fn impl_enum(input: Enum) -> TokenStream {
}
}

#[cfg_attr(test, derive(Debug))]
#[derive(Clone)]
struct GenericUsageVisitor {
generics: std::collections::HashMap<proc_macro2::Ident, bool>,
}

impl GenericUsageVisitor {
pub fn new<TPairs>(generics: TPairs) -> Self
where
TPairs: IntoIterator<Item = (syn::Ident, bool)>,
{
Self {
generics: generics.into_iter().collect(),
}
}

pub fn new_unmarked<TIdents>(generics: TIdents) -> Self
where
TIdents: IntoIterator<Item = syn::Ident>,
{
Self::new(generics.into_iter().map(|ident| (ident, false)))
}

pub fn iter_marked(&self) -> impl Iterator<Item = &proc_macro2::Ident> {
self.generics.iter().filter(|(_k, v)| **v).map(|(k, _v)| k)
}

#[allow(dead_code)]
Copy link
Owner

Choose a reason for hiding this comment

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

No dead code please.

Copy link
Author

Choose a reason for hiding this comment

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

My bad- that one got left behind- I must've forgotten to tag it todo and wasn't clear I'd use it or not before the feature was ready.

pub fn is_marked<'a, T: PartialEq<&'a proc_macro2::Ident> + 'a>(&'a self, item: T) -> bool {
self.iter_marked().any(|ident| item == ident)
}

pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>(
&'a mut self,
Comment on lines +582 to +583
Copy link
Owner

Choose a reason for hiding this comment

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

The 'a lifetime requirements in this signature are never used.

- pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident> + 'a>(
-     &'a mut self,
+ pub fn mark_from<'a, TMarkedSource: IntoIterator<Item = &'a proc_macro2::Ident>>(
+     &mut self,

Copy link
Author

Choose a reason for hiding this comment

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

Hum. I thought if I had one explicit (the one used in IntoIterator::Item) they were supposed to all be explicit. Neat.

source: TMarkedSource,
) -> bool {
let mut any_marked = false;
for other_marked in source {
if let Some(is_marked) = self.generics.get_mut(other_marked) {
*is_marked = true;
any_marked = true;
}
}
any_marked
}
}

impl<'ast> syn::visit::Visit<'ast> for GenericUsageVisitor {
fn visit_type_path(&mut self, i: &'ast syn::TypePath) {
if let Some(ident) = i.path.get_ident() {
if let Some(entry) = self.generics.get_mut(ident) {
*entry = true;
}
}
syn::visit::visit_type_path(self, i);
}

fn visit_type_param(&mut self, i: &'ast syn::TypeParam) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is not reachable — except maybe in contrived code like field: [T; { fn f<T>() {}; 0 }] where you actually don't want the behavior here of considering the inner T TypeParam as being a use of the outer T.

Copy link
Author

Choose a reason for hiding this comment

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

I used this to deep-search for generic usages because- when a generic wasn't used- I was running into circumstances where fixed types would succeed in compilation where they should not, because it generated impossible/unlikely bounds, such as PathBuf: Display, which you seem to have resolved using the PathAsDisplay trait in the past. My solution was to deep-scan for generics in types, and only add bounds for fields which used at least one generic.

if let Some(entry) = self.generics.get_mut(&i.ident) {
*entry = true;
}
syn::visit::visit_type_param(self, i);
}
}

#[cfg(test)]
mod test_visitors {
use proc_macro2::Span;

use crate::expand::GenericUsageVisitor;

#[test]
fn test_generic_usage_visitor() {
fn ident_for<'a>(ident_str: &'a str) -> syn::Ident {
syn::Ident::new(ident_str, Span::call_site())
}

let field: syn::Variant = syn::parse_quote! { X(Foo<Bar>) };

let mut visitor = GenericUsageVisitor::new(
vec!["Bar", "Baz"]
.into_iter()
.map(|ident_str| (ident_for(ident_str), false)),
);
syn::visit::visit_variant(&mut visitor, &field);
assert_eq!(
visitor.generics.get(&ident_for("Bar")),
Some(&true),
"Bar must be marked"
);
assert_eq!(
visitor.generics.get(&ident_for("Baz")),
Some(&false),
"Baz must be present but not marked"
);
}
}

fn augment_where_clause<TPredicates>(
where_clause: Option<&syn::WhereClause>,
extra_predicates: TPredicates,
) -> Option<syn::WhereClause>
where
TPredicates: IntoIterator<Item = syn::WherePredicate>,
{
let mut extra_predicates = extra_predicates.into_iter().peekable();
if extra_predicates.peek().is_none() {
return where_clause.cloned();
}
Some(match where_clause {
Some(w) => syn::WhereClause {
where_token: w.where_token,
predicates: w
.predicates
.iter()
.cloned()
.chain(extra_predicates)
.collect(),
},
None => syn::WhereClause {
where_token: syn::token::Where::default(),
predicates: extra_predicates.into_iter().collect(),
},
})
}

fn fields_pat(fields: &[Field]) -> TokenStream {
let mut members = fields.iter().map(|field| &field.member).peekable();
match members.peek() {
Expand Down