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

[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]

@nikomatsakis

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]

@cuviper

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.

[sharazam]

I just noticed the following, which I found a bit weird:

pub trait MyTrait {
    pub fn my_function;
}

This is not allowed currently - rust complains about the pub modifier in front of the fn my_function:

error: expected one of `const`, `extern`, `fn`, `type`, or `unsafe`, found `pub`

Why not rethink this a bit - this is just my idea:

pub trait { pub fn } means public trait and method can be implemented from outside the crate

pub trait { fn } means that the trait is public and can be implemented, but the function cannot be implemented. If all functions of a trait are private (to the trait), then the trait itself is public, but it can't be implemented from outside the current library.

trait { pub fn } is an error, can't leak public implementable function from crate-internal trait

trait { fn } - both trait and function are only public to the crate they're in.

I have also thought of using pub(trait) modifier as an extension to restricted types.

When there is a pub in front of a trait, it should be documented, when there is a pub in front of a fn, it should be documented.

I think this solves the problem for Rayon:

@cuviper

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

This would result in:

pub trait Pattern {
    fn find_in() { } /* find_in is not marked pub and cannot be called outside of rayon*/
}

Additionally, find_in would be hidden from the documentation. This is also something I would like to adress - rustdoc should differentiate between the current crate and external crates. If you are working with your own library, it is useful to document private functions, but if other people use your library then you may not want private functions to be documented. Just a thought.

[hanna-kruppe]

@sharazam

Why not rethink this a bit - this is just my idea:

This is super backwards incompatible. Since pub trait { pub fn } is currently illegal, every single public trait would be broken by this, which is probably too much breakage even for Rust 2.0 / future epochs / whatever solution we'll settle on for breaking changes.

Aside from that, I don't particularly like the thought of making trait contents private by default,even in a vacuum. Keep in mind that I'm probably unconsciously biased towards the status quo, but hear me out: Unlike functions in modules and inherent impls, trait methods are almost always intended for wide consumption outside of the current module (if the trait is public). Even what rayon does is only about implementing the trait, not (primarily) about access to methods. It feel more like a hack, an abuse of the privacy system rather than a first-class use case (of course, that doesn't mean it shouldn't be possible, especially when there's no other option currently, but it shouldn't unduly influence the language design). Trait items being public by default meshes well by that, despite being inconsistent with items everywhere else.

[sharazam]

@hanna-kruppe exactly, I expected this answer. Which is why I thought about pub(trait). The rules would then be:

pub trait { pub fn } || trait and method can be implemented outside the crate pub trait { pub(trait) fn } || fn is private to the trait. This is what rayon, etc. should use pub trait { fn } || currently in use. compiler makes this to: pub trait { pub fn }, see above

trait { pub fn } || Error. This doesn’t break crates since it is currently illegal trait { pub(trait) fn } || trait is private to the crate, fn is private to the trait. trait { fn } || currently in use, behaviour stays as it is. compiler makes this to: pub(super) trait { pub(super) fn } or looks if trait has been exported or trait has any other restricted items on it, then copies those to the fn

This design would not break any crates. Documentation is a different topic (what public / private) methods should be documented / hidden. If this is done, the compiler could lint these public-in-private hacks and it could warn effectively for private types in public APIs. Essentially pub(trait) is like “public, but not implementable”.

This is just an idea, but I couldn’t really think of any other way to solve this. People who really want to use the public-in-private thing can disable the warning.

[cuviper]

It seems like a reasonable compromise to keep the default-pub visibility in traits, but still allow pub(restricted) to change it. A normal private item is equivalent to pub(self).

It would remain compatible for existing code that doesn’t (can’t) annotate publicity, and code that starts using it will require a new Rust like any other new feature.