New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bound
attribute for generics support
#143
Conversation
aee3e31
to
0ce38bc
Compare
0ce38bc
to
a95fd83
Compare
This comment has been minimized.
This comment has been minimized.
Also add a test to catch regressions in this behaviour.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
From skimming the implementation this broadly looks good to me, but two questions below before I review for real.
#[derive(Error, Debug)] | ||
#[error(bound = std::error::Error + 'static)] | ||
struct WithGenericStructRef<'a, T> { | ||
inner: &'a WithGenericStruct<T>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is WithGenericStructRef
based on a real world use case? Or do your real world uses pretty much all look like one of the ones above, where the type parameter is used as the whole field type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have at least one of this cases occurring in my current codebase.
I am, however, seeing some difficulties wherein I'd like to be able to "skip" bounds on generics that only exist due to a lack of universal type qualifiers on content fields in enums. That, or some way of specifying bounds on a per-generic-parameter basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you have a suggested solution here, I've fixed the other issue and believe that, while some patterns may be inconvenient, this change does help quite a bit in my code. It may be a stepping stone toward a more robust solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look through all the error types containing generic parameters in your work repo which got linked to this PR. As of commit microsoft/snocat@837451a it seems they basically follow 3 patterns:
#[derive(Error, Debug)] #[error(bound = Debug)] enum T<E> { ..., V(E) }
#[derive(Error, Debug)] enum T<E: Debug + Display> { ..., V(E) }
#[derive(Error, Debug)] enum T<E: Debug + Display + 'static> { ..., V(#[from] U<E>) }
The ones that have E: Bounds...
in the data structure are probably misguided according to C-STRUCT-BOUNDS, so I'll ignore those bounds and assume they were all written using a bound
attribute instead. Then the only remaining variation is a matter of E
vs U<E>
, #[from]
vs no #[from]
.
From this I get the sense that a bound
attribute is probably the wrong tool. In all of these cases the correct bound is readily determined programmatically and a handwritten bound
attribute would be just noise and overhead for the programmer.
I am going to go ahead and close this PR so that we have an implementation of bound
preserved, but I would recommend pursuing something along the lines of #79 (comment) instead. Namely infer a Display
bound on every field type used in #[error("{}")]
if it contains an unbounded type parameter, infer Debug
on every field type used in #[error("{:?}")]
if it contains an unbounded type parameter, and infer Error + 'static
on every field type annotated #[from]
or #[source]
which contains an unbounded type parameter.
So for example we'd have:
#[derive(Error, Debug)]
pub enum NegotiationError<ApplicationError> {
#[error("Negotiation fatal error: {0:?}")]
FatalError(ApplicationError),
}
#[derive(Error, Debug)]
pub enum TunnelLifecycleError<RegistryError> {
#[error("Tunnel registration error")]
RegistrationError(#[from] TunnelRegistrationError<RegistryError>),
}
expand into:
impl<AppE> Display for NegotiationError<AppE>
where
AppE: Debug;
impl<AppE> Error for NegotiationError<AppE>
where
Self: Debug + Display;
impl<RegE> Display for TunnelLifecycleError<RegE>;
impl<RegE> Error for TunnelLifecycleError<RegE>
where
TunnelRegistrationError<RegE>: Error,
RegE: 'static,
Self: Debug + Display;
Attempts to fix #79 via the bounds suggestion in dtolnay's comment.
Note that this adds the bounds to any constructed
Display
instance as well as thestd::error::Error
instance, which may deviate from intent; if that is not desired, or if a separatedisplay_bound
parameter would be preferred, it would be trivial to remove or separate.