[Pre-RFC] Warn about inaccessible types in public function signatures

[withoutboats]

Projects like Rayon can then use pub(crate) instead of the public / private hack.

I think Rayon is actually doing this intentionally - they want the trait to be public but not implementable. There may be a better way to express this, but basically crates like rayon are exploiting this gap right now.

There is another use case for the current system: you want to hide were the item is actually defined, but re-export it publicly at a different point. A lot of crates do this to make their public API surface smaller than their internal module structure.

[edmccard]

any time I want to re-export a type upward (even if not all the way pub) I have to use pub right now, so a huge number of types are annotated pub in private modules.

The title of the RFC is unfortunately misleading – the goal is not to eliminate all uses of public structs in private modules, but only [quote] If a struct / enum is used in a public function (such as a argument or return type) [/quote]

where I assume “public function” means one that is accessible to external clients.

[glaebhoerl]

I think Rayon is actually doing this intentionally - they want the trait to be public but not implementable. There may be a better way to express this, but basically crates like rayon are exploiting this gap right now.

(FWIW the better way would be to allow private associated items (methods etc) in a trait, which would make it impossible for anyone without access to the private item to implement the trait, in much the same way as you can’t construct an instance of a struct without having access to its private fields. This is an awkward question syntactically because Rust already made the choice that the only visibility qualifier is pub (there is no priv) and that it’s not required on trait methods.)

[cuviper]

Maybe an attribute on the trait could flip it to opt-in pub?

[sharazam]

I think Rayon is actually doing this intentionally - they want the trait to be public but not implementable.

I edited the last section of my first post. First of all this should be introduced after pub(restricted) is stable - pub(restricted) also works for traits. It should be a WARNING, not an error (changed title). Rayon should use pub(crate) to limit the scope of their public types to the rayon crate.

This discussion was already done here: https://github.com/rust-lang/rust/issues/35005 and it ended with “won’t change because current system is working”. Yes, it’s working, but it’s not working nicely.

If the community still says “it’s ok”, we can end this discussion because it would lead nowhere. Exposing non-accessible items in public APIs is not a good design because it confuses outside library users (my opinion). Meanwhile you still want flexibility like exposing structs / traits within your crate, but not expose them in a public API. This could be solved by using pub(restricted).

Rayon, etc. did it this way because they had no other choice. There is simply no way to do it in stable yet. The logical decision would be to postpone this until after pub(restricted), otherwise it would warn needlessly in crates like this.

Again, there are several things I’ve changed in correction to my original post:

1. It’s a warning, not an error, it can be turned off.
2. Do this after pub(restricted) is stable and advise people to either check their libraries API or use pub(restricted)

pub should be handled like pub(absolutely_public).

[cuviper]

Rayon doesn’t want pub(crate) for those though. We want ParallelString to be globally usable, yet we want to control implementations so we can freely extend it.

[sharazam]

I have several questions:

1. What is the reason against putting ParallelString in a public module?
2. For people not too familiar with this, could you give an example of how you intend ParallelString to be used?
3. Does this only apply to implementations of traits? My original post was about this stuff in function signatures, not trait implementations.

[tom.prince]

It isn’t ParallelString that is private. It is a public trait in a public module. But it has a method that returns an unnameable type. Here is the implementation. PrivateMarker is a publicly unnameable type that is returned from a method of ParallelString. That means that means that the trait can’t be implemented outside of rayon, but is usable anywhere.

This means that new methods can be added to ParallelString, without needing to worry that dependent crates have implemented that trait on their own types (and thus be broken when new trait methods are added). The point of ParallelString is to add new methods to str, rather than to define a new type of thing, so it makes sense to be able to add stuff, as new uses occur.

Regarding (3), the type occurs in the method signature of a trait method (and incidentally in the trait implementation).

[cuviper]

Right - it’s a fully public trait apart from that one hidden method, which uses the pub-in-private trick to let us control implementations.

That said, I opened rayon#327 to make it so we can always implement the methods directly in the trait, with a blanket impl for anything meeting the constraints. So we should get the desired ability to add more methods without incurring a breaking change.

I still don’t have a good way to deal with rayon::str::Pattern. That’s not an API I really want users to call at all – they just need to know what types implement it to use with par_split. If we had stability attributes, I would mark it #[unstable] like std::str::pattern::Pattern. Or if we had the means to define private trait items, I would mark all of its methods private, so only its basic existence is known outside the crate.

[JeffBurdges]

I think rayon’s PrivateMarker sounds like a standard enough technique, so if this warning were added then we’d need #[private_allowed_in_public] for traits and types like PrivateMarker or whatever.

One could however reduce its necessity by supporting inherent impls for traits : https://github.com/rust-lang/rfcs/issues/1971 And conceivably a trait could be private while its inherent impls created public methods.

[sharazam]

Just asking, but what if you would do: pub(crate) trait ParallelString // pub(crate) trait Pattern? This way nobody outside of the crate can implement the trait, but the functions, etc can be public. The struct PrivateMarker should then also be pub(crate) to disallow other crates to use it.

Or am I thinking the wrong way?

[withoutboats]

I think the real problem with this proposal has to do with re-exports. Essentially what you want is to actually fully prove that every item used in the public API is actually publicly accessible, whereas right now we take a rather weak best effort approach. The text says that it should be accessible at its canonical path, but that’s pretty problematic - many items are only accessible through a pub use self::whatever::Whatever; type of re-export, and this is intentional.

But a check that accepts those re-exports would have to walk the entire module graph (which could be cyclic) looking for re-exports (which could be transitive). I think solving this perfectly is NP hard or something like that, which is why this incomplete solution was accepted right now.

A solution that I think would satisfy @sharazam’s intent and also be feasible to implement is this: if an item is marked pub, but is not accessible at its canonical name, there will be a warning. You can supress this warning by tagging the item allow(something_or_other), which you have to do if you’ve used a re-export to expose it and don’t want to expose it at its true name.

While that would make it less likely that errors like this occur, I’m not sure that most users would be happy having to add these annotations. I think a large portion of public items are only public through re-export. There’s a trade off here.

[nikomatsakis]

I think Rayon is actually doing this intentionally - they want the trait to be public but not implementable. There may be a better way to express this, but basically crates like rayon are exploiting this gap right now.

Rayon does both:

• We have traits that we want to be public, but not implementable.
• We also have types that have to be public because of the “public type in private API” rules, but where what we really want is pub(restricted).

[nikomatsakis]

A solution that I think would satisfy @sharazam’s intent and also be feasible to implement is this: if an item is marked pub, but is not accessible at its canonical name, there will be a warning. You can supress this warning by tagging the item allow(something_or_other), which you have to do if you’ve used a re-export to expose it and don’t want to expose it at its true name.

Yes, I am roughly in favor of such a lint, now that pub(restricted) is an option. Or at least I’d like to give it a try and see how it feels.

Another interesting thought experiment that @wycats proposed to me at one point: what if pub(export) meant public outside the crate, and pub meant pub(crate)? I feel like I would (in Rayon) very much appreciate the explicitness of that. Obviously not backwards compatible, but this intuition – that it’s worth calling out the “potentially semver breaking” sort of publicity – might inform other designs (or perhaps if we decided to proceed with some more revolutionary change with an epoch).

[cuviper]

We also have types that have to be public because of the “public type in private API” rules, but where what we really want is pub(restricted).

In the case of pub fn split<P: Pattern>, I don’t think pub(restricted) will let us restrict just Pattern, and we do want the fn itself to be fully public. So I think here we’d need a public trait with private or pub(restricted) methods.

[nikomatsakis]

Right, that falls in the first category (traits that “closed” to impls).

[nikomatsakis]

Ah, I see, well, potentially it falls into something a bit stronger – i.e., maybe we want to reserve the right to change the impl’s contents at will.

I’ve wondered from time to time what the semantics of a “private trait item” ought to be – clearly the method can’t be called from outside the given module, but can the trait even be implemented?

I’ve definitely wanted cases where the trait can be implemented, but the method cannot be called (mostly because it ought not to be). I suppose that can be simulated by having an argument with no public constructor or something like that.

[cuviper]

I’ve definitely wanted cases where the trait can be implemented, but the method cannot be called (mostly because it ought not to be). I suppose that can be simulated by having an argument with no public constructor or something like that.

It’s simpler if the private trait fn has a default implementation, so outsiders can implement just the public parts and be done. But if you really want users to implement something they can’t call, that’s weird.

[nikomatsakis]

It’s simpler if the private trait fn has a default implementation, so outsiders can implement just the public parts and be done. But if you really want users to implement something they can’t call, that’s weird.

It is a bit weird, but I feel like it comes up a lot. Basically I have “callback methods” that really ought not be called from outside of my code. But ultimately I guess it probably has an overall smoother meaning to say that anything you can implement, you can call. The interaction with defaults is interesting, and I had not thought much about it before.

[cuviper]

I guess there’s a distinction when considering other types that implement the trait. On a method the user implemented, they could always bypass such restrictions by writing a free fn and implementing the trait calling that, so they always have access to their own code. But it still might be desirable to disallow arbitrary calls to that trait fn on other types out of that user’s control.

IOW they may need visibility to implement it for their types, without getting access to it on other types.