Make TryFrom and FromStr infallible if there's a default (#476)

Author: Peternator7

strum_macros/src/lib.rs

@@ -34,15 +34,15 @@ fn debug_print_generated(ast: &DeriveInput, toks: &TokenStream) {
 
 /// Converts strings to enum variants based on their name.
 ///
-/// auto-derives `std::str::FromStr` on the enum (for Rust 1.34 and above, `std::convert::TryFrom<&str>`
-/// will be derived as well). Each variant of the enum will match on it's own name.
-/// This can be overridden using `serialize="DifferentName"` or `to_string="DifferentName"`
+/// auto-derives `std::str::FromStr` on the enum. Each variant of the enum will match on its own
+/// name. This can be overridden using `serialize="DifferentName"` or `to_string="DifferentName"`
 /// on the attribute as shown below.
-/// Multiple deserializations can be added to the same variant. If the variant contains additional data,
-/// they will be set to their default values upon deserialization.
+/// Multiple deserializations can be added to the same variant. If the variant contains additional
+/// data, they will be set to their default values upon deserialization.
 ///
-/// The `default` attribute can be applied to a tuple variant with a single data parameter. When a match isn't
-/// found, the given variant will be returned and the input string will be captured in the parameter.
+/// The `default` attribute can be applied to a tuple variant with a single data parameter. When a
+/// match isn't found, the given variant will be returned and the input string will be captured in
+/// the parameter.
 ///
 /// Note that the implementation of `FromStr` by default only matches on the name of the
 /// variant. There is an option to match on different case conversions through the
@@ -57,15 +57,20 @@ fn debug_print_generated(ast: &DeriveInput, toks: &TokenStream) {
 /// rather than just assume it will be faster. With SIMD + pipelining, linear string search (aka memcmp)
 /// can be very fast for enums with a surprisingly large number of enum variants.
 ///
-/// The default error type is `strum::ParseError`. This can be overriden by applying both the
-/// `parse_err_ty` and `parse_err_fn` attributes at the type level.  `parse_err_fn` should be a
+/// # Infallible Parsing
+///
+/// If the enum has a `#[strum(default)]` variant and no `parse_err_ty` is set, parsing is
+/// infallible: `From<&str>` is derived instead of `TryFrom<&str>`, which allows calling
+/// `MyEnum::from("string")` directly.
+///
+/// # Custom Error Types
+///
+/// The default error type is `strum::ParseError`. This can be overridden by applying both the
+/// `parse_err_ty` and `parse_err_fn` attributes at the type level. `parse_err_fn` should be a
 /// function that accepts an `&str` and returns the type `parse_err_ty`. See [this test
 /// case](https://github.com/Peternator7/strum/blob/9db3c4dc9b6f585aeb9f5f15f9cc18b6cf4fd780/strum_tests/tests/from_str.rs#L233)
-/// for an example.
-///
-/// If the enum has a default variant (annotated with `#[strum(default)]`), then parsing is
-/// infallible. In that case, `parse_err_fn` need not exist (it will never be called) and
-/// `parse_err_ty` can be safely set to [`std::convert::Infallible`].
+/// for an example. When `parse_err_ty` is set, `TryFrom<&str>` is always derived, even if the
+/// enum has a `#[strum(default)]` variant.
 ///
 /// # Example how to use `EnumString`
 /// ```

strum_macros/src/macros/strings/from_string.rs

@@ -1,6 +1,6 @@
 use proc_macro2::TokenStream;
 use quote::quote;
-use syn::{parse_quote, Data, DeriveInput, Fields, Path};
+use syn::{Data, DeriveInput, Fields};
 
 use crate::helpers::{
     missing_parse_err_attr_error, non_enum_error, occurrence_error, HasInnerVariantProperties,
@@ -18,26 +18,14 @@ pub fn from_string_inner(ast: &DeriveInput) -> syn::Result<TokenStream> {
     let type_properties = ast.get_type_properties()?;
     let strum_module_path = type_properties.crate_module_path();
 
+    // It's an error to provide an err_fn but not an err_ty.
+    if type_properties.parse_err_fn.is_some() && type_properties.parse_err_ty.is_none() {
+        return Err(missing_parse_err_attr_error());
+    }
+
     let mut default_kw = None;
-    let (default_err_ty, mut default_match_arm) = match (
-        type_properties.parse_err_ty,
-        type_properties.parse_err_fn,
-    ) {
-        (None, None) => (
-            quote! { #strum_module_path::ParseError },
-            quote! { ::core::result::Result::Err(#strum_module_path::ParseError::VariantNotFound) },
-        ),
-        (Some(ty), Some(f)) => {
-            let ty_path: Path = parse_quote!(#ty);
-            let fn_path: Path = parse_quote!(#f);
-
-            (
-                quote! { #ty_path },
-                quote! { ::core::result::Result::Err(#fn_path(s)) },
-            )
-        }
-        _ => return Err(missing_parse_err_attr_error()),
-    };
+    let mut default_match_arm = None;
+
     let mut phf_exact_match_arms = Vec::new();
     let mut standard_match_arms = Vec::new();
     for variant in variants {
@@ -57,15 +45,13 @@ pub fn from_string_inner(ast: &DeriveInput) -> syn::Result<TokenStream> {
 
             match &variant.fields {
                 Fields::Unnamed(fields) if fields.unnamed.len() == 1 => {
-                    default_match_arm = quote! {
-                        ::core::result::Result::Ok(#name::#ident(s.into()))
-                    };
+                    default_match_arm = Some(quote! {
+                        #name::#ident(s.into())
+                    });
                 }
                 Fields::Named(ref f) if f.named.len() == 1 => {
                     let field_name = f.named.last().unwrap().ident.as_ref().unwrap();
-                    default_match_arm = quote! {
-                        ::core::result::Result::Ok(#name::#ident { #field_name : s.into() } )
-                    };
+                    default_match_arm = Some(quote! { #name::#ident { #field_name : s.into() } });
                 }
                 _ => {
                     return Err(syn::Error::new_spanned(
@@ -133,85 +119,109 @@ pub fn from_string_inner(ast: &DeriveInput) -> syn::Result<TokenStream> {
                     phf_exact_match_arms.push(quote! { #upper => #name::#ident #params, });
                     standard_match_arms.push(quote! { s if s.eq_ignore_ascii_case(#serialization) => #name::#ident #params, });
                 }
+            } else if !is_ascii_case_insensitive {
+                standard_match_arms.push(quote! { #serialization => #name::#ident #params, });
             } else {
-                standard_match_arms.push(if !is_ascii_case_insensitive {
-                    quote! { #serialization => #name::#ident #params, }
-                } else {
-                    quote! { s if s.eq_ignore_ascii_case(#serialization) => #name::#ident #params, }
-                });
+                standard_match_arms.push(quote! { s if s.eq_ignore_ascii_case(#serialization) => #name::#ident #params, });
             }
         }
     }
 
-    let phf_body = if phf_exact_match_arms.is_empty() {
-        quote!()
+    // Determine the error type on FromStr and TryFrom based on what the user
+    // has configured and whether there is a default variant.
+    let is_infallible = default_match_arm.is_some();
+    let has_custom_err_ty = type_properties.parse_err_ty.is_some();
+    let err_ty = if let Some(ty) = type_properties.parse_err_ty {
+        quote! { #ty }
+    } else if is_infallible {
+        quote! { ::core::convert::Infallible }
+    } else {
+        quote! { #strum_module_path::ParseError }
+    };
+
+    // Determine the default match arm behavior based on whether the user provided a "default"
+    // or if the user provided a custom error function.
+    let default_match_arm = if let Some(default_match_arm) = default_match_arm {
+        default_match_arm
+    } else if let Some(f) = type_properties.parse_err_fn {
+        quote! { return ::core::result::Result::Err(#f(s)) }
+    } else if has_custom_err_ty {
+        // The user defined a custom error type, but not a custom error function. This is an error
+        // if the method isn't infallible.
+        return Err(missing_parse_err_attr_error());
+    } else {
+        quote! { return ::core::result::Result::Err(#strum_module_path::ParseError::VariantNotFound) }
+    };
+
+    let mut match_expression = if standard_match_arms.is_empty() {
+        default_match_arm
     } else {
         quote! {
+            match s {
+                #(#standard_match_arms)*
+                _ => #default_match_arm,
+            }
+        }
+    };
+
+    if !phf_exact_match_arms.is_empty() {
+        match_expression = quote! {
             use #strum_module_path::_private_phf_reexport_for_macro_if_phf_feature as phf;
             static PHF: phf::Map<&'static str, #name> = phf::phf_map! {
                 #(#phf_exact_match_arms)*
             };
+
             if let Some(value) = PHF.get(s).cloned() {
-                return ::core::result::Result::Ok(value);
+                value
+            } else {
+                #match_expression
             }
         }
-    };
+    }
 
-    let standard_match_body = if standard_match_arms.is_empty() {
-        default_match_arm
+    let from_impl = if is_infallible && !has_custom_err_ty {
+        quote! {
+            #[allow(clippy::use_self)]
+            #[automatically_derived]
+            impl #impl_generics ::core::convert::From<&str> for #name #ty_generics #where_clause {
+                #[inline]
+                fn from(s: &str) -> #name #ty_generics {
+                    #match_expression
+                }
+            }
+        }
     } else {
         quote! {
-            ::core::result::Result::Ok(match s {
-                #(#standard_match_arms)*
-                _ => return #default_match_arm,
-            })
+            #[allow(clippy::use_self)]
+            #[automatically_derived]
+            impl #impl_generics ::core::convert::TryFrom<&str> for #name #ty_generics #where_clause {
+                type Error = #err_ty;
+
+                #[inline]
+                fn try_from(s: &str) -> ::core::result::Result< #name #ty_generics , <Self as ::core::convert::TryFrom<&str>>::Error> {
+                    Ok({
+                        #match_expression
+                    })
+                }
+            }
         }
     };
 
     let from_str = quote! {
         #[allow(clippy::use_self)]
         #[automatically_derived]
         impl #impl_generics ::core::str::FromStr for #name #ty_generics #where_clause {
-            type Err = #default_err_ty;
+            type Err = #err_ty;
 
             #[inline]
             fn from_str(s: &str) -> ::core::result::Result< #name #ty_generics , <Self as ::core::str::FromStr>::Err> {
-                #phf_body
-                #standard_match_body
+                <Self as ::core::convert::TryFrom<&str>>::try_from(s)
             }
         }
     };
-    let try_from_str = try_from_str(
-        name,
-        &impl_generics,
-        &ty_generics,
-        where_clause,
-        &default_err_ty,
-    );
 
     Ok(quote! {
         #from_str
-        #try_from_str
+        #from_impl
     })
 }
-
-fn try_from_str(
-    name: &proc_macro2::Ident,
-    impl_generics: &syn::ImplGenerics,
-    ty_generics: &syn::TypeGenerics,
-    where_clause: Option<&syn::WhereClause>,
-    default_err_ty: &TokenStream,
-) -> TokenStream {
-    quote! {
-        #[allow(clippy::use_self)]
-        #[automatically_derived]
-        impl #impl_generics ::core::convert::TryFrom<&str> for #name #ty_generics #where_clause {
-            type Error = #default_err_ty;
-
-            #[inline]
-            fn try_from(s: &str) -> ::core::result::Result< #name #ty_generics , <Self as ::core::convert::TryFrom<&str>>::Error> {
-                ::core::str::FromStr::from_str(s)
-            }
-        }
-    }
-}

strum_tests/src/lib.rs

@@ -12,6 +12,8 @@ pub enum Color {
     Yellow,
     #[strum(disabled)]
     Green(String),
+    #[strum(default)]
+    Purple(String),
 }
 
 /// A bunch of errors

strum_tests/tests/from_str.rs

@@ -1,4 +1,6 @@
-use std::str::FromStr;
+#![allow(clippy::infallible_try_from)]
+
+use std::{convert::Infallible, str::FromStr};
 use strum::EnumString;
 
 mod core {} // ensure macros call `::core`
@@ -70,6 +72,16 @@ fn color_default() {
     assert_from_str(Color::Green(String::from("not found")), "not found");
 }
 
+#[test]
+#[allow(clippy::unnecessary_fallible_conversions)]
+fn color2_infallible() {
+    let r: Result<Color2, Infallible> = Color2::from_str("infallible");
+    assert!(r.is_ok());
+    let r: Result<Color2, Infallible> = Color2::try_from("infallible");
+    assert!(r.is_ok());
+    let _ = Color2::from("infallible");
+}
+
 #[test]
 fn color2_default() {
     assert_from_str(
@@ -300,3 +312,26 @@ fn case_custom_infallible_parsing_with_default() {
         r
     );
 }
+
+enum Never {}
+
+#[derive(Debug, EnumString, Eq, PartialEq)]
+#[strum(
+    parse_err_ty = Never
+)]
+enum CustomErrorTyWithNoErrorFn {
+    #[strum(serialize = "foo")]
+    Foo,
+    #[strum(serialize = "bar")]
+    Bar,
+    #[strum(default)]
+    Unknown(String),
+}
+
+#[test]
+fn case_custom_infallible_parsing_with_default_no_err_fn() {
+    let r: Result<CustomErrorTyWithNoErrorFn, Never> =
+        "yellow".parse::<CustomErrorTyWithNoErrorFn>();
+
+    assert!(r.is_ok());
+}

strum_tests/tests/phf.rs

@@ -14,6 +14,48 @@ fn from_str_with_phf() {
     assert_eq!("bLuE".parse::<Color>().unwrap(), Color::Blue);
 }
 
+#[cfg(feature = "test_phf")]
+#[test]
+fn from_str_with_phf_infallible() {
+    #[derive(Debug, PartialEq, Eq, Clone, strum::EnumString)]
+    #[strum(use_phf)]
+    enum Color {
+        Red,
+        Blue,
+        #[strum(default)]
+        Unknown(String),
+    }
+
+    // Known variants still parse correctly
+    let c: Color = Color::from("Red");
+    assert_eq!(c, Color::Red);
+    let c: Color = Color::from("Blue");
+    assert_eq!(c, Color::Blue);
+
+    // Unknown input falls through to the default variant
+    let c: Color = Color::from("notacolor");
+    assert_eq!(c, Color::Unknown("notacolor".to_string()));
+}
+
+#[cfg(feature = "test_phf")]
+#[test]
+fn from_str_with_phf_infallible_case_insensitive() {
+    #[derive(Debug, PartialEq, Eq, Clone, strum::EnumString)]
+    #[strum(use_phf)]
+    enum Color {
+        #[strum(ascii_case_insensitive)]
+        Blue,
+        Red,
+        #[strum(default)]
+        Unknown(String),
+    }
+
+    let c: Color = Color::from("bLuE");
+    assert_eq!(c, Color::Blue);
+    let c: Color = Color::from("notacolor");
+    assert_eq!(c, Color::Unknown("notacolor".to_string()));
+}
+
 #[cfg(feature = "test_phf")]
 #[test]
 fn from_str_with_phf_big() {