Skip to content
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

Lack of immediate access to GenericArray to view associated functions and trait impls leads to confusion and annoyance. #575

Open
JustAnotherCodemonkey opened this issue Feb 12, 2024 · 13 comments

Comments

@JustAnotherCodemonkey
Copy link

It's understandable that aes-gcm and aes-gcm-siv use generic_array's GenericArray's for the Nonce type. What's less understandable though is why this is left an opaque type and not either a wrapper with its own documentation of its methods custom to the uses of ges-gcm* or that GenericArray is not re-exported both for convenience as well as documentation. As a new user, I was really confused by the fact that I could not access the documentation for the underlying type to see what associated functions/methods it supports or even how to get one besides converting from a slice (as is demonstrated by the top-level crate documentation). Not everyone knows the generic_array crate and knows what GenericArray is and I had to look through the source to find the implementations of what I thought were a private type but turned out to be an import that is used on the front end by the user despite not only not being re-exported but there being no mention in the top-level crate documentation where this type comes from.

I would personally prefer turning Nonce into a newtype custom wrapper but I understand that this can be an issue due to the fact that it fundamentally introduces breaking changes.

The second best option is to simply re-export GenericArray. After all, aes-gcm-siv re-exports the entire aead crate just like aes-gcm which also re-exports aes in addition. I think this is by far the most uncontroversial option but the seemingly random re-export seems for some reason distasteful, I propose a third proposal:

This one is a compromise between the last two: create a new type called something like NonceWrapper that does exactly what I mentioned in the first proposal and then impl From for Nonce, unwrapping it. One could then have replace all instances of taking a Nonce type with impl Into. I can't see any breakage that would come of this but obviously it would still work even without the function refactors, just less convenient.

I would be willing to make a PR if and when a solution is agreed upon. I would love to hear others' thoughts.

@tarcieri
Copy link
Member

This seems like a similar bug report to RustCrypto/hashes#563. For some reason we haven't gotten any reports on this in years, then get two in as many days. Strange.

See my response here. It seems like a rustdoc bug.

@JustAnotherCodemonkey
Copy link
Author

Ah I didn't realize that Rustdoc should be on this. Strange. I will say that I have seen multiple cases where different crates seem to use different versions of Rustdoc or something. I don't know much about the publishing/documenting process. Maybe we're stuck with a older version of Rustdoc? Now that I think about it, I do know what you're talking about and yeah Rustdoc should be linking it.

I'll keep this open for now only to ask if we should do anything temporary like manually link in the docs or explain what's going on?

@tarcieri
Copy link
Member

This seems like a recent rustdoc regression. I've filed an upstream issue: rust-lang/rust#120983

@JustAnotherCodemonkey
Copy link
Author

Should I create a PR for temporary documentation? I wonder how long it will take for this to get looked at and investigated none the less patched.

@tarcieri
Copy link
Member

Sure, more documentation would be good

@JustAnotherCodemonkey
Copy link
Author

I'll get on it

@JustAnotherCodemonkey
Copy link
Author

Ok, finally getting to it. Interestingly, when I open the docs locally, they link correctly. Hopefully that other issue goes somewhere.

@David-OConnor
Copy link

David-OConnor commented Feb 15, 2024

Strongly agree!

I will add another way to mitigate this, although your proposed 3 methods are all fine: Include realistic, complete code examples of importing, initializing etc. Either in addition to the simple ones at the main Docs page, or in an examples folder on this Github.

I personally would prefer if nonces were stored as [u8; 12] directly, or a thin wrapper over it, but I understand that there may be advantages to generics.

@JustAnotherCodemonkey
Copy link
Author

I personally would prefer if nonces were stored as [u8; 12] directly, or a thin wrapper over it, but I understand that there may be advantages to generics.

I personally totally 100% agree. The problem is introducing breaking changes like the proposed switch. I believe the reason behind the generic_array crate usage is that this crate was created and started using GenericArray's long before generic constants were available in stable rust (they hit stable in 2021 to my understanding). Now we're stuck with GenericArray's until the next major release if the maintainers even allow breaking changes at all (many don't in the Rust community)

@JustAnotherCodemonkey
Copy link
Author

I'm still having trouble knowing exactly how to link. It all works locally and we can't see the results immediately without pushing for docs.rs. I'm going ahead and hard-linking but I wonder if other kinds of linking would work. There's no way to know though...

@tarcieri
Copy link
Member

I believe the reason behind the generic_array crate usage is that this crate was created and started using GenericArray's long before generic constants were available in stable rust (they hit stable in 2021 to my understanding). Now we're stuck with GenericArray's until the next major release

It is definitely not that simple. min_const_generics lack many features we need to be a complete replacement for our use cases. There's an issue here which provides some background: RustCrypto/traits#970

We're in the process of making breaking changes and migrating to the new hybrid-array crate. This crate notably does make it possible to start presenting some const-generic public facing APIs. But to simplify the migration from generic-array to hybrid-array, we'll probably not change external-facing APIs to this approach yet. It's still a possibility, though.

@JustAnotherCodemonkey
Copy link
Author

Ah cool. Did not know that! I should check out hybrid-array. Is there anywhere I can follow the progress being made on that front? I'm curious what breaking changes have happened / are planned.

@JustAnotherCodemonkey
Copy link
Author

This would be resolved by #579. Linking here because apparently links in titles don't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@tarcieri @David-OConnor @JustAnotherCodemonkey and others