Closed jseyfried closed 8 years ago
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.
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.
cc @nikomatsakis @nrc @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.
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.
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
}
PrivacyChecker
is pretty buggy, the last autumn I had time to audit other parts of rustc_privacy
, but not PrivacyChecker
unfortunately.
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.
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.
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
.
Oh, and pub
on a trait method is inherited from its trait, since trait items can't have their own publicity.
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.
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.
I'm pretty sure #18241 can be fixed trivially by reapplying https://github.com/rust-lang/rust/pull/28504, public traits can't inherit from private traits anymore, so the reason why https://github.com/rust-lang/rust/pull/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.
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.
Consider the following code: