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

Check types for privacy #42125

Merged
merged 3 commits into from
Jul 7, 2017
Merged

Check types for privacy #42125

merged 3 commits into from
Jul 7, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented May 20, 2017

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, 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 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

None
}
ty::TyAnon(..) => {
// FIXME: How to visit `impl Trait` type?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.
IIRC, my attempts to visit it with TypeVisitor either infinitely recursed or ICEd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't impl Trait be able to "export" private types? cc @rust-lang/lang

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in anonymized form, but I'm talking about preventing use of

impl PrivateTrait
     ^^^^^^^^^^^^

and

impl Trait<PrivateType>
           ^^^^^^^^^^^

here, not use of private underlying types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. So using API that involves private types? The private types would show up elsewhere if they were involved. Like, using any method on Trait would end up with that part in its Substs.

return true;
} else if let ty::TyFnDef(..) = ty.sty {
// Inherent static methods don't have self type in substs,
// we have to check it additionally.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason Self for static inherent methods is not listed in substs.
IIRC, it's not the first time I see it requiring special treatment for this reason.

Copy link
Member

@eddyb eddyb May 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not because Self is a mere alias in them. The substitutions for impls (inherent or trait impls, but you never get the latter from type-checking, only e.g. trans or MIR inlining can see them) are literally for the <...> in impl<...>.

However, it's (AFAIK) impossible to access an inherent method without one of:

  • method call, which means there is an expression containing the Self type
  • T::method which is sugar for <T>::method, and the semantic version of T can be read (see rustdoc and rustc_save_analysis)

// ext::Z; // ERROR type `ext::Z` is private
// ERROR type `fn(u8) -> ext::Z {ext::Z::{{constructor}}}` is private
// ext::Z1; // ERROR type `fn(u8) -> ext::Z1 {ext::Z1::{{constructor}}}` is private
// ext::Z1::k; // ERROR type `fn() {ext::Z1::k}` is private
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these cases cannot be tested until macros 2.0 are merged.

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 20, 2017
@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 24, 2017

@petrochenkov I'm very excited to see this PR! I have a few questions. I think there are some key questions that we were wrestling with:

  • Is it legal to define an associated type in a "public impl" that references a private type?
    • Update: In this PR, no. Maybe later.
  • Is it legal to make a pub type Foo = PrivateType? (Presumably it would be illegal to reference it.)
    • Update: In this PR, no. Maybe later.
  • Is it legal to make a pub use self::foo::PrivateThing? (Presumably it would be illegal to reference it.)
    • Update: No. No changes planned.
  • Are these the right questions? Other corner cases or interesting cases I've forgotten about?
    • Update: None suggested.

I'm sure I'll know the answers once I skim the code, but I thought it'd be helpful to try and summarize these key points so that people don't have to do so.

@nikomatsakis
Copy link
Contributor

Also, I think we should FCP this before we land it. I don't anticipate a lot of back-and-forth but this is basically an insta-stable language change, so I think it makes sense.

@petrochenkov
Copy link
Contributor Author

@nikomatsakis

Is it legal to define an associated type in a "public impl" that references a private type?
Is it legal to make a pub type Foo = PrivateType? (Presumably it would be illegal to reference it.)

I didn't relax the private-in-public checker yet, so these two are still errors. I'm going to send a separate PR with private-in-public changes later. That PR will certainly require an FCP, this one - I'm not so sure.

My preliminary plan is that pub type Foo = PrivateType will be legal, prohibiting it achieves nothing in presence of macros 2.0 (pub macro Foo { PrivateType } does the same thing).
pub associated_type Foo = PrivateType is a thorny case, discussed at length in the internals thread. I think I'll follow the decision from that discussion and keep it an error. It's viable because this check is based on semantic types and traits and not affected by macros. But this is a restriction that can certainly be relaxed in the future if all types are normalized before checking for privacy.

Is it legal to make a pub use self::foo::PrivateThing? (Presumably it would be illegal to reference it.)

This is a separate case unrelated to this PR and its successors, and it will continue being an error.
With current resolution model this is basically an "unused import"/"use importing nothing" error in disguise.
pub([visibility]) [pattern] filters names matching [pattern] using [visibility] and imports what remains. This works for everything - globs, names matching in several namespaces and single names as well. pub use self::foo::PrivateThing just filters away all matching names and imports nothing, then it reports an error instead of being useless.

@brson
Copy link
Contributor

brson commented May 24, 2017

@brson
Copy link
Contributor

brson commented May 25, 2017

I've started the crate builds for crater.

@brson
Copy link
Contributor

brson commented May 25, 2017

@nikomatsakis
Copy link
Contributor

Inline (not yet analyzed)

Coverage

  • 9476 crates tested: 7024 working / 2169 broken / 61 regressed / 27 fixed / 195 unknown.

Regressions

  • There are 37 root regressions
  • There are 61 regressions

Root regressions, sorted by rank:

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 26, 2017

Analysis:
🌳 rmp-serde-0.13.1 - type inference is too smart (#33479)
🍂 coyoneda-0.5.2 - actually morphism-0.4.0, type inference is too smart (#33479)
🌳 mysql-11.1.0 - type inference is too smart (similar to #33479, but with a trait impl)
🍂 locale-0.2.1 - unfixed private_in_public in the same crate, fixed on master but unpublished
🍂 couchdb-0.5.1 - unfixed private_in_public in the same crate, fixed on master but unpublished
🍂 mpd-0.0.11 - unfixed private_in_public in the same crate, fixed on master but unpublished
🍂 rgmk-0.2.0 - unfixed private_in_public in the same crate, fixed on master but unpublished
🍂 rules-0.0.2 - unfixed private_in_public in the same crate, not fixed on master
🍂 rustpather-0.1.0 - unfixed private_in_public in the same crate, the repository doesn't exist anymore
🍂 swindon-0.4.0 - allow(private_in_public) in the same crate, not fixed on master
🌳 slog-async-2.0.1 - allow(private_in_public) in the same project (but other crate, slog)
🍂 slog-perf-0.1.0 - allow(private_in_public) in the same project (but other crate, slog)
🍂 reql-0.0.8 - allow(private_in_public) in another project (slog)
🌳 secret-service-0.3.1 - actually rust-gmp-0.2.10 (old version), unfixed private_in_public in the same crate
🍂 carboxyl_time-0.0.2 - actually carboxyl-0.1.2 (old version), unfixed private_in_public in the same crate

Crates that have other crates dependent on them are marked with 🌳.

I think it'll be okay to send PRs with fixes and/or version bumps, and then land this without additional warnings.

@nikomatsakis
Copy link
Contributor

unfixed private_in_public in the same crate, fixed on master but unpublished

what does "unfixed" mean? oh, unfixed in that crate, you mean? (i.e., not on rustc master)

@petrochenkov
Copy link
Contributor Author

@nikomatsakis

unfixed in that crate, you mean?

Yes

@petrochenkov
Copy link
Contributor Author

PRs/issues are submitted to all affected crates.

@arielb1
Copy link
Contributor

arielb1 commented May 27, 2017

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.

It's hard to offer a strong boundary using private types in macros anyway, because of inference leakages. Unreachable-but-public types can be used if you want to hide a type name.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2017
Cleanup checking of inherent static methods and fix checking of inherent associated constants
Add more tests
Fix regressions after rebase
@petrochenkov
Copy link
Contributor Author

Rebased.
@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 6, 2017

📌 Commit a27f8cf has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 7, 2017

⌛ Testing commit a27f8cf with merge b80b659...

bors added a commit that referenced this pull request Jul 7, 2017
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
Copy link
Contributor

bors commented Jul 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b80b659 to master...

@alexcrichton
Copy link
Member

@rust-lang/compiler just FYI but there's 5 open regressions against beta for this PR, all linked above this comment.

@petrochenkov petrochenkov deleted the privty branch August 26, 2017 00:16
bors added a commit that referenced this pull request Sep 23, 2017
Record semantic types for all syntactic types in bodies

... and use recorded types in type privacy checking (types are recorded after inference, so there are no `_`s left).
Also use `hir_ty_to_ty` for types in signatures in type privacy checking.

This could also be potentially useful for save-analysis and diagnostics.

Fixes #42125 (comment)
r? @eddyb
bors added a commit that referenced this pull request Dec 21, 2017
Type privacy polishing

Various preparations before implementing rust-lang/rfcs#2145 containing final minor breaking changes (mostly for unstable code or code using `allow(private_in_public)`).
(Continuation of #42125, #44633 and #41332.)

It would be good to run crater on this.
r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.