digest: problems with the Digest trait

[rvolgers]

I'm implementing a Merkle Tree based hash function, which uses an existing hash function as its base, and I ran into some annoying aspects of the Digest trait.

The documentation describes Digest as a "wrapper" around various other traits, namely Update, FixedOutput, Reset, Clone, and Default. But it is not actually a subtrait of these traits! This was very surprising to me.

In fact, it duplicates some functionality of the other traits, without being an implementation of them. In particular Digest::chain is an exact duplicate of Update::chain, Digest::reset is an exact duplicate of Reset::reset, Digest::update duplicates Update::update and the associated type Digest::OutputSize duplicates FixedOutput::OutputSize.

I ran into this when I tried to use finalize_into on a generic bounded by Digest. This function (which is defined in FixedOutput) is not available on Digest because Digest does not actually implement FixedOutput, it just requires it internally and then duplicates some of its functionality in its API (but not finalize_into).

So, my next idea was to set the generic bound to Digest + Update + FixedOutput + Reset. This causes chaos though, because now there are conflicts for the names chain, reset, and OutputSize, so you have to explicitly tell Rust whether you want the one defined by Digest or the one from the underlying trait, even though they should be the same. There was also some trouble because Rust could not see that FixedOutput::OutputSize and Digest::OutputSize are equal, and I had some variables that had to match both.

Finally, I decided Digest was not usable and I decided to simply use the underlying traits. This fixed all my problems but it's a shame to do without the convenience functions.

I don't really understand why the Digest trait works this way.

Concretely, I would suggest making Digest an actual subtrait of the traits it wraps, and removing the confusing duplicated functionality. This would technically be a breaking change. I would also move the digest::Output type alias to the fixed module.

(The DynDigest trait is very similar, everything I've said applies to that as well)

[rvolgers]

To be clear, I'm happy to submit a PR with my proposed changes, but I'm feeling a bit confused and wondering if I'm missing something.

I'm also wondering what the requirements are for compatibility, because while there is basically no change to the actual API in my proposed changes, they may cause some (easy to fix) breakage in code that uses e.g. <D as Digest>::OutputSize, which is something that people may have done to work around the problems I describe.

[newpavlov]

I vaguely remember that long ago I had some issues with making Digest an extension trait. But since then API has changed quite significantly, so I may take a second look at it. Introducing a breaking change is not an issue, since I already work on some extensive changes which will be published in v0.10, see #380.

Though, it's a different story for DynDigest. Since the sole reason for its existence is object-safety, we can not make it an extension trait.

[tarcieri]

I think the main advantage of the current setup is that consumers of the trait do not have to have all of the sub-traits in scope to use the various methods. I suppose an alternative to that would be something like use digest::prelude::*, but that still makes it unclear which traits are doing what until you hit a compile error.

Finally, I decided Digest was not usable and I decided to simply use the underlying traits. This fixed all my problems but it's a shame to do without the convenience functions.

@rvolgers it sounds like you actually find the aforementioned setup preferable when all the methods are available?

I ran into this when I tried to use finalize_into on a generic bounded by Digest. This function (which is defined in FixedOutput) is not available on Digest.

It seems like this problem could also be addressed by adding a finalize_into method to Digest.

[newpavlov]

I think the main advantage of the current setup is that consumers of the trait do not have to have all of the sub-traits in scope to use the various methods.

Ah, it was exactly this problem! It can be demonstrated using this example. Even though Foo is imported, we can not use methods from Foo1 and Foo2 on a concrete type without importing those traits explicitly. Surprisingly, in a generic function which takes impl Foo we can use those methods.

Ideally, I would love to have "inherent traits", which would make the Digest trait redundant. But, unfortunately, it does not look like we will get them in the near feature.

[tarcieri]

Also note that a rationale for the current layered trait design is given in the toplevel rustdoc, although perhaps it could be improved:

https://docs.rs/digest/0.9.0/digest/

Traits in this repository are organized into high-level convenience traits, mid-level traits which expose more fine-grained functionality, and low-level traits intended to only be used by algorithm implementations:

• High-level convenience traits: Digest, DynDigest. They are wrappers around lower-level traits for most common hash-function use-cases.
• Mid-level traits: Update, BlockInput, Reset, FixedOutput, VariableOutput, ExtendableOutput. These traits atomically describe available functionality of hash function implementations.
• Low-level traits: FixedOutputDirty, VariableOutputDirty, ExtendableOutputDirty. These traits are intended to be implemented by low-level algorithm providers only and simplify the amount of work implementers need to do and therefore shouldn't be used in application-level code.

[burdges]

You cannot mix Digest with the mid-level traits, so invariably I've always wound up using only the mid-level traits. This is not really a problem, just always use the mid-level traits and never use ..::Digest.

[rvolgers]

Thanks for the explanation, I guess it is a bit inconvenient to import the mid level traits, but the IDE takes care of that for me so I personally don't mind. But this does invalidate my initial suggestions.

I do think the documentation can be improved. I realize now the intention behind the word "wrapper" that is used here, but it was not at all clear to me initially. No matter how many traits you have it is still the same type, so to me it was natural to try and mix and match traits. The resulting errors can also get really strange if you are unlucky.

I would still like to try to improve this experience a little, even if only by updating documentation. Concretely I would like to add some text to Digest and DynDigest that importing the lower level traits is not recommended when using the high level one, since they use identical names for functions.

What I'm not going to change (because it feels like a significant change and I'm not sure I have enough time and I know I don't have enough experience with this project), but might be worth considering, is whether the high/mid/low idea is a helpful concept. If the "high level" traits became separate (trivial) wrapper structs documented as a "simplified API" and the "low-level" traits were instead described as "implementation details" I think it would help people a ton in building the right mental model for how they are expected to be used.

[newpavlov]

I've added the finalize_into methods to the Digest trait in #380, so I am gonna close this issue. Either way doc improvement PRs are welcomed!