Repository: google/argh (language: rust)

Issue:
Clippy warnings
Derived code is triggering Clippy warnings, more specifically:

* clippy::indexing_slicing,
* clippy::panic_in_result_fn,
* clippy::str_to_string,
* clippy::unreachable,
* clippy::unwrap_in_result

Please consider using `#[automatically_derived]`/`#[allow(clippy::SOME_LINT]` or audit the codebase.


Discussion / hints:

Fix patch:
diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index d307088..02b22a7 100644
--- a/.github/workflows/rust.yml
+++ b/.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
diff --git a/argh/src/lib.rs b/argh/src/lib.rs
index 984d927..1bd6ba9 100644
--- a/argh/src/lib.rs
+++ b/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);
diff --git a/argh_derive/src/help.rs b/argh_derive/src/help.rs
index 5bf02b1..c295825 100644
--- a/argh_derive/src/help.rs
+++ b/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 {
diff --git a/argh_derive/src/lib.rs b/argh_derive/src/lib.rs
index a123d4e..ab72a40 100644
--- a/argh_derive/src/lib.rs
+++ b/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,7 +865,12 @@ 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(
@@ -869,17 +878,24 @@ fn impl_from_args_enum(
                         ));
                     }
                 )*
-                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()))
             }
         }
 
diff --git a/argh_derive/src/parse_attrs.rs b/argh_derive/src/parse_attrs.rs
index 83807ed..33e18b5 100644
--- a/argh_derive/src/parse_attrs.rs
+++ b/argh_derive/src/parse_attrs.rs
@@ -79,29 +79,29 @@ impl FieldAttrs {
 
                 let name = meta.path();
                 if name.is_ident("arg_name") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_arg_name(errors, m);
                     }
                 } else if name.is_ident("default") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_default(errors, m);
                     }
                 } else if name.is_ident("description") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         parse_attr_description(errors, m, &mut this.description);
                     }
                 } else if name.is_ident("from_str_fn") {
-                    if let Some(m) = errors.expect_meta_list(&meta) {
+                    if let Some(m) = errors.expect_meta_list(meta) {
                         this.parse_attr_from_str_fn(errors, m);
                     }
                 } else if name.is_ident("long") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_long(errors, m);
                     }
                 } else if name.is_ident("option") {
                     parse_attr_field_type(errors, meta, FieldKind::Option, &mut this.field_type);
                 } else if name.is_ident("short") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_short(errors, m);
                     }
                 } else if name.is_ident("subcommand") {
@@ -218,10 +218,8 @@ fn parse_attr_field_type(
     if let Some(path) = errors.expect_meta_word(meta) {
         if let Some(first) = slot {
             errors.duplicate_attrs("field kind", &first.ident, path);
-        } else {
-            if let Some(word) = path.get_ident() {
-                *slot = Some(FieldType { kind, ident: word.clone() });
-            }
+        } else if let Some(word) = path.get_ident() {
+            *slot = Some(FieldType { kind, ident: word.clone() });
         }
     }
 }
@@ -304,28 +302,27 @@ impl TypeAttrs {
 
                 let name = meta.path();
                 if name.is_ident("description") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         parse_attr_description(errors, m, &mut this.description);
                     }
                 } else if name.is_ident("error_code") {
-                    if let Some(m) = errors.expect_meta_list(&meta) {
+                    if let Some(m) = errors.expect_meta_list(meta) {
                         this.parse_attr_error_code(errors, m);
                     }
                 } else if name.is_ident("example") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_example(errors, m);
                     }
                 } else if name.is_ident("name") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_name(errors, m);
                     }
                 } else if name.is_ident("note") {
-                    if let Some(m) = errors.expect_meta_name_value(&meta) {
+                    if let Some(m) = errors.expect_meta_name_value(meta) {
                         this.parse_attr_note(errors, m);
                     }
                 } else if name.is_ident("subcommand") {
-                    if let Some(ident) = errors.expect_meta_word(&meta).and_then(|p| p.get_ident())
-                    {
+                    if let Some(ident) = errors.expect_meta_word(meta).and_then(|p| p.get_ident()) {
                         this.parse_attr_subcommand(errors, ident);
                     }
                 } else {
@@ -498,7 +495,7 @@ pub fn check_enum_type_attrs(errors: &Errors, type_attrs: &TypeAttrs, type_span:
     // Ensure that `#[argh(subcommand)]` is present.
     if is_subcommand.is_none() {
         errors.err_span(
-            type_span.clone(),
+            *type_span,
             concat!(
                 "`#![derive(FromArgs)]` on `enum`s can only be used to enumerate subcommands.\n",
                 "Consider adding `#[argh(subcommand)]` to the `enum` declaration.",


Test patch:
diff --git a/argh/tests/lib.rs b/argh/tests/lib.rs
index fe8c858..e01e6d0 100644
--- a/argh/tests/lib.rs
+++ b/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(()),
         }),
     );
@@ -1306,3 +1314,58 @@ fn redact_arg_values_does_not_warn_if_used() {
     let actual = Cmd::redact_arg_values(&["program-name"], &["5"]).unwrap();
     assert_eq!(actual, &["program-name", "speed"]);
 }
+
+#[test]
+fn subcommand_does_not_panic() {
+    #[derive(FromArgs, PartialEq, Debug)]
+    #[argh(subcommand)]
+    enum SubCommandEnum {
+        Cmd(SubCommand),
+    }
+
+    #[derive(FromArgs, PartialEq, Debug)]
+    /// First subcommand.
+    #[argh(subcommand, name = "one")]
+    struct SubCommand {
+        #[argh(positional)]
+        /// how many x
+        x: usize,
+    }
+
+    #[derive(FromArgs, PartialEq, Debug)]
+    /// Second subcommand.
+    #[argh(subcommand, name = "two")]
+    struct SubCommandTwo {
+        #[argh(switch)]
+        /// whether to fooey
+        fooey: bool,
+    }
+
+    // Passing no subcommand name to an emum
+    assert_eq!(
+        SubCommandEnum::from_args(&[], &["5"]).unwrap_err(),
+        argh::EarlyExit { output: "no subcommand name".into(), status: Err(()) },
+    );
+
+    assert_eq!(
+        SubCommandEnum::redact_arg_values(&[], &["5"]).unwrap_err(),
+        argh::EarlyExit { output: "no subcommand name".into(), status: Err(()) },
+    );
+
+    // Passing unknown subcommand name to an emum
+    assert_eq!(
+        SubCommandEnum::from_args(&["fooey"], &["5"]).unwrap_err(),
+        argh::EarlyExit { output: "no subcommand matched".into(), status: Err(()) },
+    );
+
+    assert_eq!(
+        SubCommandEnum::redact_arg_values(&["fooey"], &["5"]).unwrap_err(),
+        argh::EarlyExit { output: "no subcommand matched".into(), status: Err(()) },
+    );
+
+    // Passing unknown subcommand name to a struct
+    assert_eq!(
+        SubCommand::redact_arg_values(&[], &["5"]).unwrap_err(),
+        argh::EarlyExit { output: "no subcommand name".into(), status: Err(()) },
+    );
+}