Fix clippy lints; return errors rather than panic

This cleans up clippy lints, both in the library itself, and in the
generated code. As part of this change, it changes a few locations where
we would have panicked on values passed in by the user to instead return
an error.

However, I left one place that unwraps in the generated code. After we have
parsed all the arguments, we check that all of the required arguments. If
not, we return an error. It's unfortunately a little difficult to prove
to rust that we've handled all the arguments. I left the unwrap()s in place
in case there's a logic error we have in handling this, and annotated the
code with `#[allow(clippy::unwrap_in_result)]` to allow for this case.

Closes #114

Author: erickt

.github/workflows/rust.yml

@@ -15,3 +15,5 @@ jobs:
       run: cargo build --verbose
     - name: Run tests
       run: cargo test --verbose
+    - name: Run clippy
+      run: cargo clippy -- -D warnings

argh/src/lib.rs

@@ -31,9 +31,7 @@
 //!     pilot_nickname: Option<String>,
 //! }
 //!
-//! fn main() {
-//!     let up: GoUp = argh::from_env();
-//! }
+//! let up: GoUp = argh::from_env();
 //! ```
 //!
 //! `./some_bin --help` will then output the following:
@@ -481,8 +479,8 @@ impl From<String> for EarlyExit {
 }
 
 /// Extract the base cmd from a path
-fn cmd<'a>(default: &'a String, path: &'a String) -> &'a str {
-    std::path::Path::new(path).file_name().map(|s| s.to_str()).flatten().unwrap_or(default.as_str())
+fn cmd<'a>(default: &'a str, path: &'a str) -> &'a str {
+    std::path::Path::new(path).file_name().map(|s| s.to_str()).flatten().unwrap_or(default)
 }
 
 /// Create a `FromArgs` type from the current process's `env::args`.
@@ -690,7 +688,7 @@ pub fn parse_struct_args(
             continue;
         }
 
-        if next_arg.starts_with("-") && !options_ended {
+        if next_arg.starts_with('-') && !options_ended {
             if next_arg == "--" {
                 options_ended = true;
                 continue;
@@ -877,7 +875,7 @@ impl<'a> ParseStructSubCommand<'a> {
             }
         }
 
-        return Ok(false);
+        Ok(false)
     }
 }
 
@@ -951,7 +949,7 @@ impl MissingRequirements {
 
         if !self.options.is_empty() {
             if !self.positional_args.is_empty() {
-                output.push_str("\n");
+                output.push('\n');
             }
             output.push_str("Required options not provided:");
             for option in &self.options {
@@ -962,7 +960,7 @@ impl MissingRequirements {
 
         if let Some(missing_subcommands) = self.subcommands {
             if !self.options.is_empty() {
-                output.push_str("\n");
+                output.push('\n');
             }
             output.push_str("One of the following subcommands must be present:");
             output.push_str(NEWLINE_INDENT);

argh/tests/lib.rs

@@ -1,8 +1,16 @@
-#![cfg(test)]
 // Copyright (c) 2020 Google LLC All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+// Deny a bunch of uncommon clippy lints to make sure the generated code won't trigger a warning.
+#![deny(
+    clippy::indexing_slicing,
+    clippy::panic_in_result_fn,
+    clippy::str_to_string,
+    clippy::unreachable,
+    clippy::unwrap_in_result
+)]
+
 use {argh::FromArgs, std::fmt::Debug};
 
 #[test]
@@ -154,7 +162,7 @@ fn default_number() {
 fn default_function() {
     const MSG: &str = "hey I just met you";
     fn call_me_maybe() -> String {
-        MSG.to_string()
+        MSG.to_owned()
     }
 
     #[derive(FromArgs)]
@@ -821,7 +829,7 @@ Options:
             help_example,
             HelpExample {
                 force: true,
-                scribble: "fooey".to_string(),
+                scribble: "fooey".to_owned(),
                 really_really_really_long_name_for_pat: false,
                 verbose: false,
                 command: HelpExampleSubCommands::BlowUp(BlowUp { safely: true }),
@@ -1264,7 +1272,7 @@ Options:
   -n, --n           fooey
   --help            display usage information
 "###
-            .to_string(),
+            .to_owned(),
             status: Ok(()),
         }),
     );
@@ -1283,7 +1291,7 @@ fn redact_arg_values_produces_errors_with_bad_arguments() {
     assert_eq!(
         Cmd::redact_arg_values(&["program-name"], &["--n"]),
         Err(argh::EarlyExit {
-            output: "No value provided for option '--n'.\n".to_string(),
+            output: "No value provided for option '--n'.\n".to_owned(),
             status: Err(()),
         }),
     );

argh_derive/src/help.rs

@@ -96,7 +96,7 @@ pub(crate) fn help(
 
     lits_section(&mut format_lit, "Notes:", &ty_attrs.notes);
 
-    if ty_attrs.error_codes.len() != 0 {
+    if !ty_attrs.error_codes.is_empty() {
         format_lit.push_str(SECTION_SEPARATOR);
         format_lit.push_str("Error codes:");
         for (code, text) in &ty_attrs.error_codes {
@@ -106,7 +106,7 @@ pub(crate) fn help(
         }
     }
 
-    format_lit.push_str("\n");
+    format_lit.push('\n');
 
     quote! { {
         #subcommand_calculation
@@ -116,7 +116,7 @@ pub(crate) fn help(
 
 /// A section composed of exactly just the literals provided to the program.
 fn lits_section(out: &mut String, heading: &str, lits: &[syn::LitStr]) {
-    if lits.len() != 0 {
+    if !lits.is_empty() {
         out.push_str(SECTION_SEPARATOR);
         out.push_str(heading);
         for lit in lits {

argh_derive/src/lib.rs

@@ -35,7 +35,7 @@ pub fn argh_derive(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
 /// as well as all errors that occurred.
 fn impl_from_args(input: &syn::DeriveInput) -> TokenStream {
     let errors = &Errors::default();
-    if input.generics.params.len() != 0 {
+    if !input.generics.params.is_empty() {
         errors.err(
             &input.generics,
             "`#![derive(FromArgs)]` cannot be applied to types with generic parameters",
@@ -65,22 +65,15 @@ enum Optionality {
 impl PartialEq<Optionality> for Optionality {
     fn eq(&self, other: &Optionality) -> bool {
         use Optionality::*;
-        match (self, other) {
-            (None, None) | (Optional, Optional) | (Repeating, Repeating) => true,
-            // NB: (Defaulted, Defaulted) can't contain the same token streams
-            _ => false,
-        }
+        // NB: (Defaulted, Defaulted) can't contain the same token streams
+        matches!((self, other), (Optional, Optional) | (Repeating, Repeating))
     }
 }
 
 impl Optionality {
     /// Whether or not this is `Optionality::None`
     fn is_required(&self) -> bool {
-        if let Optionality::None = self {
-            true
-        } else {
-            false
-        }
+        matches!(self, Optionality::None)
     }
 }
 
@@ -252,6 +245,7 @@ fn impl_from_args_struct(
     let top_or_sub_cmd_impl = top_or_sub_cmd_impl(errors, name, type_attrs);
 
     let trait_impl = quote_spanned! { impl_span =>
+        #[automatically_derived]
         impl argh::FromArgs for #name {
             #from_args_method
 
@@ -269,8 +263,8 @@ fn impl_from_args_struct_from_args<'a>(
     type_attrs: &TypeAttrs,
     fields: &'a [StructField<'a>],
 ) -> TokenStream {
-    let init_fields = declare_local_storage_for_from_args_fields(&fields);
-    let unwrap_fields = unwrap_from_args_fields(&fields);
+    let init_fields = declare_local_storage_for_from_args_fields(fields);
+    let unwrap_fields = unwrap_from_args_fields(fields);
     let positional_fields: Vec<&StructField<'_>> =
         fields.iter().filter(|field| field.kind == FieldKind::Positional).collect();
     let positional_field_idents = positional_fields.iter().map(|field| &field.field.ident);
@@ -289,13 +283,13 @@ fn impl_from_args_struct_from_args<'a>(
         }
     });
 
-    let flag_str_to_output_table_map = flag_str_to_output_table_map_entries(&fields);
+    let flag_str_to_output_table_map = flag_str_to_output_table_map_entries(fields);
 
     let mut subcommands_iter =
         fields.iter().filter(|field| field.kind == FieldKind::SubCommand).fuse();
 
     let subcommand: Option<&StructField<'_>> = subcommands_iter.next();
-    while let Some(dup_subcommand) = subcommands_iter.next() {
+    for dup_subcommand in subcommands_iter {
         errors.duplicate_attrs("subcommand", subcommand.unwrap().field, dup_subcommand.field);
     }
 
@@ -304,7 +298,7 @@ fn impl_from_args_struct_from_args<'a>(
     let missing_requirements_ident = syn::Ident::new("__missing_requirements", impl_span);
 
     let append_missing_requirements =
-        append_missing_requirements(&missing_requirements_ident, &fields);
+        append_missing_requirements(&missing_requirements_ident, fields);
 
     let parse_subcommands = if let Some(subcommand) = subcommand {
         let name = subcommand.name;
@@ -324,12 +318,14 @@ fn impl_from_args_struct_from_args<'a>(
 
     // Identifier referring to a value containing the name of the current command as an `&[&str]`.
     let cmd_name_str_array_ident = syn::Ident::new("__cmd_name", impl_span);
-    let help = help::help(errors, cmd_name_str_array_ident, type_attrs, &fields, subcommand);
+    let help = help::help(errors, cmd_name_str_array_ident, type_attrs, fields, subcommand);
 
     let method_impl = quote_spanned! { impl_span =>
         fn from_args(__cmd_name: &[&str], __args: &[&str])
             -> std::result::Result<Self, argh::EarlyExit>
         {
+            #![allow(clippy::unwrap_in_result)]
+
             #( #init_fields )*
 
             argh::parse_struct_args(
@@ -374,8 +370,8 @@ fn impl_from_args_struct_redact_arg_values<'a>(
     type_attrs: &TypeAttrs,
     fields: &'a [StructField<'a>],
 ) -> TokenStream {
-    let init_fields = declare_local_storage_for_redacted_fields(&fields);
-    let unwrap_fields = unwrap_redacted_fields(&fields);
+    let init_fields = declare_local_storage_for_redacted_fields(fields);
+    let unwrap_fields = unwrap_redacted_fields(fields);
 
     let positional_fields: Vec<&StructField<'_>> =
         fields.iter().filter(|field| field.kind == FieldKind::Positional).collect();
@@ -395,13 +391,13 @@ fn impl_from_args_struct_redact_arg_values<'a>(
         }
     });
 
-    let flag_str_to_output_table_map = flag_str_to_output_table_map_entries(&fields);
+    let flag_str_to_output_table_map = flag_str_to_output_table_map_entries(fields);
 
     let mut subcommands_iter =
         fields.iter().filter(|field| field.kind == FieldKind::SubCommand).fuse();
 
     let subcommand: Option<&StructField<'_>> = subcommands_iter.next();
-    while let Some(dup_subcommand) = subcommands_iter.next() {
+    for dup_subcommand in subcommands_iter {
         errors.duplicate_attrs("subcommand", subcommand.unwrap().field, dup_subcommand.field);
     }
 
@@ -410,7 +406,7 @@ fn impl_from_args_struct_redact_arg_values<'a>(
     let missing_requirements_ident = syn::Ident::new("__missing_requirements", impl_span);
 
     let append_missing_requirements =
-        append_missing_requirements(&missing_requirements_ident, &fields);
+        append_missing_requirements(&missing_requirements_ident, fields);
 
     let redact_subcommands = if let Some(subcommand) = subcommand {
         let name = subcommand.name;
@@ -428,15 +424,15 @@ fn impl_from_args_struct_redact_arg_values<'a>(
         quote_spanned! { impl_span => None }
     };
 
-    let cmd_name = if type_attrs.is_subcommand.is_none() {
-        quote! { __cmd_name.last().expect("no command name").to_string() }
+    let unwrap_cmd_name_err_string = if type_attrs.is_subcommand.is_none() {
+        quote! { "no command name" }
     } else {
-        quote! { __cmd_name.last().expect("no subcommand name").to_string() }
+        quote! { "no subcommand name" }
     };
 
     // Identifier referring to a value containing the name of the current command as an `&[&str]`.
     let cmd_name_str_array_ident = syn::Ident::new("__cmd_name", impl_span);
-    let help = help::help(errors, cmd_name_str_array_ident, type_attrs, &fields, subcommand);
+    let help = help::help(errors, cmd_name_str_array_ident, type_attrs, fields, subcommand);
 
     let method_impl = quote_spanned! { impl_span =>
         fn redact_arg_values(__cmd_name: &[&str], __args: &[&str]) -> std::result::Result<Vec<String>, argh::EarlyExit> {
@@ -471,7 +467,11 @@ fn impl_from_args_struct_redact_arg_values<'a>(
             #missing_requirements_ident.err_on_any()?;
 
             let mut __redacted = vec![
-                #cmd_name,
+                if let Some(cmd_name) = __cmd_name.last() {
+                    (*cmd_name).to_owned()
+                } else {
+                    return Err(argh::EarlyExit::from(#unwrap_cmd_name_err_string.to_owned()));
+                }
             ];
 
             #( #unwrap_fields )*
@@ -510,6 +510,7 @@ fn top_or_sub_cmd_impl(errors: &Errors, name: &syn::Ident, type_attrs: &TypeAttr
     if type_attrs.is_subcommand.is_none() {
         // Not a subcommand
         quote! {
+            #[automatically_derived]
             impl argh::TopLevelCommand for #name {}
         }
     } else {
@@ -519,6 +520,7 @@ fn top_or_sub_cmd_impl(errors: &Errors, name: &syn::Ident, type_attrs: &TypeAttr
             &empty_str
         });
         quote! {
+            #[automatically_derived]
             impl argh::SubCommand for #name {
                 const COMMAND: &'static argh::CommandInfo = &argh::CommandInfo {
                     name: #subcommand_name,
@@ -586,7 +588,9 @@ fn unwrap_from_args_fields<'a>(
         let field_name = field.name;
         match field.kind {
             FieldKind::Option | FieldKind::Positional => match &field.optionality {
-                Optionality::None => quote! { #field_name: #field_name.slot.unwrap() },
+                Optionality::None => quote! {
+                    #field_name: #field_name.slot.unwrap()
+                },
                 Optionality::Optional | Optionality::Repeating => {
                     quote! { #field_name: #field_name.slot }
                 }
@@ -639,7 +643,7 @@ fn declare_local_storage_for_redacted_fields<'a>(
                     let mut #field_name: argh::ParseValueSlotTy::<#field_slot_type, String> =
                         argh::ParseValueSlotTy {
                         slot: std::default::Default::default(),
-                        parse_func: |arg, _| { Ok(arg.to_string()) },
+                        parse_func: |arg, _| { Ok(arg.to_owned()) },
                     };
                 }
             }
@@ -658,7 +662,7 @@ fn declare_local_storage_for_redacted_fields<'a>(
                     let mut #field_name: argh::ParseValueSlotTy::<#field_slot_type, String> =
                         argh::ParseValueSlotTy {
                         slot: std::default::Default::default(),
-                        parse_func: |_, _| { Ok(#arg_name.to_string()) },
+                        parse_func: |_, _| { Ok(#arg_name.to_owned()) },
                     };
                 }
             }
@@ -861,25 +865,37 @@ fn impl_from_args_enum(
             fn from_args(command_name: &[&str], args: &[&str])
                 -> std::result::Result<Self, argh::EarlyExit>
             {
-                let subcommand_name = *command_name.last().expect("no subcommand name");
+                let subcommand_name = if let Some(subcommand_name) = command_name.last() {
+                    *subcommand_name
+                } else {
+                    return Err(argh::EarlyExit::from("no subcommand name".to_owned()));
+                };
+
                 #(
                     if subcommand_name == <#variant_ty as argh::SubCommand>::COMMAND.name {
                         return Ok(#name_repeating::#variant_names(
                             <#variant_ty as argh::FromArgs>::from_args(command_name, args)?
                         ));
                     }
                 )*
-                unreachable!("no subcommand matched")
+
+                Err(argh::EarlyExit::from("no subcommand matched".to_owned()))
             }
 
             fn redact_arg_values(command_name: &[&str], args: &[&str]) -> std::result::Result<Vec<String>, argh::EarlyExit> {
-                let subcommand_name = *command_name.last().expect("no subcommand name");
+                let subcommand_name = if let Some(subcommand_name) = command_name.last() {
+                    *subcommand_name
+                } else {
+                    return Err(argh::EarlyExit::from("no subcommand name".to_owned()));
+                };
+
                 #(
                     if subcommand_name == <#variant_ty as argh::SubCommand>::COMMAND.name {
                         return <#variant_ty as argh::FromArgs>::redact_arg_values(command_name, args);
                     }
                 )*
-                unreachable!("no subcommand matched")
+
+                Err(argh::EarlyExit::from("no subcommand matched".to_owned()))
             }
         }