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

"error: private type in public interface" in private module #30905

Closed
SimonSapin opened this issue Jan 14, 2016 · 22 comments
Closed

"error: private type in public interface" in private module #30905

SimonSapin opened this issue Jan 14, 2016 · 22 comments

Comments

@SimonSapin
Copy link
Contributor

Reduced test case:

struct HostInternal;

mod parser {
    use super::HostInternal;
    pub fn parse() -> HostInternal {
        unimplemented!();
    }
}

Output with rustc 1.7.0-nightly (1447ce7 2016-01-13)

a.rs:5:23: 5:35 error: private type in public interface [E0446]
a.rs:5     pub fn parse() -> HostInternal {
                             ^~~~~~~~~~~~
a.rs:5:23: 5:35 help: run `rustc --explain E0446` to see a detailed explanation
error: aborting due to previous error

This is incorrect: the parse function is not visible anywhere HostInternal isn’t since the parser module is not public.

In my non-reduced crate I get a warning rather than an error but I haven’t managed to reproduce that in the reduced case. (Regardless, that warning is also incorrect.)

src/parser.rs:520:43: 520:55 warning: private type in public interface (error E0446), #[warn(private_in_public)] on by default
src/parser.rs:520                           -> ParseResult<(HostInternal, &'i str)> {
                                                            ^~~~~~~~~~~~
src/parser.rs:520:43: 520:55 note: this lint will become a HARD ERROR in a future release!
src/parser.rs:520                           -> ParseResult<(HostInternal, &'i str)> {
@oli-obk
Copy link
Contributor

oli-obk commented Jan 14, 2016

This is expected, because the outer module could re-export that function.

@SimonSapin
Copy link
Contributor Author

I couldn’t but it doesn’t. And the compiler should know that. It’s all in the same crate, a re-export can not be added without recompiling that crate.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 14, 2016

see #22261 for the discussion, the point is that the privacy checks don't need to reason about the entire crate, but just about the module

@SimonSapin
Copy link
Contributor Author

I understand that fixing this bug may require doing a more complex analysis than is currently done, but i maintain that it's a bug. #22261 (comment) says the same.

@SimonSapin
Copy link
Contributor Author

Conversely, this compile fine but should be an error:

pub fn parse() -> parser::HostInternal {
    unimplemented!();
}
mod parser {
    pub struct HostInternal;
}

@petrochenkov
Copy link
Contributor

@SimonSapin

fixing this bug may require doing a more complex analysis than is currently done

Conversely, this compile fine but should be an error

More complex analysis (reachability by reexports, "nameability") was done before and the example with HostInternal was an error (based on rust-lang/rfcs#136), but the rules were changed by the newer edition of RFC 136 to be local, module-level and based exclusively on pub annotations and not reachability (it wasn't thoroughly implemented though until recently).

Edit: some reasons are outlined by @nikomatsakis in #28450 (comment)

@SimonSapin
Copy link
Contributor Author

This sounds like a regression. Some git archaeology points to rust-lang/rfcs#200, I’ll read up on motivations.

@retep998
Copy link
Member

The new rules are not an improvement in any way except to make the code in rustc simpler. There's still a lot of false negatives, still a lot of false positives as well, and it will cause breaking changes to a huge amount of existing Rust code. If there's going to be breaking changes, the new system should at least have significant improvements to make the breakage worth it.

@petrochenkov
Copy link
Contributor

except to make the code in rustc simpler

Nah, the compiler still tracks reachability through reexports for other purposes.

@retep998
Copy link
Member

@petrochenkov So you're saying it isn't even an improvement at all and we're just breaking the world to satisfy some RFC that has been sitting around for over a year?

@petrochenkov
Copy link
Contributor

@retep998
The recent changes are an improvement because the implementation was a hardly predictable mix of the new rules (mostly), old rules (to less extent) and outright missing cases. Now it's at least consistent.

@nikomatsakis
Copy link
Contributor

@retep998 Let me just say that I'm sorry your code stopped compiling -- I know that's always a pain, whatever the cause. In this case, as @petrochenkov said, there really aren't any new rules: it's just that rustc was (very) buggy and failed to enforce the rules consistently. But I know that doesn't make it any less annoying when you have to update your code.

@retep998
Copy link
Member

The changes are still only warnings for me so far, although the warnings claim they will become errors, so thankfully it isn't the end of the world yet.

@nikomatsakis
Copy link
Contributor

@retep998 I'd actually be interested to see the specific cases that are failing for you. In particular, I'd like to know if they fall into the category of type aliases that appear in public interfaces, or something else?

@retep998
Copy link
Member

@nikomatsakis They're all basically a private struct, and then a public typedef to a raw pointer to it. The biggest example is https://github.com/retep998/winapi-rs/blob/bb182accc1cee10ea1a6e926ca51e8ddb63154c1/src/macros.rs#L4-L9 . It's easy enough for me to just make the internal opaque type public, I just don't like knowing that all existing versions of winapi will break in the future, forcing people to cargo update.

@nikomatsakis
Copy link
Contributor

@retep998 OK. That particular case would not be addressed by any of the mitigating measures we considered, I think. (It's sort of the motivating example that the rules were trying to prevent, from what I understand, in that one ought to be able to assume, because it is declared priv, that the private inner struct is not accessible from outside the module.)

@seanmonstar
Copy link
Contributor

This bit me as well. I defined an internal struct, and that sub modules would accept that struct as an argument to their methods.

The "solution" was to trick the privacy visitor, by putting the struct into its own submodule, make the struct pub there, and then import from the sub module.

playpen

mod foo {
    struct Buf {
        inner: Vec<u8>
    }

    mod bar {
        use super::Buf;

        pub struct Encoder;

        impl Encoder {
            pub fn encode(&mut self, buf: &mut Buf) {
                buf.inner.push(b'!');
            }
        }
    }
}

@SimonSapin
Copy link
Contributor Author

Oh, that’s a nice trick, thanks @seanmonstar

(Through it’s still unfortunate that we have to do this to work around the lint’s incorrect algorithm.)

@bluss
Copy link
Member

bluss commented Jan 21, 2016

Is that the right way to do it? It would be good to know, so that we don't use something that will be disallowed in the future.

@petrochenkov
Copy link
Contributor

@bluss
Yes, the current rules have the "universal workaround":

/*priv*/ item Item;

=>

/*priv*/ mod m {
    pub item Item;
}

It is even mentioned in the detailed error message.

@rphmeier
Copy link
Contributor

rphmeier commented Feb 8, 2016

I don't see how this can't be considered a bug:

mod bar {
    use super::Foo;

    pub fn foo_bar(_: Foo) {}
}

struct Foo;

It's trivial to see that this code doesn't actually export any private types. I strongly disagree with this being the intended behavior: it just seems wrong.

crumblingstatue added a commit to crumblingstatue/rgmk that referenced this issue Jul 6, 2016
crumblingstatue added a commit to crumblingstatue/rgmk that referenced this issue Jul 12, 2016
crumblingstatue added a commit to crumblingstatue/rgmk that referenced this issue Jul 12, 2016
@petrochenkov
Copy link
Contributor

Closing as working according to the rules.
Some discussion about possibly changing the rules happens in https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504/43

RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
Update E0446.md

The existing error documentation did not show how to use a child module's functions if the types used in those functions are private. These are some other places this problem has popped up that did not present a solution (these are from before the solution existed, 2016-2017. The solution was released in the Rust 2018 edition. However these were the places I was pointed to when I encountered the problem myself):
rust-lang#30905
https://stackoverflow.com/questions/39334430/how-to-reference-private-types-from-public-functions-in-private-modules/62374958#62374958
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
Update E0446.md

The existing error documentation did not show how to use a child module's functions if the types used in those functions are private. These are some other places this problem has popped up that did not present a solution (these are from before the solution existed, 2016-2017. The solution was released in the Rust 2018 edition. However these were the places I was pointed to when I encountered the problem myself):
rust-lang#30905
https://stackoverflow.com/questions/39334430/how-to-reference-private-types-from-public-functions-in-private-modules/62374958#62374958
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Feb 28, 2024
…1554)

Closes paritytech/polkadot-sdk-docs#55

- Changes 'current storage version' terminology to less ambiguous
'in-code storage version' (suggestion by @ggwpez)
- Adds a new example pallet `pallet-example-single-block-migrations`
- Adds a new reference doc to replace
https://docs.substrate.io/maintain/runtime-upgrades/ (temporarily living
in the pallet while we wait for developer hub PR to merge)
- Adds documentation for the `storage_alias` macro
- Improves `trait Hooks` docs 
- Improves `trait GetStorageVersion` docs
- Update the suggested patterns for using `VersionedMigration`, so that
version unchecked migrations are never exported
- Prevents accidental usage of version unchecked migrations in runtimes

paritytech/substrate#14421 (comment)
- Unversioned migration code is kept inside `mod version_unchecked`,
versioned code is kept in `pub mod versioned`
- It is necessary to use modules to limit visibility because the inner
migration must be `pub`. See
rust-lang/rust#30905 and

https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504/40
for more.

### todo

- [x] move to reference docs to proper place within sdk-docs (now that
#2102 is merged)
- [x] prdoc

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Juan <juangirini@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
…aritytech#1554)

Closes paritytech/polkadot-sdk-docs#55

- Changes 'current storage version' terminology to less ambiguous
'in-code storage version' (suggestion by @ggwpez)
- Adds a new example pallet `pallet-example-single-block-migrations`
- Adds a new reference doc to replace
https://docs.substrate.io/maintain/runtime-upgrades/ (temporarily living
in the pallet while we wait for developer hub PR to merge)
- Adds documentation for the `storage_alias` macro
- Improves `trait Hooks` docs 
- Improves `trait GetStorageVersion` docs
- Update the suggested patterns for using `VersionedMigration`, so that
version unchecked migrations are never exported
- Prevents accidental usage of version unchecked migrations in runtimes

paritytech/substrate#14421 (comment)
- Unversioned migration code is kept inside `mod version_unchecked`,
versioned code is kept in `pub mod versioned`
- It is necessary to use modules to limit visibility because the inner
migration must be `pub`. See
rust-lang/rust#30905 and

https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504/40
for more.

### todo

- [x] move to reference docs to proper place within sdk-docs (now that
paritytech#2102 is merged)
- [x] prdoc

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Juan <juangirini@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: gupnik <nikhilgupta.iitk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants