private type in public API of non-pub mod is allowed

[pnkfelix]

While reviewing the list of active feature gates and double-checking the behavior of visible_private_types, I found that the check is not working for e.g. the program below:

mod a {
    #[derive(Debug)]
    struct S { x: i32 }
    pub fn foo() -> S { S { x: 3 } }
    impl S { }
}

mod b {
    use a;

    #[cfg(expected)]
    pub fn call_foo() {
        let s: a::S = a::foo();
        println!("{:?}", s);
    }

    #[cfg(not(expected))]
    pub fn call_foo() {
        let s = a::foo();
        println!("{:?}", s);
    }
}

fn main() {
    b::call_foo();
}

In the playpen, the above compiles and runs. I expected to see an error due to the type of a::foo returning the private type a::S.

[pnkfelix]

cc #16463

[pnkfelix]

nominating, suggest 1.0 polish (though 1.0 beta might be warranted).

[pnkfelix]

(also, its possible I missed some discussion where we decided to remove the check here; but I would have imagined such a change would have gotten more attention.)

[pnkfelix]

Reading over the original PR #17401 i noticed that it seems like there was no compile-fail test covering return types of top-level functions (even though that was the reference example given in the description for the PR). (Is it possible that this case was not actually implemented? Seems super unlikely, but ...)

Still, RFC 136 seems to clearly state that this case should be disallowed.

[nagisa]

This sometimes causes odd linking errors such as #20201 (comment)

[pnkfelix]

P-backcompat-lang, 1.0 beta

[nikomatsakis]

Amusing.

[bombless]

Amusing, truly.

I thought it is intended, and I'm using this "feature".
https://github.com/bombless/structures-generator/blob/master/src/utils.rs
I guess I just think it's soooo useful that I don't bother to report it.

God I was trying to use my lovely utils.rs as extern crate any I found a crash of cargo.
Nice bazinga.

Just made my day.

[Gankro]

You guys, there's a tag for that.

come on, there's a system

[nikomatsakis]

Assigning to @nrc in the interest of having as little unassigned work for 1.0 beta as possible.

[nrc]

The error doesn't seem to be anything to do with the return type. The check for private types in public interfaces only occurs if the module is itself public. I.e., changing mod a to pub mod a in the original example reports the errors correctly.

I think this is still a bug, because I would expect there to be an error even if the module is private, since (as the example shows) it can cause incorrect exposure of internals.

[nikomatsakis]

@nrc indeed, that seems wrong to me.

[nrc]

A problem that has come up with this is that my fix breaks the following pattern:

struct PrivateContext;

mod internals {
    pub fn entry_point(x: super::PrivateContext) { ... }
}

fn main() {
    let pc = PrivateContext;
    internals::entry_point(pc);
}

This used to be Ok, because internals is a private module. But now we check it and find the non-pub type PrivateContext in the type of a public function, and we error. Intuitively, this is Ok, because we are not extending the visibility of PrivateContext (since it is a private module, the pub fn is only exposed in the same scope as PrivateContext). However, I can't find a straightforward way to formalise the intuition of when this is Ok (and satisfy the other cases for this check).

[nrc]

I should add that there is no easy work around - we can't make entry_point private because we want to call it from the outer module. And we don't want to make PrivateContext public because it should not be used outside of the outer module.

[nrc]

I think the property here is, if a type is used in a public item, then either that type must be public or the 'last private module' from that item must not enclose the type. Where "last private module" is the first non-pub module we encounter if we walk up the module tree from the public item to the crate root ("last" comes from walking from the crate root down).

Note that it is always safe to use a public type in a public item, if the type is inaccessible due to a private module, then so will the item be.

Note two: I think traits and impls count as modules for the above purpose.

Note three: I believe this rule may be equivalent to the 'earlier, more complex' rule discussed in the alternatives to RFC 136. Maybe I should give up and start sprinkling pub around...

[nikomatsakis]

It took me a while to digest the def'n there but I agree it is good. One thing I think we have to do is re-check pub use items, so that the following is illegal:

pub mod x {
    struct Priv;

    mod helper {
        pub fn f(_: Priv) { }
    }

    pub use self::helper::f; // Error here!
}

The idea would be that you re-check the pub use'd item, but with the notion of the "current module" changed to be relative to the pub use, not the point of declaration.

[nikomatsakis]

Key insight from IRC: the last private module in @nrc's definition is the "smallest opaque container" -- hence you require it be pub (and hence authorized to escape out of this container) or else that it already come from outside the container.

[nikomatsakis]

Also, per IRC discussion, @nrc and I more-or-less agreed that his proposed semantics seems like an improvement on the RFC, but that for now we could probably implement the RFC as specified, and come back to this extension.

[bombless]

http://is.gd/7IFKSB
I think this is a similar problem (or even not a problem?)