New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Private trait's methods reachable through a public subtrait #28514

Closed
bluss opened this Issue Sep 19, 2015 · 29 comments

Comments

Projects
None yet
10 participants
@bluss
Contributor

bluss commented Sep 19, 2015

I'm not 100% sure how the privacy rules should work.

A's methods should not be callable because the trait is private, but it's reachable though the trait C that inherits A, by calling C::a().

B's methods are reachable the same way, even if it's marked pub but not publically reachable.

pub use inner::C;

mod inner {
    trait A {
        fn a(&self) { }
    }

    pub trait B {
        fn b(&self) { }
    }

    pub trait C: A + B {
        fn c(&self) { }
    }

    impl A for i32 {}
    impl B for i32 {}
    impl C for i32 {}
}

fn main() {
    // A is private
    // B is pub, not reexported
    // C : A + B is pub, reexported

    // 0.a(); // can't call
    // 0.b(); // can't call
    0.c(); // ok

    C::a(&0); // can call
    C::b(&0); // can call
    C::c(&0); // ok
}

Playpen

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Sep 19, 2015

Contributor

cc @alexcrichton, because you've weighed in on other privacy issues

Contributor

bluss commented Sep 19, 2015

cc @alexcrichton, because you've weighed in on other privacy issues

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 19, 2015

Member

I would personally expect a privacy error to be generated. I'm also a little surprised about how resolution allows using a method without importing the trait as well, but that may perhaps be intended behavior.

cc @rust-lang/lang, @nrc

triage: I-nominated

Member

alexcrichton commented Sep 19, 2015

I would personally expect a privacy error to be generated. I'm also a little surprised about how resolution allows using a method without importing the trait as well, but that may perhaps be intended behavior.

cc @rust-lang/lang, @nrc

triage: I-nominated

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Sep 19, 2015

Contributor

C::b being callable seems to suggest b is a method in the C trait, "inherited", but .b() not being callable as a method when C is in scope is a counterargument.

Contributor

bluss commented Sep 19, 2015

C::b being callable seems to suggest b is a method in the C trait, "inherited", but .b() not being callable as a method when C is in scope is a counterargument.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 21, 2015

Contributor

I think the error here is that pub trait C: A + B ought to require that A is public.

Contributor

nikomatsakis commented Sep 21, 2015

I think the error here is that pub trait C: A + B ought to require that A is public.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 22, 2015

Member

public or accessible from all the places C is? If it is in another module, it could be public but we could still have this problem (with a more complex example). The alternative feels hard to check though.

Member

nrc commented Sep 22, 2015

public or accessible from all the places C is? If it is in another module, it could be public but we could still have this problem (with a more complex example). The alternative feels hard to check though.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 22, 2015

Member

I'd be fine so long as it was limited to "marked pub" because that's generally how the public/private check works in terms of what you can export.

Member

alexcrichton commented Sep 22, 2015

I'd be fine so long as it was limited to "marked pub" because that's generally how the public/private check works in terms of what you can export.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 1, 2015

Contributor

I am somewhat surprised, but this test case passes too. The difference here is that the type I32 is private, so I expect the impl to be in error.

mod inner {
    struct I32;

    trait A {
        fn a(&self) { }
    }

    pub trait B {
        fn b(&self) { }
    }

    pub trait C: A + B {
        fn c(&self) { }
    }

    impl A for I32 {}
    impl B for I32 {}
    impl C for I32 {} // <-- I expected an error here and on the impl of B
}

fn main() {
}
Contributor

nikomatsakis commented Oct 1, 2015

I am somewhat surprised, but this test case passes too. The difference here is that the type I32 is private, so I expect the impl to be in error.

mod inner {
    struct I32;

    trait A {
        fn a(&self) { }
    }

    pub trait B {
        fn b(&self) { }
    }

    pub trait C: A + B {
        fn c(&self) { }
    }

    impl A for I32 {}
    impl B for I32 {}
    impl C for I32 {} // <-- I expected an error here and on the impl of B
}

fn main() {
}
@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Oct 1, 2015

Member

(I'm inclined to think that the invocation C::a(&0) from the original example is still problematic, independently of the example @nikomatsakis gave above.)

Member

pnkfelix commented Oct 1, 2015

(I'm inclined to think that the invocation C::a(&0) from the original example is still problematic, independently of the example @nikomatsakis gave above.)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Oct 1, 2015

Member

(namely, if one changes @nikomatsakis example so that it says pub struct I32;, then we would still have the issue of whether 1. one can have the declaration of pub trait C: A { ... } and 2. whether one can call the methods from the private trait A on a type where only the public trait C is available for use.)

Member

pnkfelix commented Oct 1, 2015

(namely, if one changes @nikomatsakis example so that it says pub struct I32;, then we would still have the issue of whether 1. one can have the declaration of pub trait C: A { ... } and 2. whether one can call the methods from the private trait A on a type where only the public trait C is available for use.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 1, 2015

Contributor

The heart of this question is what a private trait means. After some discussion in the language team, we came to the following conclusions:

  • To be consistent with the rest of the rules, private types and traits should not appear in the where-clauses (or supertraits) of public traits. This would make pub trait B: A an error. I believe this is the in the spirit of the relevant RFC, I've not reviewed the precise wording.
  • Furthermore, if we ever were to relax this rule, we would want to define when you can call a method on a private trait. It might make sense to make that illegal outside of the module that defined the trait.

There is definitely concern that people are going to be relying on this kind of setup, though also concern that they may not be getting the guarantees that they think they are.

Contributor

nikomatsakis commented Oct 1, 2015

The heart of this question is what a private trait means. After some discussion in the language team, we came to the following conclusions:

  • To be consistent with the rest of the rules, private types and traits should not appear in the where-clauses (or supertraits) of public traits. This would make pub trait B: A an error. I believe this is the in the spirit of the relevant RFC, I've not reviewed the precise wording.
  • Furthermore, if we ever were to relax this rule, we would want to define when you can call a method on a private trait. It might make sense to make that illegal outside of the module that defined the trait.

There is definitely concern that people are going to be relying on this kind of setup, though also concern that they may not be getting the guarantees that they think they are.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 1, 2015

Contributor

triage: P-high

Contributor

nikomatsakis commented Oct 1, 2015

triage: P-high

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Oct 23, 2015

Contributor

This variation shows how using a trait bound with C makes all the methods from A, B, C directly callable, while just use inner::C; (in the original report's code) only enables the methods from C and not A nor B.

fn use_c<T: C>(x: &T) {
    // A is private
    // B is pub, not reexported
    // C : A + B is pub, reexported

    x.a(); // can call
    x.b(); // can call
    x.c(); // ok

    C::a(x); // can call
    C::b(x); // can call
    C::c(x); // ok
}

a is still callable both ways in nightly — is so it's not limited to just UFCS?!

Contributor

bluss commented Oct 23, 2015

This variation shows how using a trait bound with C makes all the methods from A, B, C directly callable, while just use inner::C; (in the original report's code) only enables the methods from C and not A nor B.

fn use_c<T: C>(x: &T) {
    // A is private
    // B is pub, not reexported
    // C : A + B is pub, reexported

    x.a(); // can call
    x.b(); // can call
    x.c(); // ok

    C::a(x); // can call
    C::b(x); // can call
    C::c(x); // ok
}

a is still callable both ways in nightly — is so it's not limited to just UFCS?!

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Oct 23, 2015

Contributor

The last comment shows a regression in nightly. x.a() compiles in nightly, used to be a privacy error.

Contributor

bluss commented Oct 23, 2015

The last comment shows a regression in nightly. x.a() compiles in nightly, used to be a privacy error.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Oct 23, 2015

Contributor

Unfortunately, the current assignee (@aturon) is out for a while, so someone else will have to look into this.

Contributor

nikomatsakis commented Oct 23, 2015

Unfortunately, the current assignee (@aturon) is out for a while, so someone else will have to look into this.

@arielb1 arielb1 changed the title from Private trait's methods reachable through UFCS to Private trait's methods reachable through a public supertrait Oct 23, 2015

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 18, 2015

Contributor

Is this on beta/stable yet?

Contributor

brson commented Nov 18, 2015

Is this on beta/stable yet?

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Nov 18, 2015

Contributor

The regression mentioned in this comment seems to be fixed in nightly (and never reached beta). I think it was fixed in #29325

Contributor

bluss commented Nov 18, 2015

The regression mentioned in this comment seems to be fixed in nightly (and never reached beta). I think it was fixed in #29325

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jan 14, 2016

Member

@petrochenkov Are you interested in taking this on?

Member

aturon commented Jan 14, 2016

@petrochenkov Are you interested in taking this on?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jan 14, 2016

Contributor

@aturon
I'm very much interested in fixing the rest of privacy bugs and then laying the road to rust-lang/rfcs#1422, but the main problem is time now. It will be clear in the next few weeks how much resources I'll be able to spend on it.

Contributor

petrochenkov commented Jan 14, 2016

@aturon
I'm very much interested in fixing the rest of privacy bugs and then laying the road to rust-lang/rfcs#1422, but the main problem is time now. It will be clear in the next few weeks how much resources I'll be able to spend on it.

@arielb1 arielb1 changed the title from Private trait's methods reachable through a public supertrait to Private trait's methods reachable through a public subtrait Jan 15, 2016

@jseyfried

This comment has been minimized.

Show comment
Hide comment
@jseyfried

jseyfried Feb 25, 2016

Contributor

If I understand correctly, this is not an issue after #29973 given #![deny(private_in_public)]. Should we still disallow this with more than the private_in_public lint?

Contributor

jseyfried commented Feb 25, 2016

If I understand correctly, this is not an issue after #29973 given #![deny(private_in_public)]. Should we still disallow this with more than the private_in_public lint?

@jseyfried

This comment has been minimized.

Show comment
Hide comment
Contributor

jseyfried commented Apr 27, 2016

@brson brson added the P-medium label Jul 14, 2016

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 14, 2016

Contributor

Triage: Moving this to P-medium since nobody is working on it. cc @rust-lang/lang @petrochenkov

Contributor

brson commented Jul 14, 2016

Triage: Moving this to P-medium since nobody is working on it. cc @rust-lang/lang @petrochenkov

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 14, 2016

Member

also ping @jseyfried, if this is no longer an issue, could you provide a summary as to why as well? It'd be great to close out!

Member

alexcrichton commented Jul 14, 2016

also ping @jseyfried, if this is no longer an issue, could you provide a summary as to why as well? It'd be great to close out!

@jseyfried

This comment has been minimized.

Show comment
Hide comment
@jseyfried

jseyfried Jul 14, 2016

Contributor

@alexcrichton I believe this issue can only happen if a public trait has a private parent, which is already a private_in_public future incompatiblity warning.

Contributor

jseyfried commented Jul 14, 2016

@alexcrichton I believe this issue can only happen if a public trait has a private parent, which is already a private_in_public future incompatiblity warning.

@alexcrichton alexcrichton removed the P-high label Jul 14, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 14, 2016

Member

Awesome! I @rust-lang/lang, thoughts about closing? (also applying a nomination for bringing it up)

Member

alexcrichton commented Jul 14, 2016

Awesome! I @rust-lang/lang, thoughts about closing? (also applying a nomination for bringing it up)

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jul 14, 2016

Contributor

I think this can be requalified to E-needtest and closed when #34206 lands. I should probably add this test to #34206 itself (EDIT: done).

Contributor

petrochenkov commented Jul 14, 2016

I think this can be requalified to E-needtest and closed when #34206 lands. I should probably add this test to #34206 itself (EDIT: done).

petrochenkov added a commit to petrochenkov/rust that referenced this issue Jul 14, 2016

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jul 14, 2016

Member

Switching to needstest!

Member

aturon commented Jul 14, 2016

Switching to needstest!

@bors bors closed this in b052dd6 Aug 14, 2016

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Apr 11, 2017

Contributor

Reopening this since private_in_public was reverted to warn-by-default and is not going to become a hard error.

Contributor

petrochenkov commented Apr 11, 2017

Reopening this since private_in_public was reverted to warn-by-default and is not going to become a hard error.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Apr 11, 2017

Contributor

Visibility of a trait item is inherited from its trait, so the fix is to check this trait visibility every time a trait item is named.

C::a(x); // ERROR, trait A is private / not accessible from here
Contributor

petrochenkov commented Apr 11, 2017

Visibility of a trait item is inherited from its trait, so the fix is to check this trait visibility every time a trait item is named.

C::a(x); // ERROR, trait A is private / not accessible from here
@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Apr 11, 2017

Contributor
C::b(&0);

is an orthogonal problem.
UFCS RFC says that UFCS works only with traits in scope, exactly like method calls, and B is not in scope, so this should supposedly be a method resolution error.

C::b(&0); // ERROR no method b in the current scope

EDIT: This interpretation incorrect, I didn't realize this is about C::b and not i32::b. C::b is OK.
C in this example is a trait object type, not a trait, and trait objects are special cased in associated item resolution, and items of their corresponding traits and super-traits are treated as inherent. That's why C::b doesn't require B to be in scope, because it's treated as inherent.

Contributor

petrochenkov commented Apr 11, 2017

C::b(&0);

is an orthogonal problem.
UFCS RFC says that UFCS works only with traits in scope, exactly like method calls, and B is not in scope, so this should supposedly be a method resolution error.

C::b(&0); // ERROR no method b in the current scope

EDIT: This interpretation incorrect, I didn't realize this is about C::b and not i32::b. C::b is OK.
C in this example is a trait object type, not a trait, and trait objects are special cased in associated item resolution, and items of their corresponding traits and super-traits are treated as inherent. That's why C::b doesn't require B to be in scope, because it's treated as inherent.

arielb1 added a commit to arielb1/rust that referenced this issue Apr 25, 2017

Rollup merge of #41332 - petrochenkov:privti, r=eddyb
Check privacy of trait items in all contexts

Fixes rust-lang#28514

This is a sufficiently rare scenario and it's currently guarded by `private_in_public` lint, so it shouldn't be a [breaking-change] in practice.

bors added a commit that referenced this issue Apr 25, 2017

Auto merge of #41332 - petrochenkov:privti, r=eddyb
Check privacy of trait items in all contexts

Fixes #28514

This is a sufficiently rare scenario and it's currently guarded by `private_in_public` lint, so it shouldn't be a [breaking-change] in practice.

bors added a commit that referenced this issue Apr 25, 2017

Auto merge of #41332 - petrochenkov:privti, r=eddyb
Check privacy of trait items in all contexts

Fixes #28514

This is a sufficiently rare scenario and it's currently guarded by `private_in_public` lint, so it shouldn't be a [breaking-change] in practice.

bors added a commit that referenced this issue Apr 25, 2017

Auto merge of #41332 - petrochenkov:privti, r=eddyb
Check privacy of trait items in all contexts

Fixes #28514

This is a sufficiently rare scenario and it's currently guarded by `private_in_public` lint, so it shouldn't be a [breaking-change] in practice.

@bors bors closed this in #41332 Apr 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment