Private trait in public interface: sometimes allowed and sometimes not?

I don't really understand why

  • impl<T: Internal> Trt for T is allowed for a private trait Internal,
  • but impl<T: Internal> Dbl<T> is not allowed if Internal is private,
  • yet impl<T: hidden::Internal> Dbl<T> works again if hidden is private and Internal is public.

Is this really intended? And if yes, why? This makes things unnecessarily complex for me.

If all three cases were disallowed, I could understand it. But only disallowing the second case, while the first and third case are legal in Rust, doesn't make sense to me. But perhaps there is a good design rationale behind it?

In my real-life code, I don't want to make Internal public because it exposes some private state.

P.S.: See also similar discussion: Why are private traits in public interfaces disallowed in this setting?

Full code example below:

mod api {
    #[derive(Default)]
    pub struct A {
        internal: i32,
    }
    #[derive(Default)]
    pub struct B {
        internal: i32,
    }
    pub struct Dbl<T> {
        pub one: T,
        pub two: T,
    }
    trait Internal {
        fn internal_ref(&self) -> &i32;
        fn internal_mut(&mut self) -> &mut i32;
    }
    impl Internal for A {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    impl Internal for B {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    pub trait Trt {
        fn increment(&mut self);
        fn result(&self) -> i32;
    }
    // This is allowed:
    impl<T: Internal> Trt for T {
        fn increment(&mut self) {
            *self.internal_mut() += 1;
        }
        fn result(&self) -> i32 {
            *self.internal_ref()
        }
    }
    // But this causes warning/error E0445:
    impl<T: Internal> Dbl<T> {
        pub fn sum(&self) -> i32 {
            self.one.result() + self.two.result()
        }
    }
    // Yet we can do the following:
    pub trait SumViaTrait {
        fn sum_via_trait(&self) -> i32;
    }
    impl<T: Internal> SumViaTrait for Dbl<T> {
        fn sum_via_trait(&self) -> i32 {
            self.one.result() + self.two.result()
        }
    }
    // Or we can do:
    mod hidden {
        pub trait Internal {
            fn internal_ref(&self) -> &i32;
            fn internal_mut(&mut self) -> &mut i32;
        }
        impl Internal for super::A {
            fn internal_ref(&self) -> &i32 {
                &self.internal
            }
            fn internal_mut(&mut self) -> &mut i32 {
                &mut self.internal
            }
        }
        impl Internal for super::B {
            fn internal_ref(&self) -> &i32 {
                &self.internal
            }
            fn internal_mut(&mut self) -> &mut i32 {
                &mut self.internal
            }
        }
    }
    pub trait Trt2 {
        fn increment2(&mut self);
        fn result2(&self) -> i32;
    }
    impl<T: hidden::Internal> Trt2 for T {
        fn increment2(&mut self) {
            *self.internal_mut() += 1;
        }
        fn result2(&self) -> i32 {
            *self.internal_ref()
        }
    }
    impl<T: hidden::Internal> Dbl<T> {
        pub fn sum_via_priv_mod(&self) -> i32 {
            self.one.result2() + self.two.result2()
        }
    }
}

fn main() {
    use api::*;
    let mut dbl = Dbl {
        one: A::default(),
        two: A::default(),
    };
    dbl.one.increment();
    dbl.two.increment();
    dbl.two.increment();
    println!("{}", dbl.sum());
    println!("{}", dbl.sum_via_trait());
    println!("{}", dbl.sum_via_priv_mod());
}

(Playground)

Output:

3
3
3

Errors:

   Compiling playground v0.0.1 (/playground)
warning: private trait `api::Internal` in public interface (error E0445)
  --> src/main.rs:48:5
   |
48 | /     impl<T: Internal> Dbl<T> {
49 | |         pub fn sum(&self) -> i32 {
50 | |             self.one.result() + self.two.result()
51 | |         }
52 | |     }
   | |_____^
   |
   = 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 4.48s
     Running `target/debug/playground`

2 Likes

I see the rational for allowing the third case (the type Internal is public, just unnamable, and we use it for the sealed trait pattern), but it's strange the first and second cases behave differently.

I understand the sealed trait pattern, but… I don't understand why Rust forces the programmer to use a module for it. I find that workaround (or should I say "pattern"?) quite ugly also :frowning:.

I tried some more things, and found out that

  • impl<T: Internal> T

(which might come in handy for me) doesn't work either, but with an entirely different error message:

mod api {
    #[derive(Default)]
    pub struct A {
        internal: i32,
    }
    #[derive(Default)]
    pub struct B {
        internal: i32,
    }
    trait Internal {
        fn internal_ref(&self) -> &i32;
        fn internal_mut(&mut self) -> &mut i32;
    }
    impl Internal for A {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    impl Internal for B {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    impl<T: Internal> T {
        pub fn increment(&mut self) {
            *self.internal_mut() += 1;
        }
        pub fn result(&self) -> i32 {
            *self.internal_ref()
        }
    }
}

fn main() {
    use api::*;
    let a = A::default();
    a.increment();
    println!("{}", a.result());
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0118]: no nominal type found for inherent implementation
  --> src/main.rs:30:23
   |
30 |     impl<T: Internal> T {
   |                       ^ impl requires a nominal type
   |
   = note: either implement a trait on it or create a newtype to wrap it instead

For more information about this error, try `rustc --explain E0118`.
error: could not compile `playground` due to previous error

That's coherence, not related at all (you can't have a non-trait impl for non-local type, and rustc doesn't analyze the trait to see it's only implemented for specific types and private so can't be implemented for more).

1 Like

Oh I see, but Internal is a private trait, so I don't see harm if it was allowed? (Not sure if I fully understand the implications though.)

I guess it was just never done (probably not motivated enough).

It makes some sense to me in the case of public functions like your sum. The fact that the type and function are public implies I should be able to do this:

fn just_takes_dbl<T>(d: api::Dbl<T>) {
    let _ = d.sum();
}

But I get:

error[E0599]: the method `sum` exists for struct `Dbl<T>`, but its trait bounds were not satisfied
    --> src/lib.rs:75:15
     |
10   |     pub struct Dbl<T> {
     |     -----------------
     |     |
     |     method `sum` not found for this
     |     doesn't satisfy `Dbl<T>: Iterator`
...
75   |     let _ = d.sum();
     |               ^^^ method cannot be called on `Dbl<T>` due to unsatisfied trait bounds
     |
     = note: the following trait bounds were not satisfied:
             `T: Internal`
             `Dbl<T>: Iterator`
             which is required by `&mut Dbl<T>: Iterator`
note: the following trait must be implemented
help: consider restricting the type parameter to satisfy the trait bound
     |
74   | fn just_takes_dbl<T>(d: api::Dbl<T>) where T: Internal {
     |                                      +++++++++++++++++

And applying the hint results in a privacy error. I.e. there is no way to write the generic bound.


However, the lint fires if sum is private too, so it's not really an answer to your question. Most the discussions I see are about keeping types private, not traits.

1 Like

Maybe the problem isn't as big as I originally thought.

In my original simplified example, a simple fix would be:

-    // But this causes warning/error E0445:
-    impl<T: Internal> Dbl<T> {
+    // Fixed:
+    impl<T: Trt> Dbl<T> {

(Playground)

Nonetheless, I feel like impl<T: PrivateTrait> … might be a helpful pattern in some cases. I would like to share some of my (yet under construction) real-world code instead of the toy example above:

#[derive(Debug)]
struct EnvBackend {
    inner: *mut lmdb::MDB_env,
    db_mutex: Mutex<()>,
}

unsafe impl Send for EnvBackend {}
unsafe impl Sync for EnvBackend {}

/// Read-only handle for accessing environment that stores key-value databases
#[derive(Clone, Debug)]
pub struct EnvRo {
    backend: Arc<EnvBackend>,
}

unsafe impl Send for EnvRo {}
unsafe impl Sync for EnvRo {}

/// Read-write handle for accessing environment that stores key-value databases
#[derive(Debug)]
pub struct EnvRw {
    backend: Arc<EnvBackend>,
}

trait EnvInternal {
    fn backend(&self) -> &Arc<EnvBackend>;
}

impl EnvInternal for EnvRo {
    fn backend(&self) -> &Arc<EnvBackend> {
        &self.backend
    }
}

impl EnvInternal for EnvRw {
    fn backend(&self) -> &Arc<EnvBackend> {
        &self.backend
    }
}

/// Read-write or read-only handle for accessing environment that stores key-value databases
pub trait Env {
    /// Get new read-only handle for environment
    fn clone_ro(&self) -> EnvRo;
    /// Get maximum size of keys and duplicate data
    fn maxkeysize(&self) -> usize;
}

impl<T: EnvInternal> Env for T {
    fn clone_ro(&self) -> EnvRo {
        EnvRo {
            backend: self.backend().clone(),
        }
    }
    fn maxkeysize(&self) -> usize {
        unsafe {
            lmdb::mdb_env_get_maxkeysize(self.backend().inner)
                .try_into()
                .unwrap_or(usize::MAX)
        }
    }
}

Here, I use EnvInternal and impl<T: EnvInteral> to avoid duplicate code. I don't want to make the backend method be part of the public trait Env, because I consider the *mut lmdb::MDB_env pointer and the Mutex an implementation detail.

P.S.: I noticed that this approach has the disadvantage that all methods of Env must then have the same implementation for EnvRo and EnvRw (and cannot differ in some cases). So maybe it's not a good idea/style anyway?

In another section of my code, I have:

impl<'a> EnvBuilder<EnvRo, &'a Path> {
    /// Open environment in read-only mode
    pub fn open(&self) -> Result<EnvRo, io::Error> {
        Ok(EnvRo {
            backend: Arc::new(self.open_backend()?),
        })
    }
}

impl<'a> EnvBuilder<EnvRw, &'a Path> {
    /// Open environment in read-write mode
    pub fn open(&self) -> Result<EnvRw, io::Error> {
        Ok(EnvRw {
            backend: Arc::new(self.open_backend()?),
        })
    }
}

I wanted to do something like impl<'a, P: PrivateTrait> EnvBuilder<P, &'a Path> here, but then ran into the problems described in my original post. However, I'm not sure it really is the best way to do it. Maybe simply having two implementations of open as above is perfectly fine.

So perhaps there is no real problem with disallowing impl<T: P> S<T> { /*…*/ } if P is private. Nonetheless, I mentioned this thread in the tracking issue earlier. Maybe it is of relevance when considering making the lint deny-by-default.

This is also true with a naive sealed trait workaround (make it a public, but unnameable due to private submodudle, trait).

A work-around, as used in std (and published as by the lang team as a guideline), is to have a does-nothing sealed supertrait instead. Then your Internal trait can be named and bound on, but still not implemented.

So that's a better way to do sealed traits.

impl<'a> EnvBuilder<EnvRo, &'a Path> {
// ...
impl<'a> EnvBuilder<EnvRw, &'a Path> {

Seems like you're anticipating only a fixed set of types for that first parameter, so maybe a sealed trait for them would be appropriate anyway?


More conversation about this general topic can be found here.

1 Like

I have a strong feeling the sealed trait "pattern" was actually a bug in how the compiler implements privacy that people started using widely before the compiler team could fix it.

Really, everyone should start using something like the sealed crate so we can move away from the hack and use a proper #[sealed] attribute... That ship has probably sailed, though :disappointed:

5 Likes

In my (real-world) case, if I make EnvInternal public (but sealed), then I would need to make EnvBackend public as well (otherwise I expose a private type in a public interface again, by returning &Arc<EnvBackend>). I don't want to have either of these types/traits in my public interface. They are implementation details.

Also note that if EnvInternal is public, I still can use its methods, thus gaining access to the underlying pointer.

Of course, I can't do anything evil with that pointer without using unsafe, but it still feels wrong if I consider that pointer to be an implementation detail.

To give a toy example, consider this playground, where I added a safety check to the Trt::increment method. If the Internal trait is public in that example (which it needs to be to avoid the to-be-made-deny-by-default lint E0445), then I can manipulate the internal state in any fashion (which may not be desired).

The only solution seems to be to move Internal to hidden::Internal (with hidden being a private module.

But transferring that to my real-world example (as posted above) again, I would need to move EnvInternal to such a hidden (private) submodule. EnvInternal then needs to be public, which means EnvBackend needs to be public too. But I want it not being part of the API, so I have to move it to he hidden submodule too. Effectively, half of my code goes to a submodule. Maybe that's cleaner anyway, but maybe not. Rust forces me to move several things to submodules in order to keep my API clean. I would like to decide on visibility and privacy independently and not by having to move things to private modules and making the inner items public, which then causes a cascade that forces me to move even more things to that private module. :slightly_frowning_face:

2 Likes

Yeah, that is a lot of fallout. You can hide some of the internal details with (private-field) new types, but that's even more boiler-plate.

(I suggested it because it matches the pattern that IMO makes sense to warn about [1] -- it's impossible to write a trait bound to cover all open-containing EnvBuilder types where EnvInternal is not usable, but it will be clear that the unnameable trait represents the ability to open.)

This accepted RFC makes me think that warning will never become an error though. [2] It talks about your use case here. It hasn't seen movement since accepted, which after this long usually makes me consider it unwise to rely on the RFC actually meaning anything, but the most pertinent dev is still acting like it will happen as of a month ago, which sways me the other direction.

If you feel the same way, you could gamble on disabling the lint. I don't even think it's a terrible gamble; you might have to switch to macros if it becomes an error, but I don't think that's breaking, since no one could name the trait anyway.

Speaking of...

...when I've ran into orphan rules trouble when trying to use traits for purely code-reduction purposes, I usually fall back to macros. I've done it a few times and now I think of the code-reduction pattern as "using traits as macros" in my head. Granted, macros are somewhat ugly.


  1. (though not necessarily error about) ↩︎

  2. If I'm right, this is the second such "Error... psych! We're (currently planning on) allowing it!" I'm aware of, which is rather disappointing. ↩︎

That would mean to simply do:

+    #[allow(private_in_public)]
     trait Internal {
         fn internal_ref(&self) -> &i32;
         fn internal_mut(&mut self) -> &mut i32;
     }

The lint has to be disabled at the private trait (and not at the impl where it's used, it seems). (Playground)

I didn't want to use the traits just for code reduction but also to make things semantically more clear (i.e. be one method instead of two distinct methods with just the same name). Now thinking about it, I found an elegant solution:

-impl<'a> EnvBuilder<EnvRo, &'a Path> {
-    /// Open environment in read-only mode
-    pub fn open(&self) -> Result<EnvRo, io::Error> {
-        Ok(EnvRo {
-            backend: Arc::new(self.open_backend()?),
-        })
-    }
-}
-impl<'a> EnvBuilder<EnvRw, &'a Path> {
-    /// Open environment in read-write mode
-    pub fn open(&self) -> Result<EnvRw, io::Error> {
-        Ok(EnvRw {
-            backend: Arc::new(self.open_backend()?),
-        })
-    }
-}

+pub trait EnvBuilderOpen<T> {
+    /// Open environment
+    fn open(&self) -> Result<T, io::Error>;
+} 
+impl<'a, T: EnvInternal> EnvBuilderOpen<T> for EnvBuilder<T, &'a Path> {
+    /// Open environment
+    fn open(&self) -> Result<T, io::Error> {
+        todo!() // can use EnvInternal
+    }
+}

This corresponds to SumViaTrait::sum in my original example. Perhaps that's the clean/intended way to go!

To someone outside your crate, your inherent opens are, practically speaking, two distinct methods with just the same name, as they can't write a bound that matches the implementation. That is in a sense the core conflict between your "it's an implementation detail" and the lint: by making the trait inaccessible, you removed its semantic relevance to consumers of your crate. They had to know the concrete type to call the method; they're not going to care about the semantics of how you implemented it if they can't take advantage of said semantics.

2 Likes

Hmmmm, interesting. So I was tempted to say that if the lint was turned into a hard error in future, then I couldn't use generic impls for code deduplication unless I give the user of my module a way to write down a bound that covers the implementations that use the deduplication. But that's not entirely true. Consider the following code:

mod api {
    #[derive(Default)]
    pub struct A {
        internal: i32,
    }
    #[derive(Default)]
    pub struct B {
        internal: i32,
    }
    pub struct Dbl<T> {
        pub one: T,
        pub two: T,
    }
    // This trait is public:
    pub trait SomePublicTrait {
        fn internal_ref(&self) -> &i32;
        fn internal_mut(&mut self) -> &mut i32;
    }
    impl SomePublicTrait for A {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    impl SomePublicTrait for B {
        fn internal_ref(&self) -> &i32 {
            &self.internal
        }
        fn internal_mut(&mut self) -> &mut i32 {
            &mut self.internal
        }
    }
    pub trait Trt {
        fn increment(&mut self);
        fn result(&self) -> i32;
    }
    impl<T: SomePublicTrait> Trt for T {
        fn increment(&mut self) {
            *self.internal_mut() += 1;
        }
        fn result(&self) -> i32 {
            *self.internal_ref()
        }
    }
    impl<T: SomePublicTrait> Dbl<T> {
        // And we use `SomePublicTrait` for code deduplication here,
        // but function `sum` is private anyway:
        fn sum(&self) -> i32 {
            self.one.result() + self.two.result()
        }
    }
    pub struct Foo {
        pub dbl_a: Dbl<A>,
        pub dbl_b: Dbl<B>,
    }
    impl Foo {
        pub fn product(&self) -> i32 {
            self.dbl_a.sum() * self.dbl_b.sum()
        }
    }
}

fn main() {
    use api::*;
    let mut dbl_a = Dbl {
        one: A::default(),
        two: A::default(),
    };
    dbl_a.one.increment();
    dbl_a.two.increment();
    dbl_a.two.increment();
    let mut dbl_b = Dbl {
        one: B::default(),
        two: B::default(),
    };
    dbl_b.one.increment();
    dbl_b.one.increment();
    let foo = Foo { dbl_a, dbl_b };
    println!("{}", foo.product());
}

(Playground)

The deduplicated function sum isn't public. So there's no need outside the api module to express bounds on types providing that function.

Yet the lint apparently forces us to make SomePublicTrait public. But do we really need to make it public in that case? Apparently not, as we can do:

         pub one: T,
         pub two: T,
     }
-    // This trait is public:
-    pub trait SomePublicTrait {
+    trait Internal {
         fn internal_ref(&self) -> &i32;
         fn internal_mut(&mut self) -> &mut i32;
     }
-    impl SomePublicTrait for A {
+    impl Internal for A {
         fn internal_ref(&self) -> &i32 {
             &self.internal
         }
@@ -24,7 +23,7 @@
             &mut self.internal
         }
     }
-    impl SomePublicTrait for B {
+    impl Internal for B {
         fn internal_ref(&self) -> &i32 {
             &self.internal
         }
@@ -36,7 +35,7 @@
         fn increment(&mut self);
         fn result(&self) -> i32;
     }
-    impl<T: SomePublicTrait> Trt for T {
+    impl<T: Internal> Trt for T {
         fn increment(&mut self) {
             *self.internal_mut() += 1;
         }
@@ -44,10 +43,12 @@
             *self.internal_ref()
         }
     }
-    impl<T: SomePublicTrait> Dbl<T> {
-        // And we use `SomePublicTrait` for code deduplication here,
-        // but function `sum` is private anyway:
-        fn sum(&self) -> i32 {
+    // Note that this trait is private:
+    trait SumViaTrait {
+        fn sum_via_trait(&self) -> i32;
+    }
+    impl<T: Internal> SumViaTrait for Dbl<T> {
+        fn sum_via_trait(&self) -> i32 {
             self.one.result() + self.two.result()
         }
     }
@@ -57,7 +58,7 @@
     }
     impl Foo {
         pub fn product(&self) -> i32 {
-            self.dbl_a.sum() * self.dbl_b.sum()
+            self.dbl_a.sum_via_trait() * self.dbl_b.sum_via_trait()
         }
     }
 }

(Playground)

Here sub_via_trait is private (as well as sum in the previous example), and we don't have SomePublicTrait in our published API anymore. We also don't use any tricks with public items in private modules.

So I think there's a solution for all cases. It just involves introducing and using traits (either public or private, depending on the application case). The latter feels a bit clunky in Rust sometimes, but the "pub use Trait in _" syntax may come in handy when dealing with a lot of traits. Hence why I now have in my real-life code:

/// Unnameable traits on data types for wildcard import
pub mod traits {
    pub use super::{Env as _, EnvBuilderOpen as _, Txn as _};
}

Btw, I hate that syntax. Maybe it would be nice if some types could automatically bring certain (sealed) traits into scope. But that would require to have sealed traits being part of the language first.

1 Like