Leaked private struct in public function

[conradludgate]

Bug Report

Version

0.4.3

Platform

N/A

Description

In the add_service function on Router, you are retuning a new Router which uses Or as a type parameter. While Or itself is a public struct, its only public inside its module, since the module it lives in is private.

On the Rust Discord, they suggested that this really is a bug in Rust itself, but it would be a breaking change to fix so it's probably expected in Rust 2024... Regardless, it should be fixed. I'm not sure if we just want to make the module public or re-export the struct

[davidpdrsn]

What is your use case that made you run into this?

[conradludgate]

I'm trying to replicate https://github.com/hashicorp/go-plugin in rust. It's a wrapper around gRPC. At present, The Router methods (notably, add_service and serve/serve_with_shutdown) are not implemented in a trait, and since the type signature ends up being behind a private module, it's impossible to implement with tonic. I forked the project and made Or accessible and I'm now able to implement something like https://github.com/conradludgate/plugin-rs/blob/main/src/server/mod.rs

[LucioFranco]

The reason it was kept private is that it has the potential to go away and change. The router type likely not but I didn't want to export unnecessary types. For your use case (which I think is pretty unique) you may be better off just vendoring the router code yourself. I know its not ideal but I am not sure if I want to expose a type publicly when its a type that wasn't meant to be public. I actually would like to reduce some of the surface area of the transport module anyways and I am not sure if this leads us down that path.

[conradludgate]

In that case, I think i would recommend router methods be implemented on a Trait, and the return type of add_service would be impl Trait. That way they stay private but still solve the problem of not being nameable

[LucioFranco]

I am pretty sure if we moved it to a trait it would likely make the code more confusing to consume for most users? Traits generally produce tricky error messages and that is something I want to avoid. Tonic actually tries to avoid traits at the high level types just to avoid any confusion. Obviously there is only so much we can do without traits but that was an original design goal.

That said, I am happy to explore what you are suggesting I just don't want to go at the expense of users that may not be familiar with Rust and traits.

[conradludgate]

Yeah I totally understand. When I have some time, I'll open a new PR that makes use of the traits method and I'll also generate some docs to see what it looks like. Then it can be decided whether it's a good pattern or not for tonic.

As present, I believe that the code that is in tonic today should not compile. I have raised it as a bug in Rust since in most other scenarios, it should not be possible to return such a type (and is an error that you do get sometimes)

[cab]

This also seems to be the case for ServerReflectionServer and ServerReflection in tonic-reflection: https://docs.rs/tonic-reflection/0.1.0/tonic_reflection/server/struct.Builder.html#method.build.

[cab]

@LucioFranco I have an example branch here that uses impl trait without also introducing any new traits, with passing tests: https://github.com/hyperium/tonic/compare/master...cab:cab/public-structs?expand=1

Would you be open to a PR for something like this?

[LucioFranco]

I would assume making that change is breaking but I don't quite understand if the impl Trait is better than what we have already? We could always expose that server type though.

[cab]

I don't think it's a breaking change (at least for Or), because you can't access that type/value anyways. But I'm not 100% sure -- I obviously defer to you here :).

I don't personally have a strong opinion on using impl Trait vs exposing the type. My use case is similar to @conradludgate -- I'm trying to make something that's effectively a wrapper around tonic and need to use Or (and ServerReflectionServer) in the return type.

[LucioFranco]

You may want to consider just copying the implementation of the route, I believe you can vendor it and have the same feature set without needing to tie to tonic. We will likely switch routers at some point.