Merge pull request #21 from LukasKalbertodt/better-generic-names

Better lifetime/type parameter names

Author: KodrAus

examples/names.rs

@@ -0,0 +1,61 @@
+//! Example to demonstrate how `auto_impl` chooses a name for the type and
+//! lifetime parameter.
+//!
+//! For documentation and compiler errors it would be nice to have very simple
+//! names for type and lifetime parameters:
+//!
+//! ```rust
+//! // not nice
+//! impl<'auto_impl_lifetime, AutoImplT> Foo for &'auto_impl_lifetime AutoImplT { ...}
+//!
+//! // better
+//! impl<'a, T> Foo for &'a T { ... }
+//! ```
+//!
+//! `auto_impl` tries the full alphabet, picking a name that is not yet taken.
+//! "Not taken" means that the name is not used anywhere in the `impl` block.
+//! Right now, we are a bit careful and mark all names as "taken" that are used
+//! in the trait def -- apart from names only appearing in the body of provided
+//! methods.
+//!
+//! In the trait below, a bunch of names are already "taken":
+//! - type names: T--Z and A--G (H is not taken, because it only appear in the
+//!   default method body)
+//! - lifetime names: 'a--'c
+//!
+//! Thus, the names `H` and `'d` are used. Run `cargo expand --example names`
+//! to see the output.
+
+
+// This code is really ugly on purpose...
+#![allow(non_snake_case, dead_code, unused_variables)]
+#![feature(use_extern_macros)]
+
+extern crate auto_impl;
+use auto_impl::auto_impl;
+
+
+
+struct X {}
+trait Z {}
+
+struct C {}
+struct E<T>(Vec<T>);
+struct F {}
+
+struct G<T>(Vec<T>);
+struct H {}
+
+#[auto_impl(&)]
+trait U<'a, V> {
+    const W: Option<Box<&'static X>>;
+
+    type Y: Z;
+
+    fn A<'b, 'c>(&self, B: C, D: E<&[F; 1]>) -> G<fn((H,))> {
+        let H = ();
+        unimplemented!()
+    }
+}
+
+fn main() {}

src/analyze.rs

@@ -0,0 +1,103 @@
+use std::collections::HashSet;
+
+use proc_macro::Span;
+use proc_macro2::Span as Span2;
+use syn::{
+    Ident, ItemTrait, Lifetime, Block,
+    token::Apostrophe,
+    visit::{Visit, visit_item_trait},
+};
+
+
+/// The type parameter used in the proxy type. Usually, one would just use `T`,
+/// but this could conflict with type parameters on the trait.
+///
+/// Why do we have to care about this? Why about hygiene? In the first version
+/// of stable proc_macros, only call site spans are included. That means that
+/// we cannot generate spans that do not conflict with any other ident the user
+/// wrote. Once proper hygiene is available to proc_macros, this should be
+/// changed.
+const PROXY_TY_PARAM_NAME: &str = "__AutoImplProxyT";
+
+/// The lifetime parameter used in the proxy type if the proxy type is `&` or
+/// `&mut`. For more information see `PROXY_TY_PARAM_NAME`.
+const PROXY_LT_PARAM_NAME: &str = "'__auto_impl_proxy_lifetime";
+
+
+/// We need to introduce our own type and lifetime parameter. Regardless of
+/// what kind of hygiene we use for the parameter, it would be nice (for docs
+/// and compiler errors) if the names are as simple as possible ('a and T, for
+/// example).
+///
+/// This function searches for names that we can use. Such a name must not
+/// conflict with any other name we'll use in the `impl` block. Luckily, we
+/// know all those names in advance.
+///
+/// The idea is to collect all names that might conflict with our names, store
+/// them in a set and later check which name we can use. If we can't use a simple
+/// name, we'll use the ugly `PROXY_TY_PARAM_NAME` and `PROXY_LT_PARAM_NAME`.
+///
+/// This method returns two idents: (type_parameter, lifetime_parameter).
+pub(crate) fn find_suitable_param_names(trait_def: &ItemTrait) -> (Ident, Lifetime) {
+    // Define the visitor that just collects names
+    struct IdentCollector<'ast> {
+        ty_names: HashSet<&'ast Ident>,
+        lt_names: HashSet<&'ast Ident>,
+    }
+
+    impl<'ast> Visit<'ast> for IdentCollector<'ast> {
+        fn visit_ident(&mut self, i: &'ast Ident) {
+            self.ty_names.insert(i);
+        }
+
+        // We overwrite this to make sure to put lifetime names into
+        // `lt_names`. We also don't recurse, so `visit_ident` won't be called
+        // for lifetime names.
+        fn visit_lifetime(&mut self, lt: &'ast Lifetime) {
+            self.lt_names.insert(&lt.ident);
+        }
+
+        // Visiting a block just does nothing. It is the default body of a method
+        // in the trait. But since that block won't be in the impl block, we can
+        // just ignore it.
+        fn visit_block(&mut self, _: &'ast Block) {}
+    }
+
+    // Create the visitor and visit the trait
+    let mut visitor = IdentCollector {
+        ty_names: HashSet::new(),
+        lt_names: HashSet::new(),
+    };
+    visit_item_trait(&mut visitor, trait_def);
+
+
+    fn param_span() -> Span2 {
+        // TODO: change for stable builds
+        Span::def_site().into()
+    }
+
+    fn char_to_ident(c: u8) -> Ident {
+        let arr = [c];
+        let s = ::std::str::from_utf8(&arr).unwrap();
+        Ident::new(s, param_span())
+    }
+
+    // Find suitable type name (T..=Z and A..=S)
+    let ty_name = (b'T'..=b'Z')
+        .chain(b'A'..=b'S')
+        .map(char_to_ident)
+        .find(|i| !visitor.ty_names.contains(i))
+        .unwrap_or_else(|| Ident::new(PROXY_TY_PARAM_NAME, param_span()));
+
+    // Find suitable lifetime name ('a..='z)
+    let lt_name = (b'a'..=b'z')
+        .map(char_to_ident)
+        .find(|i| !visitor.lt_names.contains(i))
+        .unwrap_or_else(|| Ident::new(PROXY_LT_PARAM_NAME, param_span()));
+    let lt = Lifetime {
+        apostrophe: Apostrophe::new(param_span()),
+        ident: lt_name,
+    };
+
+    (ty_name, lt)
+}

src/diag.rs

@@ -0,0 +1,18 @@
+use proc_macro::{Diagnostic, Span};
+
+
+pub trait DiagnosticExt {
+    /// Helper function to add a note to the diagnostic (with a span pointing
+    /// to the `auto_impl` attribute) and emit the error. Additionally,
+    /// `Err(())` is always returned.
+    fn emit_with_attr_note<T>(self) -> Result<T, ()>;
+}
+
+impl DiagnosticExt for Diagnostic {
+    fn emit_with_attr_note<T>(self) -> Result<T, ()> {
+        self.span_note(Span::call_site(), "auto-impl requested here")
+            .emit();
+
+        Err(())
+    }
+}

src/gen.rs

@@ -1,26 +1,18 @@
-use proc_macro::{Diagnostic, Span};
-use proc_macro2::{Span as Span2, TokenStream as TokenStream2};
+use proc_macro::Span;
+use proc_macro2::TokenStream as TokenStream2;
 use quote::{ToTokens, TokenStreamExt};
 use syn::{
     FnArg, Ident, ItemTrait, Lifetime, MethodSig, Pat, PatIdent, TraitItem, TraitItemMethod,
     TraitItemType, TraitItemConst,
 };
 
-use crate::{proxy::ProxyType, spanned::Spanned};
-
-/// The type parameter used in the proxy type. Usually, one would just use `T`,
-/// but this could conflict with type parameters on the trait.
-///
-/// Why do we have to care about this? Why about hygiene? In the first version
-/// of stable proc_macros, only call site spans are included. That means that
-/// we cannot generate spans that do not conflict with any other ident the user
-/// wrote. Once proper hygiene is available to proc_macros, this should be
-/// changed.
-const PROXY_TY_PARAM_NAME: &str = "__AutoImplProxyT";
+use crate::{
+    analyze::{find_suitable_param_names},
+    diag::DiagnosticExt,
+    proxy::ProxyType,
+    spanned::Spanned
+};
 
-/// The lifetime parameter used in the proxy type if the proxy type is `&` or
-/// `&mut`. For more information see `PROXY_TY_PARAM_NAME`.
-const PROXY_LT_PARAM_NAME: &str = "'__auto_impl_proxy_lifetime";
 
 
 /// Generates one complete impl of the given trait for each of the given proxy
@@ -31,10 +23,12 @@ pub(crate) fn gen_impls(
 ) -> Result<::proc_macro::TokenStream, ()> {
     let mut tokens = TokenStream2::new();
 
+    let (proxy_ty_param, proxy_lt_param) = find_suitable_param_names(trait_def);
+
     // One impl for each proxy type
     for proxy_type in proxy_types {
-        let header = header(proxy_type, trait_def)?;
-        let items = gen_items(proxy_type, trait_def)?;
+        let header = header(proxy_type, trait_def, &proxy_ty_param, &proxy_lt_param)?;
+        let items = gen_items(proxy_type, trait_def, &proxy_ty_param)?;
 
         tokens.append_all(quote! {
             #header { #( #items )* }
@@ -46,10 +40,12 @@ pub(crate) fn gen_impls(
 
 /// Generates the header of the impl of the given trait for the given proxy
 /// type.
-fn header(proxy_type: &ProxyType, trait_def: &ItemTrait) -> Result<TokenStream2, ()> {
-    let proxy_ty_param = Ident::new(PROXY_TY_PARAM_NAME, Span2::call_site());
-    let proxy_lt_param = &Lifetime::new(PROXY_LT_PARAM_NAME, Span2::call_site());
-
+fn header(
+    proxy_type: &ProxyType,
+    trait_def: &ItemTrait,
+    proxy_ty_param: &Ident,
+    proxy_lt_param: &Lifetime,
+) -> Result<TokenStream2, ()> {
     // Generate generics for impl positions from trait generics.
     let (impl_generics, trait_generics, where_clause) = trait_def.generics.split_for_impl();
 
@@ -63,10 +59,10 @@ fn header(proxy_type: &ProxyType, trait_def: &ItemTrait) -> Result<TokenStream2,
     // one or two parameters added. For a trait `trait Foo<'x, 'y, A, B>`,
     // it will look like this:
     //
-    //    '__auto_impl_proxy_lifetime, 'x, 'y, A, B, __AutoImplProxyT
+    //    '{proxy_lt_param}, 'x, 'y, A, B, {proxy_ty_param}
     //
-    // The `'__auto_impl_proxy_lifetime` in the beginning is only added when
-    // the proxy type is `&` or `&mut`.
+    // The `'{proxy_lt_param}` in the beginning is only added when the proxy
+    // type is `&` or `&mut`.
     let impl_generics = {
         // Determine if our proxy type needs a lifetime parameter
         let (mut params, ty_bounds) = match proxy_type {
@@ -297,12 +293,19 @@ fn gen_fn_type_for_trait(
 fn gen_items(
     proxy_type: &ProxyType,
     trait_def: &ItemTrait,
+    proxy_ty_param: &Ident,
 ) -> Result<Vec<TokenStream2>, ()> {
     trait_def.items.iter().map(|item| {
         match item {
-            TraitItem::Const(c) => gen_const_item(proxy_type, c, trait_def),
-            TraitItem::Method(method) => gen_method_item(proxy_type, method, trait_def),
-            TraitItem::Type(ty) => gen_type_item(proxy_type, ty, trait_def),
+            TraitItem::Const(c) => {
+                gen_const_item(proxy_type, c, trait_def, proxy_ty_param)
+            }
+            TraitItem::Method(method) => {
+                gen_method_item(proxy_type, method, trait_def, proxy_ty_param)
+            }
+            TraitItem::Type(ty) => {
+                gen_type_item(proxy_type, ty, trait_def, proxy_ty_param)
+            }
             TraitItem::Macro(mac) => {
                 // We cannot resolve the macro invocation and thus cannot know
                 // if it adds additional items to the trait. Thus, we have to
@@ -334,6 +337,7 @@ fn gen_const_item(
     proxy_type: &ProxyType,
     item: &TraitItemConst,
     trait_def: &ItemTrait,
+    proxy_ty_param: &Ident,
 ) -> Result<TokenStream2, ()> {
     // A trait with associated consts cannot be implemented for Fn* types.
     if proxy_type.is_fn() {
@@ -350,7 +354,6 @@ fn gen_const_item(
     // We simply use the associated const from our type parameter.
     let const_name = &item.ident;
     let const_ty = &item.ty;
-    let proxy_ty_param = Ident::new(PROXY_TY_PARAM_NAME, Span2::call_site());
 
     Ok(quote ! {
         const #const_name: #const_ty = #proxy_ty_param::#const_name;
@@ -366,6 +369,7 @@ fn gen_type_item(
     proxy_type: &ProxyType,
     item: &TraitItemType,
     trait_def: &ItemTrait,
+    proxy_ty_param: &Ident,
 ) -> Result<TokenStream2, ()> {
     // A trait with associated types cannot be implemented for Fn* types.
     if proxy_type.is_fn() {
@@ -381,7 +385,6 @@ fn gen_type_item(
 
     // We simply use the associated type from our type parameter.
     let assoc_name = &item.ident;
-    let proxy_ty_param = Ident::new(PROXY_TY_PARAM_NAME, Span2::call_site());
 
     Ok(quote ! {
         type #assoc_name = #proxy_ty_param::#assoc_name;
@@ -398,6 +401,7 @@ fn gen_method_item(
     proxy_type: &ProxyType,
     item: &TraitItemMethod,
     trait_def: &ItemTrait,
+    proxy_ty_param: &Ident,
 ) -> Result<TokenStream2, ()> {
     // Determine the kind of the method, determined by the self type.
     let sig = &item.sig;
@@ -421,7 +425,6 @@ fn gen_method_item(
         // No receiver
         SelfType::None => {
             // The proxy type is a reference, smartpointer or Box.
-            let proxy_ty_param = Ident::new(PROXY_TY_PARAM_NAME, Span2::call_site());
             quote! { #proxy_ty_param::#name(#args) }
         }
 
@@ -588,19 +591,3 @@ fn get_arg_list(inputs: impl Iterator<Item = &'a FnArg>) -> Result<TokenStream2,
 
     Ok(args)
 }
-
-trait DiagnosticExt {
-    /// Helper function to add a note to the diagnostic (with a span pointing
-    /// to the `auto_impl` attribute) and emit the error. Additionally,
-    /// `Err(())` is always returned.
-    fn emit_with_attr_note<T>(self) -> Result<T, ()>;
-}
-
-impl DiagnosticExt for Diagnostic {
-    fn emit_with_attr_note<T>(self) -> Result<T, ()> {
-        self.span_note(Span::call_site(), "auto-impl requested here")
-            .emit();
-
-        Err(())
-    }
-}

src/lib.rs

@@ -16,6 +16,8 @@ extern crate syn;
 
 use proc_macro::{Diagnostic, Level, Span, TokenStream};
 
+mod analyze;
+mod diag;
 mod gen;
 mod proxy;
 mod spanned;