Leaking traits or types from private modules

When wanting to provide automatic implementations based on whether a type can provide a certain raw pointer (as defined in a private module), I came across the following pattern(s):

Case 1:

/// Some module provided by a crate (this is public!)
pub mod module {

    /// Some internals (this is supposed to be private!)
    mod internal {
        pub struct State {}
        impl State {
            pub fn hello_world(&self) {
                println!("Hello world!")
            }
        }
        pub trait HasState {
            fn state(&self) -> &State;
        }
    }

    /// A public trait
    pub trait FooTrait {
        fn foo(&self);
    }

    /// Anything that has a state (i.e. implements internal::HasState)
    /// will automatically get an implementation of FooTrait
    impl<T: internal::HasState> FooTrait for T {
        fn foo(&self) {
            self.state().hello_world();
        }
    }

    /// Just one particular struct, we could have more like
    /// S2, S3, S3, …
    pub struct S1 {
        state: internal::State,
    }

    impl S1 {
        pub fn new() -> Self {
            Self {
                state: internal::State {},
            }
        }
    }

    impl internal::HasState for S1 {
        fn state(&self) -> &internal::State {
            &self.state
        }
    }
}

fn main() {
    use module::{FooTrait, S1};
    let v1 = S1::new();
    v1.foo();
}

(Playground)

This code works. However, when I generate the documentation (assuming I create a library instead of a binary crate), then I get:

Trait mycrate::module::FooTrait

Implementors

impl<T: HasState> FooTrait for T`

Anything that has a state (i.e. implements internal::HasState) will automatically get an implementation of FooTrait

Note that HasState is in a private module (mod internal). It will not be linked by rustdoc but its name (and documentation comment) leaked as above.

Case 2:

I move my trait HasState one level up and declare it private (it really doesn't matter for the public whether a type has a state or not; the important trait for the public is FooTrait). Now I get a warning that my code will soon fail to work:

/// Some module provided by a crate (this is public!)
pub mod module {

    /// Some internals (this is supposed to be private!)
    mod internal {
        pub struct State {}
        impl State {
            pub fn hello_world(&self) {
                println!("Hello world!")
            }
        }
    }

    trait HasState {
        fn state(&self) -> &internal::State;
    }

    /// A public trait
    pub trait FooTrait {
        fn foo(&self);
    }

    /// Anything that has a state (i.e. implements internal::HasState)
    /// will automatically get an implementation of FooTrait
    impl<T: HasState> FooTrait for T {
        fn foo(&self) {
            self.state().hello_world();
        }
    }

    /// Just one particular struct, we could have more like
    /// S2, S3, S3, …
    pub struct S1 {
        state: internal::State,
    }

    impl S1 {
        pub fn new() -> Self {
            Self {
                state: internal::State {},
            }
        }
    }

    impl HasState for S1 {
        fn state(&self) -> &internal::State {
            &self.state
        }
    }
}

fn main() {
    use module::{FooTrait, S1};
    let v1 = S1::new();
    v1.foo();
}

(Playground)

The warning I get is:

   Compiling playground v0.0.1 (/playground)
warning: private trait `HasState` in public interface (error E0445)
  --> src/main.rs:25:5
   |
25 | /     impl<T: HasState> FooTrait for T {
26 | |         fn foo(&self) {
27 | |             self.state().hello_world();
28 | |         }
29 | |     }
   | |_____^
   |
   = note: `#[warn(private_in_public)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #34537 <https://github.com/rust-lang/rust/issues/34537>

warning: `playground` (bin "playground") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 1.10s
     Running `target/debug/playground`

Case 3:

I keep HasState one level up (i.e. directly in module) and make it public (which I don't really want as it's an implementation detail).

/// Some module provided by a crate (this is public!)
pub mod module {

    /// Some internals (this is supposed to be private!)
    mod internal {
        pub struct State {}
        impl State {
            pub fn hello_world(&self) {
                println!("Hello world!")
            }
        }
    }

    pub trait HasState {
        fn state(&self) -> &internal::State;
    }

    /// A public trait
    pub trait FooTrait {
        fn foo(&self);
    }

    /// Anything that has a state (i.e. implements internal::HasState)
    /// will automatically get an implementation of FooTrait
    impl<T: HasState> FooTrait for T {
        fn foo(&self) {
            self.state().hello_world();
        }
    }

    /// Just one particular struct, we could have more like
    /// S2, S3, S3, …
    pub struct S1 {
        state: internal::State,
    }

    impl S1 {
        pub fn new() -> Self {
            Self {
                state: internal::State {},
            }
        }
    }

    impl HasState for S1 {
        fn state(&self) -> &internal::State {
            &self.state
        }
    }
}

fn main() {
    use module::{FooTrait, S1};
    let v1 = S1::new();
    v1.foo();
}

(Playground)

This works again without problems, but now I leak a type from a private module through the publicly available module::HasState, whose method state returns &internal::State.

What do do?

I figured out a complicated way to avoid any of this by providing a wrapper type for State, but it's very verbose. My question is:

Should I make the effort to avoid any of the three patterns?

Taking the warning seriously, Case 2 will be disallowed soon. But what about Case 1 and Case 3? Are these sound to work with or would I risk running into problems in future as these seem to be very similar? Also, leaking the trait internal::HasState (in Case 1) or the type internal::State (in Case 3) seems to be a bit ugly at least and could confuse readers of the documentation.

What's usually done here?


Potentially related threads:

Case 3 should only be implemented if the trait indeed should be public.

Case 2 should never happen.

Case 1 is valid, and in fact, the way to make sealed traits in Rust (pub trait Public: private::Sealed {}). So I guess any experienced Rust developer will understand what happening here, although I recommend making it clear in the name that internal::State is private (putting it in the internal module, for instance :stuck_out_tongue: ) since it can be confusing that you can't click it in rustdoc otherwise.

Not answering your questions, but considering how cargo doc works at the moment, I am considering ( and tending ) to make almost everything public in my modules, to make the library comprehensible at the documentation level, and place responsibility on the consumer of the library as to what internal details to use ( with the risk they may change ). Highly unconventional I know!

[ Sorry if this hijacks your post, maybe I should post separately ]

I sometimes do miss an option to process documentation comments to my private submodules, traits and types. While it isn't used by users of my crate or module, I, as a developer, would like to have such documentation.

I think making these items pub is the wrong choice, but I understand your approach, given that I don't think rustdoc will document private items (or is there any switch for that?)

No problem in posting here, but maybe you can rise this issue in a new thread (unless you know the answer already; I'd like to know it).

You can document private items with

cargo doc --document-private-items
2 Likes

I posted here.

And are there any implications in leaking a private an inaccessible type?

Considering the warning I got, I have to agree.

I don't think this is identical to the "sealed trait pattern". This is because the trait HasState isn't visible from outside the module module, and the trait FooTrait isn't sealed. Compare to the example in the API guidelines, where TheTrait is public:

/// This trait is sealed and cannot be implemented for types outside this crate.
pub trait TheTrait: private::Sealed {
    // Zero or more methods that the user is allowed to call.
    fn ...();

    // Zero or more private methods, not allowed for user to call.
    #[doc(hidden)]
    fn ...();
}

// Implement for some types.
impl TheTrait for usize {
    /* ... */
}

mod private {
    pub trait Sealed {}

    // Implement for those same types, but no others.
    impl Sealed for usize {}
}

Instead, in my Case 1, mentioning HasState happens here:

    /// Anything that has a state (i.e. implements internal::HasState)
    /// will automatically get an implementation of FooTrait
    impl<T: internal::HasState> FooTrait for T {
        fn foo(&self) {
            self.state().hello_world();
        }
    }

Thus I see a difference between those "patterns".

That said, the pattern in Case 1 utilizes the same odd behavior that the sealed trait pattern is exploiting: Inside a module, I can, within a public item, refer to a trait X that is non-visible from outside the module by making X public but placing it in a private submodule.

The Rust documentation guarantees that X will not be visible. See section 12.6 (Visibility and Privacy):

With the notion of an item being either public or private, Rust allows item accesses in two cases:

  1. If an item is public, then it can be accessed externally from some module m if you can access all the item's ancestor modules from m. You can also potentially be able to name the item through re-exports. See below.
  2. If an item is private, it may be accessed by the current module and its descendants.

Thus it's clear that HasState isn't "accessible". But I still get a guarantee that the

impl<T: internal::HasState> FooTrait for T

is "accessible" from the outside? I don't see a clear answer for that in the reference. Maybe there is one. If so, can you help me find it?

Even if in case of sealed traits, the behavior will be stable (I didn't find any guarantee on that either in the reference), the behavior in case of using such a non-visible trait in impls might be a different case.

Following the other threads linked above, I came to the conclusion that the "sealed trait pattern" (and thus relying on the current behavior) seems to be common. However, I also found a voice which described this as possible "bug":

That's a lot of links to follow, and I have to admit, I didn't read all of that. The last one, however, indeed seems to be official(ish?) because its source is located under https://github.com/rust-lang/api-guidelines. However, the API guidelines reflect best practices and aren't a reference, right?

Besides, my case doesn't seem to be identical to the sealed trait pattern, as explained above.