Support for custom error type in TryInto derive

[kiendang]

Resolves #396

Synopsis

Add support for specifying a custom error type for TryInto derive via a #[try_into(error(error_ty[, error_fn]))] attribute similarly to #494 for FromStr.

However there are a couple of issues that need clarifying

1. TryInto derive supports [#try_into(owned, ref, ref_mut)] attributes which are handled by State::with_attr_params. To add support for the error attribute, I took the approach that requires less amount of code change: find the attributes in input.attrs that follow the [#try_into(error pattern, parse them with attr::Error for the custom error support. These attributes are also taken out of input.attrs before the State::with_attr_params call, leaving the rest of the TryInto derive code unchanged. This works. However, this leads to the error message being unhelpful in cases like this

   #[try_into(owned, ref, ref_mut)]
   #[try_into(asdf)] // some invalid attribute
   ^Only a single attribute is allowed

   "Only 1 [#try_into(owned, ref, ref_mut)] and 1 #[try_into(error(...))] attribute allowed" would be more helpful.

2. The PR currently works if the custom error is non-generic, e.g struct CustomError(String) (see the added test for a working example). However, in this case

   struct CustomError<T>(TryIntoError<T>)

   there is a problem with specifying the type for ref and ref_mut.

   struct CustomError<T>(TryIntoError<T>);
   
   impl<'a, T> From<TryIntoError<&'a T>> for CustomError<&'a T> {
       fn from(value: TryIntoError<&'a T>) -> Self {
           Self(value)
       }
   }
   
   #[try_into(ref)]
   #[try_into(error(CustomError<&MixedInts>))]
                                ^this lifetime must come from the implemented type
                                ^expected named lifetime parameter
   enum MixedInts {
       SmallInt(i32),
       NamedBigInt {
           int: i64,
       },
   }

   Another thing is suppose we want to derive for all 3 owned, ref and ref_mut, the custom error types might be different for each of them. The syntax can be extended to #[try_into(error(owned(...), ref(...), ref_mut(...)))] to accommodate this, but the issue above needs solving first.

Solution

Checklist

• Documentation is updated (if required)
• Tests are added/updated (if required)
• CHANGELOG entry is added (if required)

[tyranron]

@kiendang

1. TryInto derive supports [#try_into(owned, ref, ref_mut)] attributes which are handled by State::with_attr_params.

This works. However, this leads to the error message being unhelpful in cases like this

A very dirty hack could be used after parsing State::with_attr_params(), like: checking the returned error message and putting the better one. Covering this with test will ensure no regression is happened if the error from State::with_attr_params() is changed accidentally.

Not an optimal solution, of course, but does the job, until handling owned/ref/ref_mut is refactored with the new utils::attr machinery.

2. The PR currently works if the custom error is non-generic

I think this could easily be supported like this:

struct CustomError<T>(TryIntoError<T>);

impl<T> From<TryIntoError<T>> for CustomError<T> {
    fn from(value: TryIntoError<T>) -> Self {
        Self(value)
    }
}

impl<T> CustomError<T> {
    fn new(err: TryIntoError<T>) -> Self {
        Self(err)
    }
}

fn into_custom_err<T>(err: TryIntoError<T>) -> CustomError<T> {
    err.into()
}

#[derive(TryInto)]
#[try_into(ref)]
#[try_into(error::<T>(CustomError<T>))]
// or
#[try_into(error::<T>(CustomError<T>, CustomError<T>::new))]
// or
#[try_into(error::<T>(CustomError<T>, into_custom_err<T>))]
enum MixedInts {
    SmallInt(i32),
    NamedBigInt { int: i64 },
}

where T, of course, would be substituted with the appropriate type, like it's done for derive_more::TryIntoError in the expansion.

This requires a slight extension of the attr::Error machinery. Note, that this behavior should be controlled, in a way that error::<T> is not allowed for derive(FromStr) at all. Maybe, parametrizing attr::Error with behaviour marker will do the trick:

pub(crate) struct Error<const GENERICS: bool = false> {
    // ...
}

/// [`Error`] attribute with type parameters support (e.g. `error::<T>(...)`).
pub(crate) type ErrorGeneric = Error<true>;

And so, derive(TryInto) will use attr::ErrorGeneric, while derive(FromStr) will continue to use attr::Error which disallows using error::<T> syntax.

[kiendang]

Thanks for the detailed response @tyranron!

A very dirty hack could be used after parsing State::with_attr_params(), like: checking the returned error message and putting the better one. Covering this with test will ensure no regression is happened if the error from State::with_attr_params() is changed accidentally.

Ah yes this would work.

I think this could easily be supported like this:

This would work. My concern is if this syntax is too specific to this use case. I'm not sure how useful or common this use case of having a custom error parametrized on T is. In the original issue #396, the user just wants to have a &'static str error (making output_types and variant_names public member of TryIntoError like Jelte suggested would be useful here. We can add that). Another thing is the syntax is a bit confusing. Looking at

#[try_into(error::<T>(CustomError<T>, into_custom_err<T>))]
enum MixedInts {
    SmallInt(i32),
    NamedBigInt { int: i64 },
}

it's a bit unclear what T means. It's not immediately obvious that T refers to MixedInts and is related to TryIntoError<T>.

[kiendang]

We don't even have to support that use case now, though. We can support only non-generic error type now. Extending the syntax to #[try_into(error(owned(...), ref(...), ref_mut(...)))] and/or #[try_into(error::<T>(CustomError<T>))] is a backward-compatible change anw.

[tyranron]

@kiendang

My concern is if this syntax is too specific to this use case. I'm not sure how useful or common this use case of having a custom error parametrized on T is.

Well, the point is:

• If you don't define your error like this struct CustomError<T>(TryIntoError<T>);, you don't need to use this turbofish ::<T> parametrization and just #[try_into(error(CustomError))] does the job.
• If, however, you do want to preserve the original value in the error, then you should specify the parametrization for the macro to understand which parameter should be substituted. Usually for<T> is used for such cases, like #[try_into(error(for<T> CustomError<T>))]. However, we have to attribute arguments here, and the custom conversion function will need to specify parametrization too very much likely, which requires repeating that for<T> twice (e.g. #[try_into(error(for<T> CustomError<T>, for<T> CustomError<T>::new))]). So, I thought turbofish would be more ergonomic in this case, defining the parameter for all arguments, like Rust does in functions. Maybe, instead turbofish ::<T>, just <T> would be enough: #[try_into(error<T>(CustomError<T>, CustomError<T>::new))]? I'm open to better syntactic suggestions, of course.

So, overall, you don't use weird syntax if you don't have to.

it's a bit unclear what T means. It's not immediately obvious that T refers to MixedInts and is related to TryIntoError<T>.

That's what documentation is for!

We don't even have to support that use case now, though.

I'm OK with not supporting it at all! Only giving suggestions how we could solve this if we want to!
Though, this caveat should be explicitly stated in docs.

[kiendang]

I'm OK with not supporting it at all! Only giving suggestions how we could solve this if we want to!
Though, this caveat should be explicitly stated in docs.

Yup I agree. Let's support just the simple non-generic case for this PR.

Still, I gave much thought to the syntax for the generic case so I'll just put it down here anw.

If, however, you do want to preserve the original value in the error, then you should specify the parametrization for the macro to understand which parameter should be substituted.

I agree with this. The most appropriate syntax for this case would probably look like what you proposed, struct CustomError<T>(TryIntoError<T>);.

1. A generic parameter T, without any type constraint, usually would mean any type could substitute it. However T does not mean just any type here. It has to be one of MixedInt &MixedInt or &mut MixedInt where appropriate, which is not shown in the syntax. There is documentation of course, just that (a) this is a bit out of the normal convention and (b) if I understand correctly, code for automatic derivation should aim to be obvious to read.
2. Usually (not always) in a generic function/trait with a generic parameter T, users would expect to look into the body of the function/trait to see where T is used.

I'm open to better syntactic suggestions, of course.

Originally, I thought of something like #[try_into(error(owned(MixedInt), ref(&MixedInt), ref_mut(&mut MixedInt)))], which is a bit verbose, but obvious. However, this doesn't work. It's not even correct. Besides the lifetime annotation issue, in this example

enum MixedInts {
    SmallInt(i32),
    NamedBigInt { int: i64 },
    Unit
}

the error type for the Unit case for ref is TryIntoError<MixedInt>, not TryIntoError<&MixedInt>! In comparison, your syntax struct CustomError<T>(TryIntoError<T>); with T means one of MixedInt &MixedInt or &mut MixedInt where appropriate is not only more concise, but also correct.

Maybe struct CustomError<T: MixedInt | &MixedInt | &mut MixedInt>(TryIntoError<_>);? I might just make it worse though.

[tyranron]

@kiendang okay, so let's roll out the solution without generics first and only support them if there will be enough demand for them. As I've mentioned above, caveats regarding generics should be mentioned in docs.

[kiendang]

@tyranron sorry for the delay.

The failed CI seems unrelated. I submitted #508 to fix.

I've added to doc and adjusted the error message as suggested

A very dirty hack could be used after parsing State::with_attr_params(), like: checking the returned error message and putting the better one. Covering this with test will ensure no regression is happened if the error from State::with_attr_params() is changed accidentally.

I want to adjust the message for this error as well, but I can't seem to be able to trigger it. Looks like both #[tryinto] and #[tryinto()] are allowed?

derive_more/impl/src/utils.rs

Line 858 in e0a0713

"Empty attribute is not allowed, add one of the following parameters: {}",

For the surplus attribute case I changed the message to Only a single attribute, plus a optional `#[try_into(error(...))]`, is allowed, but obviously the optional #[try_into(error(...))] is only applicable to the top level attribute, not the nested one. Should I adjust it to Only a single attribute, plus a optional `#[try_into(error(...))] if it is a top level attribute`, is allowed? Is there a more precise/formal term than top level attribute?

[tyranron]

@kiendang

Looks like both #[tryinto] and #[tryinto()] are allowed?

Maybe... it's quite entangled. Maybe an additional check is required in such a case?

Should I adjust it to Only a single attribute, plus a optional `#[try_into(error(...))] if it is a top level attribute`, is allowed? Is there a more precise/formal term than top level attribute?

"Top level" is a normal term for that. However, I think it makes sense to emit just different errors for top-level and variant-level.

[kiendang]

"Top level" is a normal term for that. However, I think it makes sense to emit just different errors for top-level and variant-level.

I agree, but for just adjusting the error message emitted from State::with_attr_params() we can't really differentiate. I think we can do that more easily once attributes are refactored with the utils::attr machinery as you said?

[kiendang]

anw the error now reads

Only a single attribute is allowed.
For the top-level attribute, an additional `#[try_into(error(...))]` may be provided.

PR's ready for review.

[tyranron]

@kiendang thanks!