Further improve error messages (#346)

* Further improve error messages

structopt-derive/Cargo.toml

@@ -18,7 +18,7 @@ syn = { version = "1", default-features = false, features = ["derive", "parsing"
 quote = "1"
 proc-macro2 = "1"
 heck = "0.3.0"
-proc-macro-error = "0.4.3"
+proc-macro-error = "0.4.7"
 
 [features]
 paw = []

structopt-derive/src/attrs.rs

@@ -103,7 +103,7 @@ impl Method {
             None => match env::var(env_var) {
                 Ok(val) => LitStr::new(&val, ident.span()),
                 Err(_) => {
-                    abort!(ident.span(),
+                    abort!(ident,
                         "cannot derive `{}` from Cargo.toml", ident;
                         note = "`{}` environment variable is not set", env_var;
                         help = "use `{} = \"...\"` to set {} manually", ident, ident;
@@ -145,7 +145,7 @@ impl Parser {
             "try_from_os_str" => TryFromOsStr,
             "from_occurrences" => FromOccurrences,
             "from_flag" => FromFlag,
-            s => abort!(spec.kind.span(), "unsupported parser `{}`", s),
+            s => abort!(spec.kind, "unsupported parser `{}`", s),
         };
 
         let func = match spec.parse_func {
@@ -155,7 +155,7 @@ impl Parser {
                 }
                 TryFromStr => quote_spanned!(spec.kind.span()=> ::std::str::FromStr::from_str),
                 TryFromOsStr => abort!(
-                    spec.kind.span(),
+                    spec.kind,
                     "you must set parser for `try_from_os_str` explicitly"
                 ),
                 FromOccurrences => quote_spanned!(spec.kind.span()=> { |v| v as _ }),
@@ -164,7 +164,7 @@ impl Parser {
 
             Some(func) => match func {
                 syn::Expr::Path(_) => quote!(#func),
-                _ => abort!(func.span(), "`parse` argument must be a function path"),
+                _ => abort!(func, "`parse` argument must be a function path"),
             },
         };
 
@@ -188,7 +188,7 @@ impl CasingStyle {
             "screamingsnake" | "screamingsnakecase" => cs(ScreamingSnake),
             "snake" | "snakecase" => cs(Snake),
             "verbatim" | "verbatimcase" => cs(Verbatim),
-            s => abort!(name.span(), "unsupported casing: `{}`", s),
+            s => abort!(name, "unsupported casing: `{}`", s),
         }
     }
 }
@@ -303,7 +303,7 @@ impl Attrs {
                             ty
                         } else {
                             abort!(
-                                ident.span(),
+                                ident,
                                 "#[structopt(default_value)] (without an argument) can be used \
                                 only on field level";
 
@@ -420,7 +420,7 @@ impl Attrs {
         let name = field.ident.clone().unwrap();
         let mut res = Self::new(
             field.span(),
-            Name::Derived(name.clone()),
+            Name::Derived(name),
             parent_attrs,
             Some(field.ty.clone()),
             struct_casing,
@@ -465,13 +465,13 @@ impl Attrs {
                 match *ty {
                     Ty::OptionOption => {
                         abort!(
-                            ty.span(),
+                            field.ty,
                             "Option<Option<T>> type is not allowed for subcommand"
                         );
                     }
                     Ty::OptionVec => {
                         abort!(
-                            ty.span(),
+                            field.ty,
                             "Option<Vec<T>> type is not allowed for subcommand"
                         );
                     }
@@ -500,7 +500,7 @@ impl Attrs {
                 match *ty {
                     Ty::Bool => {
                         if res.is_positional() && !res.has_custom_parser {
-                            abort!(ty.span(),
+                            abort!(field.ty,
                                 "`bool` cannot be used as positional parameter with default parser";
                                 help = "if you want to create a flag add `long` or `short`";
                                 help = "If you really want a boolean parameter \
@@ -509,32 +509,32 @@ impl Attrs {
                             )
                         }
                         if let Some(m) = res.find_method("default_value") {
-                            abort!(m.name.span(), "default_value is meaningless for bool")
+                            abort!(m.name, "default_value is meaningless for bool")
                         }
                         if let Some(m) = res.find_method("required") {
-                            abort!(m.name.span(), "required is meaningless for bool")
+                            abort!(m.name, "required is meaningless for bool")
                         }
                     }
                     Ty::Option => {
                         if let Some(m) = res.find_method("default_value") {
-                            abort!(m.name.span(), "default_value is meaningless for Option")
+                            abort!(m.name, "default_value is meaningless for Option")
                         }
                         if let Some(m) = res.find_method("required") {
-                            abort!(m.name.span(), "required is meaningless for Option")
+                            abort!(m.name, "required is meaningless for Option")
                         }
                     }
                     Ty::OptionOption => {
                         if res.is_positional() {
                             abort!(
-                                ty.span(),
+                                field.ty,
                                 "Option<Option<T>> type is meaningless for positional argument"
                             )
                         }
                     }
                     Ty::OptionVec => {
                         if res.is_positional() {
                             abort!(
-                                ty.span(),
+                                field.ty,
                                 "Option<Vec<T>> type is meaningless for positional argument"
                             )
                         }

structopt-derive/src/doc_comments.rs

@@ -74,7 +74,7 @@ fn split_paragraphs(lines: &[&str]) -> Vec<String> {
         let len = slice
             .iter()
             .position(|s| is_blank(s))
-            .unwrap_or(slice.len());
+            .unwrap_or_else(|| slice.len());
 
         last_line += start + len;
 

structopt-derive/src/lib.rs

@@ -487,7 +487,7 @@ fn gen_augment_clap_enum(
                         }
                     },
                     _ => abort!(
-                        variant.span(),
+                        variant,
                         "`flatten` is usable only with single-typed tuple variants"
                     ),
                 }
@@ -515,7 +515,7 @@ fn gen_augment_clap_enum(
                             }
                         }
                     }
-                    Unnamed(..) => abort!(variant.span(), "non single-typed tuple enums are not supported"),
+                    Unnamed(..) => abort!(variant, "non single-typed tuple enums are not supported"),
                 };
 
                 let name = attrs.cased_name();
@@ -590,7 +590,7 @@ fn gen_from_subcommand(
                     Unnamed(ref fields) if fields.unnamed.len() == 1 => &fields.unnamed[0].ty,
 
                     _ => abort!(
-                        variant.span(),
+                        variant,
                         "The enum variant marked with `external_attribute` must be \
                          a single-typed tuple, and the type must be either `Vec<String>` \
                          or `Vec<OsString>`."
@@ -615,7 +615,7 @@ fn gen_from_subcommand(
                     }
 
                     None => abort!(
-                        ty.span(),
+                        ty,
                         "The type must be either `Vec<String>` or `Vec<OsString>` \
                          to be used with `external_subcommand`."
                     ),
@@ -670,7 +670,7 @@ fn gen_from_subcommand(
                 quote!( ( <#ty as ::structopt::StructOpt>::from_clap(matches) ) )
             }
             Unnamed(..) => abort!(
-                variant.ident.span(),
+                variant.ident,
                 "non single-typed tuple enums are not supported"
             ),
         };
@@ -696,7 +696,7 @@ fn gen_from_subcommand(
                 }
             }
             _ => abort!(
-                variant.span(),
+                variant,
                 "`flatten` is usable only with single-typed tuple variants"
             ),
         }

structopt-derive/src/parse.rs

@@ -6,7 +6,6 @@ use syn::{
     self, parenthesized,
     parse::{Parse, ParseBuffer, ParseStream},
     punctuated::Punctuated,
-    spanned::Spanned,
     Attribute, Expr, ExprLit, Ident, Lit, LitBool, LitStr, Token,
 };
 
@@ -63,15 +62,15 @@ impl Parse for StructOptAttr {
                 let check_empty_lit = |s| {
                     if lit_str.is_empty() {
                         abort!(
-                            lit.span(),
+                            lit,
                             "`#[structopt({} = \"\")]` is deprecated in structopt 0.3, \
                              now it's default behavior",
                             s
                         );
                     }
                 };
 
-                match &*name_str.to_string() {
+                match &*name_str {
                     "rename_all" => Ok(RenameAll(name, lit)),
                     "rename_all_env" => Ok(RenameAllEnv(name, lit)),
                     "default_value" => Ok(DefaultValue(name, Some(lit))),
@@ -113,7 +112,7 @@ impl Parse for StructOptAttr {
                     }
 
                     Err(_) => abort! {
-                        assign_token.span(),
+                        assign_token,
                         "expected `string literal` or `expression` after `=`"
                     },
                 }
@@ -131,7 +130,7 @@ impl Parse for StructOptAttr {
                     if parser_specs.len() == 1 {
                         Ok(Parse(name, parser_specs[0].clone()))
                     } else {
-                        abort!(name.span(), "parse must have exactly one argument")
+                        abort!(name, "`parse` must have exactly one argument")
                     }
                 }
 
@@ -146,7 +145,7 @@ impl Parse for StructOptAttr {
                     }
 
                     Err(_) => {
-                        abort!(name.span(),
+                        abort!(name,
                             "`#[structopt(raw(...))` attributes are removed in structopt 0.3, \
                             they are replaced with raw methods";
                             help = "if you meant to call `clap::Arg::raw()` method \
@@ -181,13 +180,13 @@ impl Parse for StructOptAttr {
                 "skip" => Ok(Skip(name, None)),
 
                 "version" => abort!(
-                    name.span(),
+                    name,
                     "#[structopt(version)] is invalid attribute, \
                      structopt 0.3 inherits version from Cargo.toml by default, \
                      no attribute needed"
                 ),
 
-                _ => abort!(name.span(), "unexpected attribute: {}", name_str),
+                _ => abort!(name, "unexpected attribute: {}", name_str),
             }
         }
     }

tests/ui/enum_flatten.stderr

@@ -1,5 +1,6 @@
 error: `flatten` is usable only with single-typed tuple variants
   --> $DIR/enum_flatten.rs:14:5
    |
-14 |     #[structopt(flatten)]
-   |     ^
+14 | /     #[structopt(flatten)]
+15 | |     Variant1,
+   | |____________^

tests/ui/external_subcommand_wrong_type.stderr

@@ -4,5 +4,5 @@ error[E0308]: mismatched types
 13 |     Other(Vec<CString>)
    |               ^^^^^^^ expected struct `std::ffi::CString`, found struct `std::ffi::OsString`
    |
-   = note: expected type `std::vec::Vec<std::ffi::CString>`
-              found type `std::vec::Vec<std::ffi::OsString>`
+   = note: expected struct `std::vec::Vec<std::ffi::CString>`
+              found struct `std::vec::Vec<std::ffi::OsString>`

tests/ui/opt_opt_nonpositional.stderr

@@ -2,4 +2,4 @@ error: Option<Option<T>> type is meaningless for positional argument
   --> $DIR/opt_opt_nonpositional.rs:14:8
    |
 14 |     n: Option<Option<u32>>,
-   |        ^^^^^^
+   |        ^^^^^^^^^^^^^^^^^^^

tests/ui/opt_vec_nonpositional.stderr

@@ -2,4 +2,4 @@ error: Option<Vec<T>> type is meaningless for positional argument
   --> $DIR/opt_vec_nonpositional.rs:14:8
    |
 14 |     n: Option<Vec<u32>>,
-   |        ^^^^^^
+   |        ^^^^^^^^^^^^^^^^

tests/ui/parse_not_zero_args.stderr

@@ -1,4 +1,4 @@
-error: parse must have exactly one argument
+error: `parse` must have exactly one argument
   --> $DIR/parse_not_zero_args.rs:14:17
    |
 14 |     #[structopt(parse(from_str, from_str))]

tests/ui/structopt_empty_attr.stderr

@@ -2,4 +2,4 @@ error: expected attribute arguments in parentheses: #[structopt(...)]
   --> $DIR/structopt_empty_attr.rs:14:5
    |
 14 |     #[structopt]
-   |     ^
+   |     ^^^^^^^^^^^^

tests/ui/subcommand_opt_opt.stderr

@@ -2,4 +2,4 @@ error: Option<Option<T>> type is not allowed for subcommand
   --> $DIR/subcommand_opt_opt.rs:18:10
    |
 18 |     cmd: Option<Option<Command>>,
-   |          ^^^^^^
+   |          ^^^^^^^^^^^^^^^^^^^^^^^

tests/ui/subcommand_opt_vec.stderr

@@ -2,4 +2,4 @@ error: Option<Vec<T>> type is not allowed for subcommand
   --> $DIR/subcommand_opt_vec.rs:18:10
    |
 18 |     cmd: Option<Vec<Command>>,
-   |          ^^^^^^
+   |          ^^^^^^^^^^^^^^^^^^^^