Skip to content

Conversation

TomAFrench
Copy link
Contributor

@TomAFrench TomAFrench commented Sep 9, 2025

This PR rolls out the unreachable_pub lint over all the crates, fixing any issues raised. These are either items which have not been exported for quite some time or are very clearly internal implementation details.

I've used an inner attribute to activate the lint - my preference for this is to use the linting system in cargo.toml as it makes it easiest to enforce consistency over multiple crates but I didn't want to cause a split with the existing config.

@TomAFrench TomAFrench changed the title Tf/unreachable pub chore: apply unreachable_pub lint to subset of crates Sep 9, 2025
@TomAFrench TomAFrench changed the title chore: apply unreachable_pub lint to subset of crates chore: apply unreachable_pub lint to crates Sep 9, 2025
@newpavlov
Copy link
Member

I don't think I agree with this lint. It's pretty common in my experience to define a pub(crate) module and pub items in it with the implicit assumption that items will also be pub(crate). Same with pub(crate) struct and pub fn methods on it. There are also the sealed traits which use unreachability of pub items intentionally (I know people want to add a proper support for it, but the proposals do not have a clear timeline).

@newpavlov
Copy link
Member

@tarcieri
WDYT? Should we be more strict with pubs? If yes, then we would need to gradually enable the lint in other repos as well.

@tarcieri
Copy link
Member

tarcieri commented Sep 9, 2025

I'm a fan of using pub(crate) when you mean pub(crate), since it will prevent accidental leakage, though sealed traits are a notable exception.

If it's just warn though, you can always do allow where you need exceptions

@newpavlov
Copy link
Member

Ok, let's move forward with it then.

@TomAFrench
Could you create similar PRs later in other repos as well?

);

pub fn compress(h: &mut [u32; DIGEST_BUF_LEN], data: &[u8; 64]) {
pub(crate) fn compress(h: &mut [u32; DIGEST_BUF_LEN], data: &[u8; 64]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use either pub(crate) or pub(super) for both the function and the constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I've changed the remaining uses of super to basically just cover the architecture-specific compression functions as these are the only items which should definitely never be imported elsewhere in the crate.

@TomAFrench
Copy link
Contributor Author

Should be good to go now

@newpavlov
Copy link
Member

Thank you!

@newpavlov newpavlov merged commit 1f5267d into RustCrypto:master Sep 10, 2025
236 checks passed
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

Successfully merging this pull request may close these issues.

3 participants