RUST-2412 Provide an operation wrapper abstraction#1738
Conversation
| } | ||
|
|
||
| impl<T: OperationWithDefaults> Operation for T | ||
| pub(crate) trait OperationDispatch<Kind> { |
There was a problem hiding this comment.
This intermediate trait is needed because the rule is that there can be only one blanket impl per trait, so the blanket impl is Operation for OperationDispatch<K>. The blanket impls for the helper traits don't run into this issue because OperationDispatch<WithDefaults> and OperationDispatch<Wrapper> are considered to be distinct traits for the purposes of overlap checks.
Ideally we could just include OperationImpl<Kind=WithDefaults> / <Kind=Wrapper> in a blanket Operation impl but it turns out there's a longstanding compiler bug that prevents that 🙁
There was a problem hiding this comment.
Makes sense - can you add a comment on this file explaining the basic trait hierarchy? I don't think I'll remember this rationale next time I look at this 🙂
|
The failures here are fixed by #1739. |
abr-egn
force-pushed
the
RUST-2412/operation-wrapper
branch
from
2873c7e to
3e6c9fd
Compare
| } | ||
|
|
||
| impl OperationImpl for Create { | ||
| type Kind = WithDefaults; |
There was a problem hiding this comment.
I think the WithDefaults naming is more descriptive of the implementation than the concept. Can we change this to be something like Base or Root? And ditto for the OperationWithDefaults trait - something like BaseOperation or RootOperation would make it more clear that the types that define that trait are ones that don't wrap another operation.
(Not necessarily a new concern on this PR, but something I got to thinking about more while reviewing.)
| } | ||
|
|
||
| impl<T: OperationWithDefaults> Operation for T | ||
| pub(crate) trait OperationDispatch<Kind> { |
There was a problem hiding this comment.
Makes sense - can you add a comment on this file explaining the basic trait hierarchy? I don't think I'll remember this rationale next time I look at this 🙂
2 participants
RUST-2412
Not directly related to the main work for this ticket, but that involves adding a field to
Operationand I got annoyed at having to always update the various wrappers.This adds an
OperationWrappertrait that knows how to dispatch to a generic innerOperation; this means that the various places we have such (four as of this writing) are both easier to read, since they only need to list out the methods where the behavior is changed by the wrapper, and don't have to be updated when we add new methods to the core trait.The downside is that, because we can't have two blanket traits that impl
OperationforOperationWithDefaultsandOperationWrapper, those now work via a helper traitOperationDispatchand tag traitOperationImpl. IMO, this is worth it because the added boilerplate per-operation is minimal and the helpers are largely ignorable (nothing outside the one blanket impl inoperation.rsneeds to care aboutOperationDispatch).