Skip to content
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

privacy exposure bug when struct has field(s) *and* an impl #10545

Closed
pnkfelix opened this issue Nov 18, 2013 · 3 comments · Fixed by #11019
Closed

privacy exposure bug when struct has field(s) *and* an impl #10545

pnkfelix opened this issue Nov 18, 2013 · 3 comments · Fixed by #11019
Labels
A-visibility Area: Visibility / privacy

Comments

@pnkfelix
Copy link
Member

This was fun one for isolating the parameters where it does and does not occur.

The code (a.rs):

mod a {
    #[cfg(hide_payload)]      struct S;
    #[cfg(not(hide_payload))] struct S { p: () }

    #[cfg(not(hide_impl))]    impl S { }
}

struct Expose;
impl Expose {
    #[cfg(not(hide_payload))] pub fn expose(&self) -> a::S { a::S { p: () } }
    #[cfg(hide_payload)]      pub fn expose(&self) -> a::S { a::S }
}

fn main() {
    let _s = Expose.expose();
}

So, S is not pub in a, so it should not be possible to access it from main.

But, out of the box, the above compiles and runs:

% rustc --version
/Users/fklock/opt/rust-dbg/bin/rustc 0.9-pre (4dded43 2013-11-17 23:41:25 -0800)
host: x86_64-apple-darwin
% rustc a.rs
%

If you remove either the payload or the impl, the privacy checker will flag an error:

% rustc --cfg hide_payload a.rs
a.rs:11:61: 11:65 error: struct `S` is inaccessible
a.rs:11     #[cfg(hide_payload)]      pub fn expose(&self) -> a::S { a::S }
                                                                     ^~~~
error: aborting due to previous error
task 'rustc' failed at 'explicit failure', /Users/fklock/Dev/Mozilla/rust.git/src/libsyntax/diagnostic.rs:101
task '<main>' failed at 'explicit failure', /Users/fklock/Dev/Mozilla/rust.git/src/librustc/lib.rs:396
% rustc --cfg hide_impl a.rs
a.rs:10:54: 10:58 error: type `S` is private
a.rs:10     #[cfg(not(hide_payload))] pub fn expose(&self) -> a::S { a::S { p: () } }
                                                              ^~~~
error: aborting due to previous error
task 'rustc' failed at 'explicit failure', /Users/fklock/Dev/Mozilla/rust.git/src/libsyntax/diagnostic.rs:101
task '<main>' failed at 'explicit failure', /Users/fklock/Dev/Mozilla/rust.git/src/librustc/lib.rs:396
@pnkfelix
Copy link
Member Author

(The fact that the error messages are different is a little bit of a red flag to me.)

@alexcrichton
Copy link
Member

Agreed, something funky is going on here.

@alexcrichton
Copy link
Member

As mentioned in #10548, I believe that this is all a similar bug as this program:

mod a {
    struct S;
    impl S { }
}

fn foo(_: a::S) {
}

fn main() {}

This should not compile, but it does.

bors added a commit that referenced this issue Dec 18, 2013
This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes #10545
niftynif added a commit to niftynif/rust that referenced this issue Dec 26, 2013
Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.
niftynif added a commit to niftynif/rust that referenced this issue Dec 30, 2013
Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.
niftynif added a commit to niftynif/rust that referenced this issue Dec 30, 2013
Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.
niftynif added a commit to niftynif/rust that referenced this issue Jan 4, 2014
Wrote some tests for the compare/clone/to_str methods.

Took out doc comments around tests.

Don't allow impls to force public types

This code in resolve accidentally forced all types with an impl to become
public. This fixes it by default inheriting the privacy of what was previously
there and then becoming `true` if nothing else exits.

Closes rust-lang#10545

Committing to show work in progress.  Everything is still messy,
but I want to take this opportunity to get feedback from relevant
parties if I can.

Recommitting after a bad commit that unnecessarily changed files.

Added more robust compare and equals for Branch and Leaf, as well
as some style changes.

Cleanup: removing features that aren't needed, and removing lines that
were eventually commented out anyway.

Cleaning up whitespace errors.

Undoing accidental deletion of file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-visibility Area: Visibility / privacy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants