A private module can be used outside its containing module

[jseyfried]

Consider the following code:

mod foo {
    mod bar { // `bar` should only be visible inside `foo`
        pub struct S;
    }
    pub use self::bar::S;
}

impl foo::S {
    fn f() {}
    pub fn g() {}
}

fn main() {
    foo::bar::S::f(); //< yet this line compiles
    // foo::bar::S::g(); //< this line would give the correct privacy error
}

[jseyfried]

Here is an example without an inherent impls:

mod foo {
    mod bar { // `bar` should only be visible inside `foo`
        pub use baz;
    }
}

pub mod baz {
    fn f() {}

    fn g() {
        ::foo::bar::baz::f(); //< yet this line compiles
    }
}

If baz::f were pub, we would get the correct privacy error.

[jseyfried]

A path P appearing in a module M is valid w.r.t. visibility iff for each segment s of P, the item to which s resolves is visible to M.

Since pub items are visible everywhere, it is sufficient to only consider the segments of P that resolve to non-pub items.

Most of the time, the visibilities of these non-pub items are monotonically decreasing along the path. If we assume this, it is sufficient to only consider last segment of P that resolves to a non-pub item since this item will have the most restricted visibility. This is what we do now with LastPrivate.

However, our approach in insufficient if this assumption does not hold, i.e. if a path has a non-pub module followed by a non-pub item not contained in the module (so that it is not less visible).

We will need to either give the privacy checker more information than just the LastPrivate of each PathResolution or we will have to move the privacy checking of paths into resolve. I prefer the latter.

[jseyfried]

cc @nikomatsakis @nrc @petrochenkov

[petrochenkov]

We will need to either give the privacy checker more information than just the LastPrivate of each PathResolution or we will have to move the privacy checking of paths into resolve. I prefer the latter.

Having the information about the way in which a particular name was introduced (directly, or through a specific use alias) would be very helpful for later stages of compilation, for many things beyond privacy - error messages, stability/deprecation checks, probably something else.
It doesn't prevent moving parts of PrivacyChecker to the name resolution stage though.

[jseyfried]

Definitely agree that it would a good for the rest of the compiler to know what item a path resolves to before following use aliases. I don't think that is enough information to fix this bug, though, especially not after RFC 1422.

[jseyfried]

This can also allow access to a private trait:

mod foo {
    trait T { // This should be private to `foo`
        type Assoc;
    }

    impl T for () {
        type Assoc = super::S;
    }
}

struct S;
impl S {
    fn f() {}
}

fn main() {
    <() as foo::T>::Assoc::f(); // but it can be used here
}

[petrochenkov]

PrivacyChecker is pretty buggy, the last autumn I had time to audit other parts of rustc_privacy, but not PrivacyChecker unfortunately.

[jseyfried]

I think this is more of a problem with the data (or lack thereof) that we are giving to PrivacyChecker rather than a bug in PrivacyChecker itself. The Path part of the qualified path is foo::T::Assoc::f, and all PrivacyChecker is given is the last private segment, f, which is visible.

[jseyfried]

PrivacyChecker does look like it has a lot of unnecessary complexity, though.

Speaking of bugs in the privacy checker, do you think this should be a privacy error?

mod foo {
    mod bar {
        pub trait Parent {
            fn f(&self) {}
        }
    }
    pub trait Child: bar::Parent {}
}

fn f<T: foo::Child>(t: T) {
    t.f(); // Right now, this line causes an error that `Parent` is inaccessible since `bar` is private
}

I don't think it should since the trait and method are pub so should be visible everywhere by RFC 1422. I was a little surprised that we allowed methods from ancestors of parameter bounds, but that's probably not up for debate at this point.

[petrochenkov]

do you think this should be a privacy error?

That's a philosophical question!
I don't think it should be an error. Things accessed not directly by paths, but by field/method access with help of values and type inference - fields, inherent methods, trait methods, are normally checked only for the presence of pub.
So, for f() to be accessible from module m, Parent should either live in m or some of m's parents, or should be marked with pub.

[petrochenkov]

Oh, and pub on a trait method is inherited from its trait, since trait items can't have their own publicity.

[jseyfried]

Ok, that will make fixing #18241 easier. The fundamental problem is that the core primitive in PrivacyChecker that determines if an item is visible in a module, def_privacy, checks for nameability when it should be using RFC 1422 compatible visibility, i.e. anything pub is visible.

Nameability only matters for paths and PrivacyChecker only checks the LastPrivate of a path for which, like all private items, visibility and nameability are the same.

[jseyfried]

The only reason why this is not more of a problem right now is that the rest of PrivacyChecker avoids calling def_privacy on some pub items (like impl items) on what appears to be a case by case basis.

[petrochenkov]

I'm pretty sure #18241 can be fixed trivially by reapplying #28504, public traits can't inherit from private traits anymore, so the reason why #28504 was reverted is not actual now.
If a trait is in scope (it's required for calling its methods), then this trait itself and its parents are guaranteed to either be public or live in the same/outer module.

[jseyfried]

Good point, but sometimes it's only a public_in_private warning and there are cases where reapplying #28504 would turn correct hard errors into warnings. This might be acceptable.