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

Stabilize pub(restricted) #40556

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Conversation

cramertj
Copy link
Member

Fix #32409

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@retep998
Copy link
Member

Shouldn't we first just stabilize pub(restricted) so people have time to switch to it before we turn private in public into a hard error?

@cramertj
Copy link
Member Author

@retep998 That seems reasonable to me.

@cramertj
Copy link
Member Author

@retep998 I'll have a new commit up in a minute with that change.

@cramertj cramertj changed the title Stabilize pub(restricted) and convert private in public to a hard error Stabilize pub(restricted) Mar 15, 2017
format!("private type `{}` in public \
interface (error E0446)", ty));
}
let mut err = struct_span_err!(self.tcx.sess, self.span, E0446,
Copy link
Contributor

Choose a reason for hiding this comment

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

private_in_public can't be turned into an error in the near future unfortunately.
self.tcx.sess.features.borrow().pub_restricted can be well approximated with is_pub_restricted(vis) && is_pub_restricted(self.required_visibility) where is_pub_restricted = not pub && not private.
The idea is to always report an error and not warning when pub(something) is involved.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by not private? Visibility is either Public, Restricted(DefId), or Invisible (reserved for private external items).

Copy link
Contributor

@petrochenkov petrochenkov Mar 15, 2017

Choose a reason for hiding this comment

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

You can use hir::Visibility for this (as opposed to ty::Visibility).
For vis HIR visibility is immediately available (item.vis) , for self.required_visibility you can pass it from PrivateItemsInPublicInterfacesVisitor::visit_item to here somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even for hir::Visibility, I still only see Public, Crate, Restricted {...}, and Inherited, no Private. Am I missing something obvious?

Copy link
Contributor

Choose a reason for hiding this comment

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

Inherited is private (i.e. no any explicit pub)

@sfackler sfackler added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2017
@sfackler
Copy link
Member

Beta nominating since I think @aturon was interested in landing this in 1.17.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 16, 2017

I'm not inclined to backport a stabilization -- or at least not this one.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2017

📌 Commit 28626ca has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

Note: doc pr is here: rust-lang/reference#12

@petrochenkov
Copy link
Contributor

Right now this PR turns all private-in-public errors for pub(restricted) into warnings (since they are not in the "old set").
I'd prefer @cramertj to implement #40556 (comment) before merging.

@bors r-

@nikomatsakis
Copy link
Contributor

Hmm. I see. OK, that probably makes sense.

@strega-nil
Copy link
Contributor

strega-nil commented Mar 16, 2017

I disagree with this stabilization. This is a poor implementation of pub(restricted) which has too much power that isn't all that useful for real code, while also being ugly.

@nikomatsakis nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2017
@nikomatsakis
Copy link
Contributor

Removing beta-nominated. Seems like there is no need to backport a stabilization, no great urgency.

@strega-nil
Copy link
Contributor

Okay, I remove my disagreement, actually (sorry). I read more of the reasoning behind the need for in path, and I can agree that it is useful. (sorry sorry sorry)

@petrochenkov
Copy link
Contributor

@cramertj
A simpler and more precise solution - just walk all the crate's HIR and check if pub(restricted) is ever used.
This can be done in advance even if no errors happen, (almost) no-op HIR walk is super fast and won't affect performance (and this is all temporary anyway).

@cramertj
Copy link
Member Author

cramertj commented Mar 17, 2017

@petrochenkov I was just finishing up piping &'tcx hir::Visibility through the whole thing and shaking my head at how much churn wound up being necessary (also made me realize how much I'm dying for static rvalue promotion :) ). This sounds like a much better idea.

Just to make sure I understood you: the process is to add a new visitor which runs in librustc_privacy/lib.rs::check_crate and returns a bool of whether or not pub_restricted was used. If it was, all "private in public" warnings become errors?

@petrochenkov
Copy link
Contributor

Just to make sure I understood you: the process is to add a new visitor which runs in librustc_privacy/lib.rs::check_crate and returns a bool of whether or not pub_restricted was used. If it was, all "private in public" warnings become errors?

Exactly.
That would be equivalent to the previous behavior (modulo unused #![feature(pub_restricted)]s).

@nikomatsakis
Copy link
Contributor

r? @petrochenkov -- since you have something specific in mind =)

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2017

📌 Commit 60c1c96 has been approved by petrochenkov

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 21, 2017
bors added a commit that referenced this pull request Mar 21, 2017
Rollup of 10 pull requests

- Successful merges: #40229, #40312, #40332, #40502, #40556, #40576, #40667, #40671, #40681, #40685
- Failed merges:
@bors bors merged commit 60c1c96 into rust-lang:master Mar 21, 2017
@bors
Copy link
Contributor

bors commented Mar 21, 2017

⌛ Testing commit 60c1c96 with merge 58c701f...

@cramertj cramertj deleted the stabilize-pub-restricted branch March 22, 2017 06:49
bors added a commit that referenced this pull request Jul 15, 2017
support pub(restricted) in thread_local! (round 2)

Resurrected #40984 now that the issue blocking it was fixed. Original description:

`pub(restricted)` was stabilized in #40556 so let's go!

Here is a [playground](https://play.rust-lang.org/?gist=f55f32f164a6ed18c219fec8f8293b98&version=nightly&backtrace=1).

I changed the interface of `__thread_local_inner!`, which is supposedly unstable but this is not checked for macros (#34097 cc @petrochenkov @jseyfried), so this may be an issue.
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

Successfully merging this pull request may close these issues.

8 participants