Dont check stability for items that are not pub to universe. #38689

pnkfelix · 2016-12-29T21:04:22Z

Dont check stability for items that are not pub to universe.

In other words, skip it for private and even pub(restricted) items, because stability checks are only relevant to things visible in other crates.

Fix #38412.

rust-highfive · 2016-12-29T21:04:27Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2016-12-29T21:06:17Z

Oops I left out a regression test.

(And it might be good to try to generalize the specific test to some other scenarios.... will do this in about two hours after i get through some meetings.)

pnkfelix · 2016-12-30T13:11:22Z

(weird, I guess I must have something subtly wrong with my check. Thank goodness for test suites.)

nikomatsakis · 2016-12-30T20:06:42Z

Marking as beta-accepted. Small patch, regression. cc @rust-lang/compiler

eddyb · 2016-12-30T20:37:12Z

src/librustc/middle/stability.rs

@@ -432,6 +432,18 @@ struct Checker<'a, 'tcx: 'a> {
 }

 impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
+ fn skip_stability_check_due_to_privacy(self, def_id: DefId) -> bool {


I was hoping this could be used to skip the "missing annotation" error in the original crate, within this same file, ensuring that the logic is self-consistent.

I'm not sure I follow.

The check you are referring to is fn check_missing_stability, right?

Is there an example you can show me of input code that is currently emitting a "missing annotation" error that you believe should be skipped via this check?

arielb1 · 2016-12-30T21:27:54Z

test?

Includes special case handling for trait methods. Fix rust-lang#38412.

pnkfelix · 2017-01-03T17:06:02Z

@arielb1 okay has good test now! :)

pnkfelix · 2017-01-03T17:08:37Z

Note that if there's a way to do this without special-casing trait methods (which is what the rustc_privacy::PrivacyVisitor seems to be doing) that would also be a small patch that could be readily back-ported, I'm all ears.

But I figured this was the simplest fix for the short term that we could backport. Separately we might consider revising what fn visibility returns for trait methods, which I think of as being implicitly pub.

petrochenkov · 2017-01-03T17:15:21Z

Strictly speaking the fix is incorrect because stability annotations are based on reachability and not local pub annotations. However, with this patch it's highly unlikely to hit the ICE accidentally - you need something from one of std crates + stability-checked during type checking + pub + unreachable.
So, r=me, especially for beta.

petrochenkov · 2017-01-03T18:09:13Z

Alternatively, the ICE can just be removed, since it's no longer an error to encounter unmarked API 😄
Originally it was useful to catch bugs in the stability system which was hastily written just before 1.0, now it's not so direly needed.

pnkfelix · 2017-01-03T19:14:18Z

(I lean slightly towards landing this over removing the ICE, in terms of what seems safest to push into the beta. But I don't have a real strong feeling about it. If other members of T-compiler think removing the ICE is the way to go, then we can try that.)

pnkfelix · 2017-01-03T19:17:07Z

@petrochenkov also, just to clarify: Your point about this fix being technically incorrect is because it is imposing too strong a constraint on the code, right?

I ask because I'm having a little trouble envisaging the scenario where something would have stability attributes and not be locally declared pub, i.e. the reasoning I gave in this PR's description. As @eddyb pointed out to me when I started working on this, even items exposed via pub use have to themselves be declared pub, right?

nikomatsakis · 2017-01-03T19:18:32Z

I'm fine with landing this PR for now, but I'm not really following closely.

petrochenkov · 2017-01-03T19:31:16Z

@pnkfelix

also, just to clarify: Your point about this fix being technically incorrect is because it is imposing too strong a constraint on the code, right?

On the contrary, the constraints are not strong enough.
Something like

fn main() {
    let a = std::sys::imp::process::process_common::StdioPipes { ..panic!() };
}

still should give and ICE with this patch because struct StdioPipes is pub, but unreachable and doesn't have a stability attribute.

arielb1 · 2017-01-03T23:43:23Z

@petrochenkov

Sure enough, ignoring the absence of stability markers, at least for items whose resolution was an error, looks like a good idea.

pnkfelix · 2017-01-04T16:39:51Z

@petrochenkov ah thank you for the example input.

nikomatsakis · 2017-01-04T20:19:53Z

@pnkfelix do you then plan to revise the PR?

alexcrichton · 2017-01-04T22:09:04Z

@bors: p=1

(for when this is approved, we'll want to backport)

pnkfelix · 2017-01-05T21:11:40Z

@bors r=petrochenkov

nikomatsakis · 2017-01-05T21:11:40Z

Discussed in @rust-lang/compiler meeting. Consensus was to land this PR for the beta, since it seems less risky, but follow-up with @petrochenkov's example afterwards (probably removing the ICE). @petrochenkov, maybe you can file an issue with your example?

@bors r+

bors · 2017-01-05T21:11:41Z

📌 Commit ab8e925 has been approved by petrochenkov

bors · 2017-01-05T21:11:41Z

📌 Commit ab8e925 has been approved by nikomatsakis

pnkfelix · 2017-01-05T21:12:44Z

(the answer to @nikomatsakis 's Q above is that I did and do not plan to revise this particular PR. However I will open an issue to make sure that we address the fact that there remains an ICE, just a much rarer one, even with this PR applied, as noted by @petrochenkov )

bors · 2017-01-05T21:58:30Z

⌛ Testing commit ab8e925 with merge 42bed72...

…, r=nikomatsakis Dont check stability for items that are not pub to universe. Dont check stability for items that are not pub to universe. In other words, skip it for private and even `pub(restricted)` items, because stability checks are only relevant to things visible in other crates. Fix #38412.

bors · 2017-01-06T00:05:22Z

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

alexcrichton · 2017-01-06T01:53:39Z

@pnkfelix this applies cleanly to beta but results in a build failure, could you take a look at backporting this to the beta branch?

rust-highfive assigned arielb1 Dec 29, 2016

pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 29, 2016

nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 30, 2016

eddyb reviewed Dec 30, 2016

View reviewed changes

pnkfelix added 2 commits January 3, 2017 09:57

Dont check stability for items that are not pub to universe.

ae13a72

Includes special case handling for trait methods. Fix rust-lang#38412.

Regression test and exploratory unit test.

ab8e925

pnkfelix force-pushed the dont-check-stability-on-private-items branch from 751cd72 to ab8e925 Compare January 3, 2017 14:58

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 3, 2017

pnkfelix mentioned this pull request Jan 5, 2017

Locally pub but inaccessible items w/o stability attributes can cause an ICE #38857

Closed

bors merged commit ab8e925 into rust-lang:master Jan 6, 2017

nikomatsakis mentioned this pull request Jan 6, 2017

Beta backports #38872

Merged

17 tasks

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont check stability for items that are not pub to universe. #38689

Dont check stability for items that are not pub to universe. #38689

pnkfelix commented Dec 29, 2016

rust-highfive commented Dec 29, 2016

pnkfelix commented Dec 29, 2016

pnkfelix commented Dec 30, 2016

nikomatsakis commented Dec 30, 2016

eddyb Dec 30, 2016

pnkfelix Jan 3, 2017

arielb1 commented Dec 30, 2016

pnkfelix commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

nikomatsakis commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

arielb1 commented Jan 3, 2017

pnkfelix commented Jan 4, 2017

nikomatsakis commented Jan 4, 2017

alexcrichton commented Jan 4, 2017

pnkfelix commented Jan 5, 2017

nikomatsakis commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

pnkfelix commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

Dont check stability for items that are not pub to universe. #38689

Dont check stability for items that are not pub to universe. #38689

Conversation

pnkfelix commented Dec 29, 2016

rust-highfive commented Dec 29, 2016

pnkfelix commented Dec 29, 2016

pnkfelix commented Dec 30, 2016

nikomatsakis commented Dec 30, 2016

eddyb Dec 30, 2016

Choose a reason for hiding this comment

pnkfelix Jan 3, 2017

Choose a reason for hiding this comment

arielb1 commented Dec 30, 2016

pnkfelix commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

pnkfelix commented Jan 3, 2017

nikomatsakis commented Jan 3, 2017

petrochenkov commented Jan 3, 2017

arielb1 commented Jan 3, 2017

pnkfelix commented Jan 4, 2017

nikomatsakis commented Jan 4, 2017

alexcrichton commented Jan 4, 2017

pnkfelix commented Jan 5, 2017

nikomatsakis commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

pnkfelix commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 6, 2017

alexcrichton commented Jan 6, 2017