Documenting and fixing resolve's notion of public/private

[alexcrichton]

Making an issue off of my email to the mailing list.

Currently today resolve is generally considered "broken", and I think that this would benefit from actually writing the rules down, writing lots of test cases and documentation, and then fixing resolve. To the best of my knowledge, these are the rules which resolve should follow for determining whether an item is public or private and whether a certain path should resolve or not:

1. All items are private by default, with the exception of struct fields.
   To flag an item as public, you use pub. Also pub is not allowed on impl
   or extern (visibility is determined by each item individually).
2. For a path which is a cross-crate reference, it is only valid of every
   item in the path is marked as pub.
3. A module may access any item from its immediate parent.
4. A module may access any item from its immediate children.
5. Any local-crate path which goes outside the bounds of 3/4 will only resolve
   if each component of the path is pub. For example I could reach into a
   child's private mod's pub items, but not the child's private mod's non-pub
   items.

I'm nominating for a backwards-compatibility milestone as there's probably lots of existing code which is doing the wrong thing by accident.

[bblum]

I stated my preference in #3505 but will restate here. I would like there to be a rule: "pub items in a non-pub mod are visible to the rest of the crate, but not visible to users outside the crate". This is necessary to finally seal the leakiness of the select trait abstraction, cf #5160.

AIUI condition 2 listed above would imply this rule.

[Aatch]

Ok, to give a concrete (if contrived example), lets say I have this:

pub mod inner_api_a {
   use super::inner_common_impl;
   pub fn api1() {
       inner_common_impl::common1()
   }
   pub fn api2() {
       inner_common_impl::common2()
   }
}

pub mod inner_api_a {
   use super::inner_common_impl;
   pub fn api1() {
       inner_common_impl::common3()
   }
   pub fn api2() {
       inner_common_impl::common4()
   }
}

mod inner_common_impl {
    fn common1() { }
    fn common2() { }
    fn common3() { }
    fn common4() { }
}

In this example, inner_common_impl is just some common, internal routines shared by multiple, otherwise-unrelated modules inside the crate. These internal routines have complex preconditions and are not suitable for public consumption.

The above will not compile, as inner_common_impl is not public, so the use statements fail, also telling me that inner_common_impl is private.

If I mark it as public (so now it is visible cross-crate) it still won't compile because none of the functions are marked public. The only solution there is marking the function as public, thus making it visible to the outside world.

There are some convolutions you can do regarding using pub use and similar, but none of them play well with the implicit mapping to files/folders, as you'd end up adding extra layers to your directory structure or putting #[path] attributes everywhere. Additionally, if you aren't in a position where you can justifiably re-structure the crate, then you are even more stuck (eg, libstd).

The thing is, internal to the crate, the visibility rules are fine, and that's what matters while compiling that crate. When it comes to users of the library, it only matters that they can't link against the symbol you want. This suggests an alternative mechanism for controlling "link-time" visibility might be in order.

By keeping the current visibility rules regarding pub/priv as existing in isolation, and adding an attribute-based system for controlling visibility for external users, it should keep the system simple and understandable.

For comparison, in C, everything in a library is exported by default, unless the function/variable is marked static, in which case it doesn't even escape the object it's in. This is similar to rust's pub/priv at the moment. However, both GCC and clang implement extensions that allow you to control the visibility at a library level, via attributes.

I'm proposing a #[visibility(<value>)] attribute, that is applied recursively. This would control the "link-time" visibility of items. For example, adding #[visibility(internal)] to a pub mod foo; would make that module public, but not actually exported from the crate. This would essentially make everything after resolution/privacy checking act as if it's private.

Another potential advantage is one that C++ benefits from the same system. Not exporting mountains of monomorphised symbols.

[alexcrichton]

Sub-bug of #6143

[catamorphism]

Meta-bug is milestoned. Denominating this one.

[alexcrichton]

Nominating for the "priority" label and also to de-milestone the metabug while milestoning this one instead.

[alexcrichton]

I just closed a few issues that I believe will be wontfix or fixed as determined by the outcome of this issue. After reviewing the various use-cases, I'm not entirely sure that my previous set of rules is actually the best thing to follow. Especially with @bblum's and @Aatch's comments, I would actually be in favor of @bblum's suggestion, written down as:

1. All items are private by default, with the exception of struct fields. To flag an item as public, you use pub. Also pub is not allowed on impl or extern (visibility is determined by each item individually).
2. For a path which is a cross-crate reference, it is only valid if every item in the path is marked as pub (including modules)
3. Inside a crate, you can use any member from any location if the destination is flagged as pub (regardless of intermediate visibility).
4. Inside a module, you can use anything within that module (regardless of being public or private).

I am coming to prefer these rules because not only are they simple to implement, but they are simpler to understand and also work with. If this set of rules is agreed upon, I will update the issue text at the top to avoid later confusion over what should get implemented.

[Kimundi]

1+ for those rules.

• Is it public? - Reachable anywhere in the crate
• Is every item on it's path public? - Reachable anywhere outside the crate.

[emberian]

Also +1 for those new rules. Much simpler than the initial ruleset, and makes it easy to express exactly what I want.

[huonw]

I think there are some more edge cases here (especially from the view of generating documentation). Currently:

• pub use allows one to reexport things that are otherwise inaccessible
• a public function can return a private struct (and this can even have public fields that are accessible externally)

e.g.

// other.rs
pub use self::bar::baz;

pub mod foo {

    struct Foo { x: int }
    pub fn mk_foo() -> Foo { Foo { x: 1 } }

}

mod bar { pub fn baz() { println("escaped!!") } }

extern mod other;

fn main() {
    // other::baz(); // fails with a link-time error, *not* a compile time one
    println!("{}", other::foo::mk_foo().x);
}

I imagine that tightening these would be required too.

[Kimundi]

Hm, in case of documentation I see basically two cases:

• The type returned from a function/ pub used is also externally visibly over its canonical path.
  Then both should link to the doc page of the type in the crate it lives in (might mean linking to docs of a different crate)
• The type returned from a function/ pub used is not externally visible over its canonical path.
  Then the doc tool should emit a "orphan" page for that type, only accessible from the pub use or function return value links. This page should not contain any mention of the canonical path of the type, because as far as the public api is designed, it doesn't have on, only explicit or implicit aliases to it. (This also means not encoding the path in the url)

Of course, the question is if we should allow returning not-externally-public types from externally-public functions at all.

If yes, then I claim the same rules should apply as if it where pub used or regularly public: All neccessary symbols get exported as public, things need to be documented if the lint is set for that, etc, you'd just not be able to refer to them with their canonical path, as not all layers in it would be public.

Also note that you might get the type of an return value with an hypothetical typeof operator in the future. (That's what this operator usually is for, right?)

[catamorphism]

Accepted for well-defined

[lilyball]

@alexcrichton I like the rules, but it's worth noting that these proposed rules means a mod tests cannot include unit tests for private items. Similarly, a module cannot privately provide common functionality to its sub-modules.

I seem to recall an earlier proposal was that a module had access to all of its direct parent's priv items as well. This would solve these concerns, though I don't know if it's worth the complexity.

[alexcrichton]

As an update on this, I was talking with @nikomatsakis and he's enlightened me as to what the intended privacy rules actually should be. The idea behind the intended rules were:

1. You can use anything pub from the top to the desired item.
2. You can use any priv item of yourself and your ancestors.

The first rule is fairly obvious, but the second rule is a little bit more subtle. What this means is that you can use any number of "helper modules" in your ancestry. It doesn't mean that you can use anything in a helper module, it just means that you can look into your ancestor's private modules. At that point anything you use must be pub all the way down.

This encompasses the mod test case because the relevant module you're testing is in your ancestry and you can access anything private from it. It also encompasses the "helper module" use case because the helper module is in the ancestry of all common modules.

I've got a branch-in-progress to refine the current system. My idea was to completely rip out privacy from resolve and put it all in the privacy passes. This should yield better error messages and be more maintainable in the future. I'm mostly doing this as a guinea-pig type situation to make sure that this works out.

I've come to like the second rule, though, as it makes sense to me and it seems to encompass almost all use cases.

[catamorphism]

This might fall under item 2. already, but I just noticed that if you do something like:

quux/lib.rs:

mod foo;

quux/foo.rs:

impl Bar {
    pub fn whatever() // ...
}

and then in a different crate foo, if you write:

extern mod quux;

...x.whatever()...

This will successfully compile, when it shouldn't. But if you run the executable compiled with foo, you get a dynamic link error since the symbol whatever isn't known. Instead, it should be a compile-type error saying that quux's foo module is private.

[nikomatsakis]

@catamorphism why do you say that x.whatever() should not work? I at least expect pub methods to be available from any module -- they don't fall under the same lexical scope rules (note that you don't have to import them either)

[catamorphism]

@nikomatsakis That seems weird to me (I would expect that nothing declared in a private module would be visible from another crate), but I wouldn't object to it. In that case, of course, the result should run without a dynamic loader error :-)

[nikomatsakis]

@catamorphism yes, certainly. As for the matter of priv vs pub, I can certainly see your reasoning, it's an interesting difference between methods and items I hadn't thought too about before. From the standpoint of the *module that declares the method/item, there is little difference: priv means "only usable from code within this module or submodules" and pub means "usable by outside code". But from the POV of some outer module, the visibility of items can be further controlled, whereas methods can not. I'm not sure if this is a bad thing, but in any case I don't know if anything can be done about it -- since methods are not imported, so there is no obvious "path" to the private module that we can check (it might, for example, be exported via a pub use etc).

[lilith]

So is it correct that if you implement a public function on a private struct, and call the function from an external crate, the compiler tells you that the function is private? Or should the compiler clarify that the struct is private?