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

Tracking issue for `private_in_public` compatibility lint. #34537

Open
petrochenkov opened this Issue Jun 28, 2016 · 30 comments

Comments

Projects
None yet
10 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 28, 2016

What is this lint about

RFC 136 describes rules for using private types and traits in interfaces of public items. The main idea is that an entity restricted to some module cannot be used outside of that module. Private items are considered restricted to the module they are defined in, pub items are considered unrestricted and being accessible for all the universe of crates, RFC 1422 describes some more fine-grained restrictions. Note that the restrictions are determined entirely by item's declarations and don't require any reexport traversal and reachability analysis. "Used" means different things for different entities, for example private modules simply can't be named outside of their module, private types can't be named and additionally values of their type cannot be obtained outside of their module, etc. To enforce these rules more visible entities cannot contain less visible entities in their interfaces. Consider, for example, this function:

mod m {
    struct S;
    pub fn f() -> S { S } // ERROR
}

If this were allowed, then values of private type S could leave its module.

Despite the RFC being accepted these rules were not completely enforced until #29973 fixed most of the missing cases. Some more missing cases were covered later by PRs #32674 and #31362. For backward compatibility the new errors were reported as warnings (lints). Now it's time to retire these warnings and turn them into hard errors, which they are supposed to be.

This issue also tracks another very similar lint, pub_use_of_private_extern_crate, checking for a specific "private-in-public" situation.

How to fix this warning/error

If you really want to use some private unnameable type or trait in a public interface, for example to emulate sealed traits, then there's a universal workaround - make the item public, but put it into a private inner module.

mod m {
    mod detail {
        pub trait Float {}
        impl Float for f32 {}
        impl Float for f64 {}
    }
    pub fn multiply_by_2<T: detail::Float>(arg: T) { .... } // OK
}

The trait Float is public from the private-in-public checker's point of view, because it uses only local reasoning, however Float is unnameable from outside of m and effectively private, because it isn't actually reexported from m despite being potentially reexportable.
You'll also have to manually document what kind of mystery set of arguments your public function multiply_by_2 accepts, because unnameable traits are effectively private for rustdoc.

Current status

  • #29973 introduces the private_in_public lint as warn-by-default
  • #34206 makes the private_in_public lint deny-by-default
  • #36270 makes the private_in_public lint warn-by-default again due to relatively large amount of breakage
  • PR ? makes the private_in_public lint deny-by-default
  • PR ? makes the private_in_public lint a hard error
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jul 14, 2016

Making private_in_public an error also fixes #28514

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 1, 2016

Current status of crates from the regression report:

🌕 encoding - fixed, published
🌓 rotor - fixed, unpublished
🌑 ioctl - pr sent, not merged
🌕 genmesh - fixed, published
🌕 daggy - fixed, published
🌓 rust-locale - fixed, unpublished
🌓 iota-editor - fixed, unpublished
🌑 json_rpc - pr sent, not merged
🌚 couchdb - can't reproduce, other errors
🌕 libxdo - fixed, published
🌑 cuticula - pr sent, not merged
🌕 peano - fixed, published
🌓 plumbum - fixed, unpublished
🌕 postgis - fixed, published
🌓 probor - fixed, unpublished
🌕 endianrw - fixed, published
🌕 rodio - fixed, published
🌕 rose_tree - fixed, published
🌓 rustpather - fixed, unpublished
🌚 fbx_direct - can't reproduce, other errors
🌕 shuffled-iter - fixed, published
🌕 flexmesh - fixed, published
🌕 skeletal_animation - fixed, published
🌓 stdx - fixed, unpublished
🌓 tick - fixed, unpublished
🌕 glitter - fixed, published

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 7, 2016

@petrochenkov @nikomatsakis Any chance of flipping the switch to hard error this year?
I'm asking because I'm having to do some node ID gymnastics to support the old error/new lint system.
OTOH, I should do a crater run, since I'm moving it to semantic types, so I can try just not checking bounds.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 8, 2016

@eddyb

Any chance of flipping the switch to hard error this year?

Not without implementing rust-lang/rfcs#1671 (or rather https://internals.rust-lang.org/t/rfc-reduce-false-positives-for-private-type-in-public-interface/3678) first, I suppose.
After that the lint can hopefully be turned to an error, or at least the old checker can be dropped.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 9, 2016

Gah, I've been meaning to start a thread to discuss this topic in more depth! I had completely forgotten.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 9, 2016

@nikomatsakis You remembered just in time! 😆
I've just opened #37676 and will do a crater run on it and on a version that errors and ignores bounds.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 10, 2016

Crater report shows 32 root regressions (out of which log-panics and sdl2_gfx are false positives).
@petrochenkov Even if it's ignoring bounds, the fallout looks pretty bad. If you have time, could you look over the ones which didn't show up on previous runs? When the error is in a dependency, that's usually due to outdated dependencies + --cap-lints allow, but there could be new cases I triggered.

Some of them are old versions of xml-rs (0.1.* and 0.2.*) and nix (0.4.*) - couldn't their authors release new patch versions pub-ifying all relevant types? cc @netvl @carllerche

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 10, 2016

I'll look at the fallout today or tomorrow.
The problem is that people sometimes use allow(private_in_public) deliberately and the previous run was done after making private_in_public deny-by-default, so it didn't caught those cases.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 10, 2016

😞 Would it then make sense to use forbid as a default, at least for crater runs?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 10, 2016

Using forbid for crater runs looks like a good idea.

I've looked through about half of regressions, all looks legitimate and many are indeed due to nix and xml-rs.
As a side note, error spans are pretty bad - they point to a whole item, it's not clear what exactly is private, especially if it's on some other line.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 10, 2016

There may be another solution to all this private-in-public problem.

Implement #30476 and make reporting of this kind of privacy errors late, as opposed to early/preventive as it is today. I expect less fallout from this, because things like this

mod m {
    struct S;
    mod n {
        pub fn f() -> super::S { super::S }
    }

    fn g() {
        les s = n::f();
    }
}

won't report errors.

All private-in-public errors in rustc_privacy (but not in resolve!) could be lowered to lints then, because early reporting is still useful for catching silly human errors.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 10, 2016

Yeah, that's a bit of a pickle. Not sure what the optimal solution is there, nothing obvious works.
One magical way would be to associate "constructors" to each syntactical type node, a type parametrized by the syntactical sub-nodes, so that leaves can be distinguished from types coming "from elsewhere".

But that doesn't scale to resolutions of associated types through where clauses, which go through the trait machinery, and at that point you'd have to negate all the benefits of uniform semantical types to get accurate location information for everything.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 10, 2016

Oh, #30476 looks interesting. We can do it uniformly in inference writeback, is intra-function enough?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 10, 2016

We can do it uniformly in inference writeback, is intra-function enough?

A privacy pass simply needs to go through all HIR nodes and check their types* (somehow avoiding duplicated errors, but that's details).
Some nodes are intra-function, some are in item interfaces. Not sure what are consequences exactly, but looks like it can't always be done through function contexts.

* At least adjusted types, not sure about non-adjusted.

bors added a commit that referenced this issue Jun 1, 2017

Auto merge of #42136 - petrochenkov:oldhard, r=nikomatsakis
Turn sufficiently old compatibility lints into hard errors

It's been almost 7 months since #36894 was merged, so it's time to take the next step.

[breaking-change], needs crater run.

PRs/issues submitted to affected crates:
gnzlbg/ctest#17
Sean1708/rusty-cheddar#55
m-r-r/helianto#3
azdle/virgil#1
rust-locale/rust-locale#24
mneumann/acyclic-network-rs#1
reem/rust-typemap#38

cc https://internals.rust-lang.org/t/moving-forward-on-forward-compatibility-lints/4204
cc #34537 #36887
Closes #36886
Closes #36888
Closes #36890
Closes #36891
Closes #36892
r? @nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 1, 2017

bors added a commit that referenced this issue Jun 5, 2017

Auto merge of #42125 - petrochenkov:privty, r=<try>
Check types for privacy

This PR implements late post factum checking of type privacy, as opposed to early preventive "private-in-public" checking.
This will allow to turn private-in-public checks into a lint and make them more heuristic-based, and more aligned with what people may expect (e.g. reachability-based behavior).

Types are privacy-checked if they are written explicitly, and also if they are inferred as expression or pattern types.
This PR checks "semantic" types and does it unhygienically, this significantly restricts what macros 2.0 (as implemented in #40847) can do (sorry @jseyfried) - they still can use private *names*, but can't use private *types*.
This is the most conservative solution, but hopefully it's temporary and can be relaxed in the future, probably using macro contexts of expression/pattern spans.

Traits are also checked in preparation for [trait aliases](#41517), which will be able to leak private traits, and macros 2.0 which will be able to leak pretty much anything.

This is a [breaking-change], but the code that is not contrived and can be broken by this patch should be guarded by `private_in_public` lint. [Previous crater run](#34537 (comment)) discovered a few abandoned crates that weren't updated since `private_in_public` has been introduced in 2015.

cc #34537 https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504
Fixes #30476
Fixes #33479

cc @nikomatsakis
r? @eddyb

bors added a commit that referenced this issue Jun 19, 2017

Auto merge of #42125 - petrochenkov:privty, r=alexcrichton
Check types for privacy

This PR implements late post factum checking of type privacy, as opposed to early preventive "private-in-public" checking.
This will allow to turn private-in-public checks into a lint and make them more heuristic-based, and more aligned with what people may expect (e.g. reachability-based behavior).

Types are privacy-checked if they are written explicitly, and also if they are inferred as expression or pattern types.
This PR checks "semantic" types and does it unhygienically, this significantly restricts what macros 2.0 (as implemented in #40847) can do (sorry @jseyfried) - they still can use private *names*, but can't use private *types*.
This is the most conservative solution, but hopefully it's temporary and can be relaxed in the future, probably using macro contexts of expression/pattern spans.

Traits are also checked in preparation for [trait aliases](#41517), which will be able to leak private traits, and macros 2.0 which will be able to leak pretty much anything.

This is a [breaking-change], but the code that is not contrived and can be broken by this patch should be guarded by `private_in_public` lint. [Previous crater run](#34537 (comment)) discovered a few abandoned crates that weren't updated since `private_in_public` has been introduced in 2015.

cc #34537 https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504
Fixes #30476
Fixes #33479

cc @nikomatsakis
r? @eddyb

bors added a commit that referenced this issue Jul 7, 2017

Auto merge of #42125 - petrochenkov:privty, r=nikomatsakis
Check types for privacy

This PR implements late post factum checking of type privacy, as opposed to early preventive "private-in-public" checking.
This will allow to turn private-in-public checks into a lint and make them more heuristic-based, and more aligned with what people may expect (e.g. reachability-based behavior).

Types are privacy-checked if they are written explicitly, and also if they are inferred as expression or pattern types.
This PR checks "semantic" types and does it unhygienically, this significantly restricts what macros 2.0 (as implemented in #40847) can do (sorry @jseyfried) - they still can use private *names*, but can't use private *types*.
This is the most conservative solution, but hopefully it's temporary and can be relaxed in the future, probably using macro contexts of expression/pattern spans.

Traits are also checked in preparation for [trait aliases](#41517), which will be able to leak private traits, and macros 2.0 which will be able to leak pretty much anything.

This is a [breaking-change], but the code that is not contrived and can be broken by this patch should be guarded by `private_in_public` lint. [Previous crater run](#34537 (comment)) discovered a few abandoned crates that weren't updated since `private_in_public` has been introduced in 2015.

cc #34537 https://internals.rust-lang.org/t/lang-team-minutes-private-in-public-rules/4504
Fixes #30476
Fixes #33479

cc @nikomatsakis
r? @eddyb
@vberger

This comment has been minimized.

Copy link
Contributor

vberger commented Sep 6, 2017

I don't know how much this is expected/already known, but the lint triggers in cases like this one:

pub mod foo {
    struct Private;
    
    mod internal {
        pub type Bar = Vec<super::Private>;
    }
}

with

warning: private type `foo::Private` in public interface (error E0446)
 --> src/lib.rs:7:9
  |
7 |         pub type Bar = Vec<super::Private>;
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(private_in_public)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #34537 <https://github.com/rust-lang/rust/issues/34537>

Though here, relative to module foo, Private and internal::Bar are both private. As such, I find this surprising that the lints triggers on this.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Sep 6, 2017

@vberger
This is expected behavior at the moment (Bar is pub, Private is pub(in foo), pub > pub(in foo) => private-in-public error), but it will change soon and this specific case won't be reported.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Nov 11, 2017

How the hell is pub_use_of_private_extern_crate a "private-in-public" situation? How does one reexport a crate now? Why are we deprecating that without an epoch? That's a breaking change that will break existing code.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Nov 11, 2017

I am aware one can now use pub extern crate, but that can mean having to bump the minimum Rust version in these crates for absolutely no real reason.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Nov 11, 2017

@nox

How the hell is pub_use_of_private_extern_crate a "private-in-public" situation?

Private items cannot be publicly reexported, that's a general rule that wasn't properly enforced for crate items in early versions of rustc. The rule is pretty fundamental for the import resolution algorithm.

How does one reexport a crate now?

Using pub extern crate.

Why are we deprecating that without an epoch? That's a breaking change that will break existing code.

Plenty of bugfixes breaking small numbers of crates were gradually landed in the previous years without epochs, using deprecation periods. The official policy is https://github.com/rust-lang/rfcs/blob/master/text/1122-language-semver.md

Regarding pub_use_of_private_extern_crate specificlly, in June crater run found 7 crates on crates.io exploiting this bug and I sent PRs with fixes to all of them. So the ecosystem in general is not affected.
If it affects something not on crates.io, e.g. Servo, there's still plenty of time to fix it, pub_use_of_private_extern_crate is still a lint and won't break dependencies of affected crates, I didn't plan making it a hard error until the next June or so, it could be delayed even further given sufficient demand.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 11, 2017

Not all Rust code in the world is in crates.io, and we don’t know how representative crates.io is. Even with several months of warning, making breaking changes that are not soundness fixes without an epoch discredits our stability promise. This one feels particularly frivolous.

@roblabla

This comment has been minimized.

Copy link
Contributor

roblabla commented Jun 29, 2018

The lint is currently warning on the following code : https://play.rust-lang.org/?gist=398cc0302ea20d2781d62db6deb53d97&version=nightly&mode=debug

Is this a bug ? If not, is the reasoning explained anywhere ? I feel like implementing a public trait on private traits shouldn't cause the warning ?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jun 29, 2018

@roblabla
This is expected behavior in the currently implemented rules, because the rules are pretty dumb - "something private anywhere in the interface -> report an error/warning".

In revised (but not yet implemented) rules this is a lint that's probably going to be allow-by-default, because this is still kind of a documentation issue (how do you explain to the library users what trait Public is implemented for?) that people may want to catch.

teddywing added a commit to teddywing/dome-key-map that referenced this issue Nov 22, 2018

parser: Make `KeyboardKey` private
Get rid of these warnings:

    warning: private type `parser::Character` in public interface (error E0446)
      --> src/parser.rs:72:15
       |
    72 |     Character(Character),
       |               ^^^^^^^^^
       |
       = note: #[warn(private_in_public)] on by default
       = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
       = note: for more information, see issue #34537 <rust-lang/rust#34537>

    warning: private type `parser::KeyCode` in public interface (error E0446)
      --> src/parser.rs:73:13
       |
    73 |     KeyCode(KeyCode),
       |             ^^^^^^^
       |
       = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
       = note: for more information, see issue #34537 <rust-lang/rust#34537>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment