Skip to content

Seal Parsable trait methods#797

Closed
Powerbyte7 wants to merge 2 commits into
time-rs:mainfrom
Powerbyte7:seal-parsable-methods
Closed

Seal Parsable trait methods#797
Powerbyte7 wants to merge 2 commits into
time-rs:mainfrom
Powerbyte7:seal-parsable-methods

Conversation

@Powerbyte7

@Powerbyte7 Powerbyte7 commented Jun 30, 2026

Copy link
Copy Markdown

This PR prevents the issue in #793 from reoccurring in the future, by embedding an unnameable type in the trait method signatures. The logic is analogous to what can be seen in the following code:

mod outer_sealed {
    mod inner_sealed {
        pub struct Token;
        pub struct Seal<T>(T);
    }
    
    pub(self) use inner_sealed::Token;
    
    pub trait SealedTrait {
        fn method(&self, _: inner_sealed::Token);
        fn method_with_return(&self) -> inner_sealed::Seal<u32>;
    }
}

use outer_sealed::SealedTrait; // The trait remains publicly usable
// use outer_sealed::Token; // This causes an error, which is what we want

For more info on this practice, see the following:
https://rust-lang.github.io/api-guidelines/future-proofing.html#newtypes-encapsulate-implementation-details-c-newtype-hide
https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/

I have tested this PR by running cargo test --all-features, and also also by checking that the build fails when importing the unnameable type. This PR can be considered breaking in the same way the changes to #793 were, but will improve future API stability.

@jhpratt

jhpratt commented Jul 1, 2026

Copy link
Copy Markdown
Member

Thanks for the quick PR! I actually ended up going a bit farther, using a similar approach for the formatting trait as well. I'm not sure if it was technically necessary, but there's no harm in adding a ZST as a parameter. Given the larger scope and timeliness of the issue, I'm doing this myself. Sorry!

@jhpratt jhpratt closed this Jul 1, 2026
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