Repository: shepmaster/snafu (rust)

Issue:
Allow visibility to be applied to context selectors
My opinion is that error types should be plentiful — each module should have one (or more!) error types. Each error type is only ever constructed by that module (and maybe child modules). Each error type is very tailored to its domain and there are less cases of "but these 31 error variants don't *actually* occur here". This fits really well with SNAFU's default of making context selectors private by default.

However, the concept of a unified error type is present in the Rust ecosystem, and I have found myself wanting to create a child's error for purposes of testing. This would be best served via marking the context selectors with `pub(crate)`. Once we open that can of worms, we can probably allow *any* visibility modifier.

How should we allow users to specify this? My first thoughts are something like this:

```rust
#[derive(Snafu)]
#[snafu::visibility(pub(crate)] // sets the default visibility (uses private if not present)
enum Error {
    Alpha,
    #[snafu::visibility(pub)] // sets the specific visibility (uses the default if not present)
    Beta,
}
```

Pull request description:
Allow specifying the visibility of the context selectors
Closes #28

Patch:
diff --git a/snafu-derive/src/lib.rs b/snafu-derive/src/lib.rs
index 18bf280..62295eb 100644
--- a/snafu-derive/src/lib.rs
+++ b/snafu-derive/src/lib.rs
@@ -12,11 +12,11 @@ use syn::parse::{Error as SynError, Result as SynResult};
 
 #[cfg_attr(
     not(feature = "unstable_display_attribute"),
-    proc_macro_derive(Snafu, attributes(snafu_display))
+    proc_macro_derive(Snafu, attributes(snafu_visibility, snafu_display))
 )]
 #[cfg_attr(
     feature = "unstable_display_attribute",
-    proc_macro_derive(Snafu, attributes(snafu::display, snafu_display))
+    proc_macro_derive(Snafu, attributes(snafu_visibility, snafu::display, snafu_display))
 )]
 pub fn snafu_derive(input: TokenStream) -> TokenStream {
     let ast = syn::parse(input).expect("Could not parse type to derive Error for");
@@ -27,6 +27,7 @@ pub fn snafu_derive(input: TokenStream) -> TokenStream {
 struct EnumInfo {
     name: syn::Ident,
     variants: Vec<VariantInfo>,
+    default_visibility: Box<quote::ToTokens>,
 }
 
 struct VariantInfo {
@@ -35,6 +36,7 @@ struct VariantInfo {
     backtrace_field: Option<Field>,
     user_fields: Vec<Field>,
     display_format: Option<DisplayFormat>,
+    visibility: Option<Box<quote::ToTokens>>,
 }
 
 enum DisplayFormat {
@@ -56,15 +58,17 @@ fn impl_hello_macro(ty: syn::DeriveInput) -> TokenStream {
 
 fn parse_snafu_information(ty: syn::DeriveInput) -> SynResult<EnumInfo> {
     use syn::spanned::Spanned;
-    use syn::{Data, Fields, Meta};
+    use syn::{Data, Fields};
+
+    let span = ty.span();
+
+    let default_visibility = parse_snafu_visibility(&ty.attrs)?;
+    let default_visibility = default_visibility.unwrap_or_else(|| private_visibility());
 
     let enum_ = match ty.data {
         Data::Enum(enum_) => enum_,
         _ => {
-            return Err(SynError::new(
-                ty.span(),
-                "Can only derive `Snafu` for an enum",
-            ));
+            return Err(SynError::new(span, "Can only derive `Snafu` for an enum"));
         }
     };
 
@@ -76,33 +80,8 @@ fn parse_snafu_information(ty: syn::DeriveInput) -> SynResult<EnumInfo> {
         .map(|variant| {
             let name = variant.ident;
 
-            let display_format = variant
-                .attrs
-                .into_iter()
-                .flat_map(|attr| {
-                    if is_snafu_display(&attr.path) {
-                        Some(parse_snafu_display_beautiful(attr))
-                    } else if attr.path.is_ident("snafu_display") {
-                        let meta = match attr.parse_meta() {
-                            Ok(meta) => meta,
-                            Err(e) => return Some(Err(e)),
-                        };
-                        match meta {
-                            Meta::List(list) => Some(parse_snafu_display_nested(list)),
-                            Meta::NameValue(nv) => Some(parse_snafu_display_nested_name_value(nv)),
-                            meta => Some(Err(SynError::new(
-                                meta.span(),
-                                "`snafu_display` requires an argument",
-                            ))),
-                        }
-                    } else {
-                        // These are ignored, hopefully they belong to
-                        // someone else...
-                        None
-                    }
-                })
-                .next()
-                .my_transpose()?;
+            let display_format = parse_snafu_display(&variant.attrs)?;
+            let visibility = parse_snafu_visibility(&variant.attrs)?;
 
             let fields = match variant.fields {
                 Fields::Named(f) => f.named.into_iter().collect(),
@@ -147,28 +126,111 @@ fn parse_snafu_information(ty: syn::DeriveInput) -> SynResult<EnumInfo> {
                 backtrace_field,
                 user_fields,
                 display_format,
+                visibility,
             })
         })
         .collect();
     let variants = variants?;
 
-    Ok(EnumInfo { name, variants })
+    Ok(EnumInfo {
+        name,
+        variants,
+        default_visibility,
+    })
 }
 
 fn is_snafu_display(p: &syn::Path) -> bool {
-    let parts = ["snafu", "display"];
+    is_path(p, &["snafu", "display"])
+}
+
+fn is_path(p: &syn::Path, parts: &[&str]) -> bool {
     p.segments
         .iter()
-        .zip(&parts)
+        .zip(parts)
         .map(|(a, b)| a.ident == b)
         .all(|b| b)
 }
 
-fn parse_snafu_display_beautiful(attr: syn::Attribute) -> SynResult<DisplayFormat> {
+fn parse_snafu_visibility(attrs: &[syn::Attribute]) -> SynResult<Option<Box<quote::ToTokens>>> {
+    use syn::spanned::Spanned;
+    use syn::Meta;
+
+    attrs
+        .into_iter()
+        .flat_map(|attr| {
+            if attr.path.is_ident("snafu_visibility") {
+                let meta = match attr.parse_meta() {
+                    Ok(meta) => meta,
+                    Err(e) => return Some(Err(e)),
+                };
+                match meta {
+                    Meta::Word(_) => Some(Ok(private_visibility())),
+                    Meta::NameValue(nv) => Some(parse_snafu_visibility_nested_name_value(nv)),
+                    meta => Some(Err(SynError::new(
+                        meta.span(),
+                        "`snafu_visibility` either has no argument or uses an equal sign",
+                    ))),
+                }
+            } else {
+                None
+            }
+        })
+        .next()
+        .my_transpose()
+}
+
+fn private_visibility() -> Box<quote::ToTokens> {
+    Box::new(quote! {})
+}
+
+fn parse_snafu_visibility_nested_name_value(
+    nv: syn::MetaNameValue,
+) -> SynResult<Box<quote::ToTokens>> {
+    use syn::Visibility;
+
+    let s = unpack_nv_string_literal(nv)?;
+    let v = s.parse::<Visibility>()?;
+
+    Ok(Box::new(v))
+}
+
+fn parse_snafu_display(attrs: &[syn::Attribute]) -> SynResult<Option<DisplayFormat>> {
+    use syn::spanned::Spanned;
+    use syn::Meta;
+
+    attrs
+        .into_iter()
+        .flat_map(|attr| {
+            if is_snafu_display(&attr.path) {
+                Some(parse_snafu_display_beautiful(attr))
+            } else if attr.path.is_ident("snafu_display") {
+                let meta = match attr.parse_meta() {
+                    Ok(meta) => meta,
+                    Err(e) => return Some(Err(e)),
+                };
+                match meta {
+                    Meta::List(list) => Some(parse_snafu_display_nested(list)),
+                    Meta::NameValue(nv) => Some(parse_snafu_display_nested_name_value(nv)),
+                    meta => Some(Err(SynError::new(
+                        meta.span(),
+                        "`snafu_display` requires an argument",
+                    ))),
+                }
+            } else {
+                // These are ignored, hopefully they belong to
+                // someone else...
+                None
+            }
+        })
+        .next()
+        .my_transpose()
+}
+
+fn parse_snafu_display_beautiful(attr: &syn::Attribute) -> SynResult<DisplayFormat> {
     use syn::spanned::Spanned;
     use syn::Expr;
 
-    let expr: Expr = syn::parse2(attr.tts)?;
+    let expr: Expr = syn::parse2(attr.tts.clone())?;
     let expr: Box<quote::ToTokens> = match expr {
         Expr::Tuple(expr_tuple) => Box::new(expr_tuple.elems),
         Expr::Paren(expr_paren) => Box::new(expr_paren.expr),
@@ -215,15 +277,9 @@ fn parse_snafu_display_nested(meta: syn::MetaList) -> SynResult<DisplayFormat> {
 }
 
 fn parse_snafu_display_nested_name_value(nv: syn::MetaNameValue) -> SynResult<DisplayFormat> {
-    use syn::spanned::Spanned;
-    use syn::{Expr, Lit};
+    use syn::Expr;
 
-    let s = match nv.lit {
-        Lit::Str(s) => s,
-        _ => {
-            return Err(SynError::new(nv.lit.span(), "A string literal is expected"));
-        }
-    };
+    let s = unpack_nv_string_literal(nv)?;
 
     let expr: Box<quote::ToTokens> = match s.parse::<Expr>()? {
         Expr::Tuple(expr_tuple) => Box::new(expr_tuple.elems),
@@ -239,6 +295,16 @@ fn parse_snafu_display_nested_name_value(nv: syn::MetaNameValue) -> SynResult<Di
     Ok(DisplayFormat::Direct(expr))
 }
 
+fn unpack_nv_string_literal(nv: syn::MetaNameValue) -> SynResult<syn::LitStr> {
+    use syn::spanned::Spanned;
+    use syn::Lit;
+
+    match nv.lit {
+        Lit::Str(s) => Ok(s),
+        _ => Err(SynError::new(nv.lit.span(), "A string literal is expected")),
+    }
+}
+
 impl From<EnumInfo> for proc_macro::TokenStream {
     fn from(other: EnumInfo) -> proc_macro::TokenStream {
         other.generate_snafu().into()
@@ -300,6 +366,12 @@ impl<'a> quote::ToTokens for ContextSelector<'a> {
             .take(user_fields.len())
             .collect();
 
+        let visibility = self
+            .1
+            .visibility
+            .as_ref()
+            .unwrap_or(&self.0.default_visibility);
+
         let generics_list = quote! { <#(#generic_names),*> };
         let selector_name = quote! { #variant_name#generics_list };
 
@@ -309,11 +381,11 @@ impl<'a> quote::ToTokens for ContextSelector<'a> {
         let variant_selector_struct = {
             if user_fields.is_empty() {
                 quote! {
-                    struct #selector_name;
+                    #visibility struct #selector_name;
                 }
             } else {
                 quote! {
-                    struct #selector_name {
+                    #visibility struct #selector_name {
                         #( #names: #types ),*
                     }
                 }
@@ -343,7 +415,7 @@ impl<'a> quote::ToTokens for ContextSelector<'a> {
                 where
                     #(#where_clauses),*
                 {
-                    fn fail<T>(self) -> std::result::Result<T, #enum_name> {
+                    #visibility fn fail<T>(self) -> std::result::Result<T, #enum_name> {
                         let Self { #(#names),* } = self;
                         let error = #enum_name::#variant_name {
                             #backtrace_field
diff --git a/src/lib.rs b/src/lib.rs
index 72fc941..e237cc4 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -162,6 +162,37 @@
 //! variant will be available and you can call methods on them, such
 //! as `filename.display()`.
 //!
+//! #### Controlling visibility
+//!
+//! By default, each of the context selectors and their inherent
+//! methods will be private. It is our opinion that each module should
+//! have one or more error types that are scoped to that module,
+//! reducing the need to deal with unrelated errors when matching and
+//! increasing cohesiveness.
+//!
+//! If you need access the context selectors outside of their module,
+//! you can use the `#[snafu_visibility]` attribute. This can be
+//! applied to the error type as a default visibility or to specific
+//! context selectors.
+//!
+//! There are two forms of the attribute:
+//!
+//! - `#[snafu_visibility = "X"]`, where `X` is a normal Rust
+//!   visibility modifier (`pub`, `pub(crate)`, `pub(in some::path)`,
+//!   etc.)
+//! - `#[snafu_visibility]` will reset back to private visibility.
+//!
+//! ```
+//! # use snafu::Snafu;
+//! #[derive(Debug, Snafu)]
+//! #[snafu_visibility = "pub(crate)"] // Default
+//! enum Error {
+//!     IsPubCrate, // Uses the default
+//!     #[snafu_visibility]
+//!     IsPrivate,  // Will be private
+//! }
+//! ```
+//!
 //! ## Version compatibility
 //!
 //! SNAFU is tested and compatible back to Rust 1.18, released on


Test patch:
diff --git a/snafu-derive/tests/compile-fail/visibility.rs b/snafu-derive/tests/compile-fail/visibility.rs
new file mode 100644
index 0000000..49c884f
--- /dev/null
+++ b/snafu-derive/tests/compile-fail/visibility.rs
@@ -0,0 +1,34 @@
+extern crate snafu;
+
+// There are also happy-path tests
+
+mod outer {
+    pub mod inner {
+        use snafu::Snafu;
+
+        #[derive(Debug, Snafu)]
+        #[snafu_visibility = "pub(crate)"]
+        pub(crate) enum Error {
+            PubCrate,
+            #[snafu_visibility = "pub(in ::outer)"]
+            PubInPath,
+            #[snafu_visibility]
+            Private,
+        }
+    }
+
+    fn private_is_applied() {
+        let _ = self::inner::Private.fail::<()>();
+        //~^ ERROR `Private` is private
+    }
+}
+
+fn pub_in_path_is_applied() {
+    let _ = self::outer::inner::PubInPath.fail::<()>();
+    //~^ ERROR `PubInPath` is private
+}
+
+fn private_is_applied() {
+    let _ = self::outer::inner::Private.fail::<()>();
+    //~^ ERROR `Private` is private
+}
diff --git a/tests/visibility.rs b/tests/visibility.rs
new file mode 100644
index 0000000..64b42aa
--- /dev/null
+++ b/tests/visibility.rs
@@ -0,0 +1,34 @@
+extern crate snafu;
+
+// There are also sad-path tests
+
+mod outer {
+    pub mod inner {
+        use snafu::Snafu;
+
+        #[derive(Debug, Snafu)]
+        #[snafu_visibility = "pub(crate)"]
+        pub(crate) enum Error {
+            PubCrate,
+            #[snafu_visibility = "pub(in ::outer)"]
+            PubInPath,
+            #[snafu_visibility]
+            Private,
+        }
+    }
+
+    #[test]
+    fn can_set_default_visibility() {
+        let _ = self::inner::PubCrate.fail::<()>();
+    }
+
+    #[test]
+    fn can_set_visibility() {
+        let _ = self::inner::PubInPath.fail::<()>();
+    }
+}
+
+#[test]
+fn can_set_default_visibility() {
+    let _ = self::outer::inner::PubCrate.fail::<()>();
+}