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

librustc_*: Use pub(crate) instead of pub and remove dead code #43192

Closed
wants to merge 6 commits into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 12, 2017

This PR does the next things:

  • Replaces all pubs in librustc* crates (except for librustc itself) with pub(crate)s.
  • Reverts all replacements preventing x.py test from passing and RLS/Miri/Clippy from building.
  • Removes all dead code reported by the dead code lint.

The changes are 99% mechanical. I didn't judge and (semi-automatically) removed everything the compiler told me. Now this needs to be reviewed and some false positives need to be reverted (either for interface completeness or because they are needed by some third party tools).
EDIT: Changes mentioned in review comments are reverted.

cc @nikomatsakis and @michaelwoerister for rustc_data_structures
cc @nrc for librustc_driver APIs and tools
cc @rust-lang/compiler for everything else, if you see something that should be kept please leave a comment!

Next steps after reviewing:

  • Replace pub(crate) with nothing in crate roots.
  • Fix tidy and formatting.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

(pub(crate) $name:ident, $level:ident, $desc:expr) => (
pub(crate) static $name: &'static ::rustc::lint::Lint
= &lint_initializer!($name, $level, $desc);
);
Copy link
Member

Choose a reason for hiding this comment

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

Just use :vis to combine all 3 arms here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, it requires #![feature(macro_vis_matcher)] in all crates where the macro containing it is used, including tests.

@petrochenkov petrochenkov changed the title librustc_*: Use pub(crate) instead of pub and remove dead code [WIP] librustc_*: Use pub(crate) instead of pub and remove dead code Jul 12, 2017
@@ -43,24 +42,6 @@ impl DynamicLibrary {
}
}

/// Prepends a path to this process's search path for dynamic libraries
pub fn prepend_search_path(path: &Path) {
Copy link
Member

Choose a reason for hiding this comment

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

Any chance a bunch of these are used on only some host OSes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like no, but CI should catch if I missed anything.

@mcarton
Copy link
Member

mcarton commented Jul 12, 2017

We'd be lucky if something like that does not break Clippy. Cc @Manishearth, @llogiq, @oli-obk

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 13, 2017
@oli-obk
Copy link
Contributor

oli-obk commented Jul 13, 2017

This is great!

Would you be so kind and also test whether clippy and miri keep building and adjust your PR accordingly?

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

I did a pass over rustc_data_structures.

@@ -23,13 +23,6 @@ impl BitVector {
BitVector { data: vec![0; num_words] }
}

#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this one.

@@ -70,29 +70,29 @@ impl<A> Clone for ArrayVec<A>
}

impl<A: Array> ArrayVec<A> {
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to keep this type publicly available.

@@ -43,29 +43,25 @@ impl<A> Clone for AccumulateVec<A>
}

impl<A: Array> AccumulateVec<A> {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep these pub.

@@ -13,7 +13,7 @@

use std::str;

pub const MAX_BASE: u64 = 64;
pub(crate) const MAX_BASE: u64 = 64;
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this pub.

@@ -66,7 +60,7 @@ impl BitVector {
}

#[inline]
pub fn grow(&mut self, num_bits: usize) {
pub(crate) fn grow(&mut self, num_bits: usize) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep pub.

@@ -81,7 +81,7 @@ impl<A: Array> SmallVec<A> {
}
}

pub fn reserve(&mut self, n: usize) {
pub(crate) fn reserve(&mut self, n: usize) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep pub.

@@ -76,7 +76,7 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
}
}

pub fn len(&self) -> usize {
pub(crate) fn len(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep pub.

@@ -98,13 +98,13 @@ impl<D: SnapshotVecDelegate> SnapshotVec<D> {
/// Returns a mutable pointer into the vec; whatever changes you make here cannot be undone
/// automatically, so you should be sure call `record()` with some sort of suitable undo
/// action.
pub fn get_mut(&mut self, index: usize) -> &mut D::Value {
pub(crate) fn get_mut(&mut self, index: usize) -> &mut D::Value {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep pub.

&mut self.values[index]
}

/// Updates the element at the given index. The old value will saved (and perhaps restored) if
/// a snapshot is active.
pub fn set(&mut self, index: usize, new_elem: D::Value) {
pub(crate) fn set(&mut self, index: usize, new_elem: D::Value) {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep pub.

@@ -1,70 +0,0 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
Copy link
Member

Choose a reason for hiding this comment

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

This still seems like a useful type to have.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2017

Thanks!

@petrochenkov petrochenkov force-pushed the pubcrate branch 2 times, most recently from 265ae78 to 55694ca Compare July 14, 2017 20:21
@petrochenkov
Copy link
Contributor Author

Updated, comments addresses.

},
];

pub struct AllocatorMethod {
pub name: &'static str,
pub inputs: &'static [AllocatorTy],
pub output: AllocatorTy,
pub is_unsafe: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_unsafe being unused looks like a bug. It should be used in expansion, but it's ignored and all methods are expanded as unsafe.
@alexcrichton, is this intentional or just an omission?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes historically used but no longer used! Should be safe to remove.

@petrochenkov petrochenkov force-pushed the pubcrate branch 2 times, most recently from 9b1c051 to 9153114 Compare July 14, 2017 23:39
@nikomatsakis
Copy link
Contributor

I think these changes look good. I am sad to see some of the code go (e.g., useful algorithms on graphs etc), but I think that it's best to kill dead code. Those bits are still around in the git history if we ever want them. (In any case, I suspect we should start moving the librustc_data_structures code into individual crates; I have a local branch for example that removes the unification code in favor of https://crates.io/crates/ena.)

@alexcrichton alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 20, 2017
@alexcrichton
Copy link
Member

ping r? @eddyb

@@ -598,12 +598,3 @@ impl Level {
}
}
}

pub fn expect<T, M>(diag: &Handler, opt: Option<T>, msg: M) -> T
Copy link
Member

Choose a reason for hiding this comment

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

Good riddance! 🎉

@@ -19,7 +19,7 @@ use IntrinsicDef::Named;
// The default inlining settings trigger a pathological behaviour in
// LLVM, which causes makes compilation very slow. See #28273.
#[inline(never)]
pub fn find(name: &str) -> Option<Intrinsic> {
pub(crate) fn find(name: &str) -> Option<Intrinsic> {
Copy link
Member

Choose a reason for hiding this comment

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

Was the generator for these updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in the latest commit.

@@ -156,7 +156,7 @@ impl<'a, 'tcx> Visitor<'tcx> for EmbargoVisitor<'a, 'tcx> {
hir::ItemForeignMod(..) => {
self.prev_level
}
// Other `pub` items inherit levels from parents
// Other `pub(crate)` items inherit levels from parents
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct? Seems a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, I fixed several similar cases, but haven't seen this one.
I grepped a bit harder and found a few more, fixed in the last commit.

You can read more about trait objects in the [Trait Objects] section of the
Reference.
You can read more about trait objects in the Trait Object section of the
Reference:
Copy link
Member

Choose a reason for hiding this comment

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

Is this file some sort of rebase failure?

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Went through the entire diff and left some comments. Not that worried about deletions, because of git history, but some of the changes seemed unintentional.

@@ -78,15 +78,13 @@ pub struct SaveContext<'l, 'tcx: 'l> {
}

#[derive(Debug)]
pub enum Data {
Copy link
Member

Choose a reason for hiding this comment

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

These Data structures should all be pub still, they are expected to be used by clients

RefData(Ref),
DefData(Def),
RelationData(Relation),
}

pub trait Dump {
Copy link
Member

Choose a reason for hiding this comment

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

LIkewsie this trait and the functions below are API and should still be public

pub after_hir_lowering: PhaseController<'a>,
pub after_analysis: PhaseController<'a>,
pub after_llvm: PhaseController<'a>,
pub compilation_done: PhaseController<'a>,
pub(crate) compilation_done: PhaseController<'a>,
Copy link
Member

Choose a reason for hiding this comment

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

These fields should still be pub

@@ -273,7 +271,7 @@ pub fn anon_src() -> String {
"<anon>".to_string()
}

pub fn source_name(input: &Input) -> String {
pub(crate) fn source_name(input: &Input) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be pub still

@@ -329,7 +327,7 @@ pub struct PhaseController<'a> {
}

impl<'a> PhaseController<'a> {
pub fn basic() -> PhaseController<'a> {
pub(crate) fn basic() -> PhaseController<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

All these changes from here on down are bogus and should still be pub.

@@ -273,7 +273,7 @@ pub enum Compilation {
}

impl Compilation {
pub fn and_then<F: FnOnce() -> Compilation>(self, next: F) -> Compilation {
Copy link
Member

Choose a reason for hiding this comment

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

pub

@@ -617,7 +617,7 @@ fn save_analysis_format(sess: &Session) -> save::Format {
}

impl RustcDefaultCalls {
pub fn list_metadata(sess: &Session, matches: &getopts::Matches, input: &Input) -> Compilation {
Copy link
Member

Choose a reason for hiding this comment

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

pub

@@ -1016,7 +1016,7 @@ fn print_flag_list<T>(cmdline_opt: &str,
///
/// So with all that in mind, the comments below have some more detail about the
/// contortions done here to get things to work out correctly.
pub fn handle_options(args: &[String]) -> Option<getopts::Matches> {
fn handle_options(args: &[String]) -> Option<getopts::Matches> {
Copy link
Member

Choose a reason for hiding this comment

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

pub

@@ -1200,7 +1200,7 @@ fn exit_on_err() -> ! {
panic!();
}

pub fn diagnostics_registry() -> errors::registry::Registry {
fn diagnostics_registry() -> errors::registry::Registry {
Copy link
Member

Choose a reason for hiding this comment

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

pub

@@ -20,7 +20,7 @@ use snippet::Style;
#[derive(Clone, Debug, PartialEq, RustcEncodable, RustcDecodable)]
pub struct Diagnostic {
pub level: Level,
pub message: Vec<(String, Style)>,
pub(crate) message: Vec<(String, Style)>,
Copy link
Member

Choose a reason for hiding this comment

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

pretty much everything here should be pub

Copy link
Member

Choose a reason for hiding this comment

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

where 'here' = all of the _errors crate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made all the interface of Diagnostic public, but kept stuff in other modules containing implementation details crate-private.

@nrc
Copy link
Member

nrc commented Jul 20, 2017

I think that any change from pub -> pub(crate) in a non-public module is just noise (both in the git history and the source code). I think I'm generally a bit sad about how much noisier this is making the code, even if the tighter encapsulation and dead code removal have benefits.

@bors
Copy link
Contributor

bors commented Jul 21, 2017

☔ The latest upstream changes (presumably #43183) made this pull request unmergeable. Please resolve the merge conflicts.

Revert some `pub(crate)`s to `pub`s to make sure Miri and Clippy work
Revert some `pub(crate)`s to `pub`s to address Michael Woerister's comments
@petrochenkov
Copy link
Contributor Author

Rebased, comments addressed.

@petrochenkov
Copy link
Contributor Author

@nrc

I think that any change from pub -> pub(crate) in a non-public module is just noise

Interesting, this was kinda the point for me.
I don't immediately know if the module with pub entity private and even if it is private, the entity still can be reexported from it publicly. With pub(crate) the visibility scope becomes immediately evident.

@petrochenkov petrochenkov changed the title [WIP] librustc_*: Use pub(crate) instead of pub and remove dead code librustc_*: Use pub(crate) instead of pub and remove dead code Jul 23, 2017
@bors
Copy link
Contributor

bors commented Jul 23, 2017

☔ The latest upstream changes (presumably #43387) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member

nrc commented Jul 24, 2017

I don't immediately know if the module with pub entity private and even if it is private, the entity still can be reexported from it publicly. With pub(crate) the visibility scope becomes immediately evident.

This is pretty much a weakness in Rust's privacy system (and one the lang team has been debating recently). Does the privacy annotation on an item mean that item is 'at most' this public or 'at least' this public?

Using pub(crate) only works for doing what you want if you only care about privacy between crates. Many people care as much or more about privacy between modules and there using pub(crate) as an 'export limiter' is not helpful.

Given that this is only helpful for crate-level encapsulation and it is so noisey, I'd prefer not to do it.

@arielb1 arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jul 25, 2017

Given that this is only helpful for crate-level encapsulation and it is so noisey, I'd prefer not to do it.

At least with pub(crate) you only have a limited amount of code that can use the item. Privacy levels between private and pub(crate) are only interesting when you have a complex in-crate module hierarchy, and I'll expect that in these cases inter-crate privacy will be somewhat dynamic anyway.

@petrochenkov
Copy link
Contributor Author

this is only helpful for crate-level encapsulation

The primary benefit from pub(crate) compared to pub (in addition to encapsulation) is also lints catching various kinds of dead code - all of their logic is limited to a single crate and pub items are assumed to be used somewhere in other crates.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jul 25, 2017

This probably needs some collective decision from people working on these crates.
I see three primary alternatives:

  • The PR should be landed as is (after rebase)
  • Only the dead code removal part should be landed
  • The PR should be closed

@nrc
Copy link
Member

nrc commented Jul 26, 2017

I think the dead code removal is certainly worth landing. I would prefer not to land the rest, however, I don't work much on the crates which are mostly affected so if others want the changes, I won't object.

@petrochenkov
Copy link
Contributor Author

change from pub -> pub(crate) ... is just noise ... in the git history

This annoys me too, btw.
I can squash all the commits to minimize the effect.
Additionally the formatting commit (b9b1013) can be reverted if history is considered more important than indent of function arguments.

@aidanhs
Copy link
Member

aidanhs commented Aug 3, 2017

@petrochenkov looks like this needs a rebase!

This probably needs some collective decision from people working on these crates.

Who are you referring to here? i.e. is this with a rust team to come to a decision?

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2017
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Aug 3, 2017

@aidanhs
The compiler team most likely.
A good occasion to try rfcbot (will it listen to me?).
EDIT: It won't listen :( Could someone with rights do this instead?

@rfcbot fcp merge

Questions:

  • Should the dead code removal part of this PR be merged?
  • Should the "pub -> pub(crate)" part of this PR be merged?
  • Should the formatting commit be reverted to reduce the noise in git history?

@eddyb
Copy link
Member

eddyb commented Aug 3, 2017

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 3, 2017
@rfcbot
Copy link

rfcbot commented Aug 3, 2017

Team member @eddyb has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@petrochenkov
Copy link
Contributor Author

Too much bit rot and not enough interest, closing.

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 19, 2017
bors added a commit that referenced this pull request Aug 21, 2017
rustc: Remove some dead code

Extracted from #43192

r? @eddyb
@petrochenkov petrochenkov deleted the pubcrate branch June 5, 2019 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.