Auto merge of #59335 - Aaron1011:fix/extern-priv-final, r=Aaron1011

Properly parse '--extern-private' with name and path

It turns out that #57586 didn't properly parse `--extern-private name=path`.

This PR properly implements the `--extern-private` option. I've added a new `extern-private` option to `compiletest`, which causes an `--extern-private` option to be passed to the compiler with the proper path.

Part of #44663

Author: bors

src/librustc/middle/cstore.rs

@@ -199,6 +199,7 @@ pub trait CrateStore {
 
     // "queries" used in resolve that aren't tracked for incremental compilation
     fn crate_name_untracked(&self, cnum: CrateNum) -> Symbol;
+    fn crate_is_private_dep_untracked(&self, cnum: CrateNum) -> bool;
     fn crate_disambiguator_untracked(&self, cnum: CrateNum) -> CrateDisambiguator;
     fn crate_hash_untracked(&self, cnum: CrateNum) -> Svh;
     fn extern_mod_stmt_cnum_untracked(&self, emod_id: ast::NodeId) -> Option<CrateNum>;

src/librustc/session/config.rs

@@ -283,22 +283,29 @@ impl OutputTypes {
 // DO NOT switch BTreeMap or BTreeSet out for an unsorted container type! That
 // would break dependency tracking for command-line arguments.
 #[derive(Clone, Hash)]
-pub struct Externs(BTreeMap<String, BTreeSet<Option<String>>>);
+pub struct Externs(BTreeMap<String, ExternEntry>);
+
+#[derive(Clone, Hash, Eq, PartialEq, Ord, PartialOrd, Debug, Default)]
+pub struct ExternEntry {
+    pub locations: BTreeSet<Option<String>>,
+    pub is_private_dep: bool
+}
 
 impl Externs {
-    pub fn new(data: BTreeMap<String, BTreeSet<Option<String>>>) -> Externs {
+    pub fn new(data: BTreeMap<String, ExternEntry>) -> Externs {
         Externs(data)
     }
 
-    pub fn get(&self, key: &str) -> Option<&BTreeSet<Option<String>>> {
+    pub fn get(&self, key: &str) -> Option<&ExternEntry> {
         self.0.get(key)
     }
 
-    pub fn iter<'a>(&'a self) -> BTreeMapIter<'a, String, BTreeSet<Option<String>>> {
+    pub fn iter<'a>(&'a self) -> BTreeMapIter<'a, String, ExternEntry> {
         self.0.iter()
     }
 }
 
+
 macro_rules! hash_option {
     ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, [UNTRACKED]) => ({});
     ($opt_name:ident, $opt_expr:expr, $sub_hashes:expr, [TRACKED]) => ({
@@ -427,10 +434,6 @@ top_level_options!(
         remap_path_prefix: Vec<(PathBuf, PathBuf)> [UNTRACKED],
 
         edition: Edition [TRACKED],
-
-        // The list of crates to consider private when
-        // checking leaked private dependency types in public interfaces
-        extern_private: Vec<String> [TRACKED],
     }
 );
 
@@ -633,7 +636,6 @@ impl Default for Options {
             cli_forced_thinlto_off: false,
             remap_path_prefix: Vec::new(),
             edition: DEFAULT_EDITION,
-            extern_private: Vec::new()
         }
     }
 }
@@ -2315,10 +2317,14 @@ pub fn build_session_options_and_crate_config(
         )
     }
 
-    let extern_private = matches.opt_strs("extern-private");
+    // We start out with a Vec<(Option<String>, bool)>>,
+    // and later convert it into a BTreeSet<(Option<String>, bool)>
+    // This allows to modify entries in-place to set their correct
+    // 'public' value
+    let mut externs: BTreeMap<String, ExternEntry> = BTreeMap::new();
+    for (arg, private) in matches.opt_strs("extern").into_iter().map(|v| (v, false))
+        .chain(matches.opt_strs("extern-private").into_iter().map(|v| (v, true))) {
 
-    let mut externs: BTreeMap<_, BTreeSet<_>> = BTreeMap::new();
-    for arg in matches.opt_strs("extern").into_iter().chain(matches.opt_strs("extern-private")) {
         let mut parts = arg.splitn(2, '=');
         let name = parts.next().unwrap_or_else(||
             early_error(error_format, "--extern value must not be empty"));
@@ -2331,10 +2337,17 @@ pub fn build_session_options_and_crate_config(
             );
         };
 
-        externs
+        let entry = externs
             .entry(name.to_owned())
-            .or_default()
-            .insert(location);
+            .or_default();
+
+
+        entry.locations.insert(location.clone());
+
+        // Crates start out being not private,
+        // and go to being private if we see an '--extern-private'
+        // flag
+        entry.is_private_dep |= private;
     }
 
     let crate_name = matches.opt_str("crate-name");
@@ -2386,7 +2399,6 @@ pub fn build_session_options_and_crate_config(
             cli_forced_thinlto_off: disable_thinlto,
             remap_path_prefix,
             edition,
-            extern_private
         },
         cfg,
     )
@@ -2651,7 +2663,7 @@ mod tests {
         build_session_options_and_crate_config,
         to_crate_config
     };
-    use crate::session::config::{LtoCli, LinkerPluginLto, PgoGenerate};
+    use crate::session::config::{LtoCli, LinkerPluginLto, PgoGenerate, ExternEntry};
     use crate::session::build_session;
     use crate::session::search_paths::SearchPath;
     use std::collections::{BTreeMap, BTreeSet};
@@ -2664,6 +2676,19 @@ mod tests {
     use syntax;
     use super::Options;
 
+    impl ExternEntry {
+        fn new_public<S: Into<String>,
+                      I: IntoIterator<Item = Option<S>>>(locations: I) -> ExternEntry {
+            let locations: BTreeSet<_> = locations.into_iter().map(|o| o.map(|s| s.into()))
+                .collect();
+
+            ExternEntry {
+                locations,
+                is_private_dep: false
+            }
+        }
+    }
+
     fn optgroups() -> getopts::Options {
         let mut opts = getopts::Options::new();
         for group in super::rustc_optgroups() {
@@ -2676,10 +2701,6 @@ mod tests {
         BTreeMap::from_iter(entries.into_iter())
     }
 
-    fn mk_set<V: Ord>(entries: Vec<V>) -> BTreeSet<V> {
-        BTreeSet::from_iter(entries.into_iter())
-    }
-
     // When the user supplies --test we should implicitly supply --cfg test
     #[test]
     fn test_switch_implies_cfg_test() {
@@ -2797,33 +2818,33 @@ mod tests {
         v1.externs = Externs::new(mk_map(vec![
             (
                 String::from("a"),
-                mk_set(vec![Some(String::from("b")), Some(String::from("c"))]),
+                ExternEntry::new_public(vec![Some("b"), Some("c")])
             ),
             (
                 String::from("d"),
-                mk_set(vec![Some(String::from("e")), Some(String::from("f"))]),
+                ExternEntry::new_public(vec![Some("e"), Some("f")])
             ),
         ]));
 
         v2.externs = Externs::new(mk_map(vec![
             (
                 String::from("d"),
-                mk_set(vec![Some(String::from("e")), Some(String::from("f"))]),
+                ExternEntry::new_public(vec![Some("e"), Some("f")])
             ),
             (
                 String::from("a"),
-                mk_set(vec![Some(String::from("b")), Some(String::from("c"))]),
+                ExternEntry::new_public(vec![Some("b"), Some("c")])
             ),
         ]));
 
         v3.externs = Externs::new(mk_map(vec![
             (
                 String::from("a"),
-                mk_set(vec![Some(String::from("b")), Some(String::from("c"))]),
+                ExternEntry::new_public(vec![Some("b"), Some("c")])
             ),
             (
                 String::from("d"),
-                mk_set(vec![Some(String::from("f")), Some(String::from("e"))]),
+                ExternEntry::new_public(vec![Some("f"), Some("e")])
             ),
         ]));
 

src/librustc/ty/context.rs

@@ -1397,6 +1397,16 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
+    /// Returns whether or not the crate with CrateNum 'cnum'
+    /// is marked as a private dependency
+    pub fn is_private_dep(self, cnum: CrateNum) -> bool {
+        if cnum == LOCAL_CRATE {
+            false
+        } else {
+            self.cstore.crate_is_private_dep_untracked(cnum)
+        }
+    }
+
     #[inline]
     pub fn def_path_hash(self, def_id: DefId) -> hir_map::DefPathHash {
         if def_id.is_local() {

src/librustc_metadata/creader.rs

@@ -131,9 +131,9 @@ impl<'a> CrateLoader<'a> {
             // `source` stores paths which are normalized which may be different
             // from the strings on the command line.
             let source = &self.cstore.get_crate_data(cnum).source;
-            if let Some(locs) = self.sess.opts.externs.get(&*name.as_str()) {
+            if let Some(entry) = self.sess.opts.externs.get(&*name.as_str()) {
                 // Only use `--extern crate_name=path` here, not `--extern crate_name`.
-                let found = locs.iter().filter_map(|l| l.as_ref()).any(|l| {
+                let found = entry.locations.iter().filter_map(|l| l.as_ref()).any(|l| {
                     let l = fs::canonicalize(l).ok();
                     source.dylib.as_ref().map(|p| &p.0) == l.as_ref() ||
                     source.rlib.as_ref().map(|p| &p.0) == l.as_ref()
@@ -195,12 +195,20 @@ impl<'a> CrateLoader<'a> {
         ident: Symbol,
         span: Span,
         lib: Library,
-        dep_kind: DepKind
+        dep_kind: DepKind,
+        name: Symbol
     ) -> (CrateNum, Lrc<cstore::CrateMetadata>) {
         let crate_root = lib.metadata.get_root();
-        info!("register crate `extern crate {} as {}`", crate_root.name, ident);
         self.verify_no_symbol_conflicts(span, &crate_root);
 
+        let private_dep = self.sess.opts.externs.get(&name.as_str())
+            .map(|e| e.is_private_dep)
+            .unwrap_or(false);
+
+        info!("register crate `extern crate {} as {}` (private_dep = {})",
+            crate_root.name, ident, private_dep);
+
+
         // Claim this crate number and cache it
         let cnum = self.cstore.alloc_new_crate_num();
 
@@ -272,7 +280,8 @@ impl<'a> CrateLoader<'a> {
                 dylib,
                 rlib,
                 rmeta,
-            }
+            },
+            private_dep
         };
 
         let cmeta = Lrc::new(cmeta);
@@ -390,7 +399,7 @@ impl<'a> CrateLoader<'a> {
                 Ok((cnum, data))
             }
             (LoadResult::Loaded(library), host_library) => {
-                Ok(self.register_crate(host_library, root, ident, span, library, dep_kind))
+                Ok(self.register_crate(host_library, root, ident, span, library, dep_kind, name))
             }
             _ => panic!()
         }

src/librustc_metadata/cstore.rs

@@ -79,6 +79,10 @@ pub struct CrateMetadata {
     pub source: CrateSource,
 
     pub proc_macros: Option<Vec<(ast::Name, Lrc<SyntaxExtension>)>>,
+
+    /// Whether or not this crate should be consider a private dependency
+    /// for purposes of the 'exported_private_dependencies' lint
+    pub private_dep: bool
 }
 
 pub struct CStore {
@@ -114,7 +118,8 @@ impl CStore {
     }
 
     pub(super) fn get_crate_data(&self, cnum: CrateNum) -> Lrc<CrateMetadata> {
-        self.metas.borrow()[cnum].clone().unwrap()
+        self.metas.borrow()[cnum].clone()
+            .unwrap_or_else(|| panic!("Failed to get crate data for {:?}", cnum))
     }
 
     pub(super) fn set_crate_data(&self, cnum: CrateNum, data: Lrc<CrateMetadata>) {

src/librustc_metadata/cstore_impl.rs

@@ -499,6 +499,10 @@ impl CrateStore for cstore::CStore {
         self.get_crate_data(cnum).name
     }
 
+    fn crate_is_private_dep_untracked(&self, cnum: CrateNum) -> bool {
+        self.get_crate_data(cnum).private_dep
+    }
+
     fn crate_disambiguator_untracked(&self, cnum: CrateNum) -> CrateDisambiguator
     {
         self.get_crate_data(cnum).root.disambiguator

src/librustc_metadata/locator.rs

@@ -442,11 +442,11 @@ impl<'a> Context<'a> {
         // must be loaded via -L plus some filtering.
         if self.hash.is_none() {
             self.should_match_name = false;
-            if let Some(s) = self.sess.opts.externs.get(&self.crate_name.as_str()) {
+            if let Some(entry) = self.sess.opts.externs.get(&self.crate_name.as_str()) {
                 // Only use `--extern crate_name=path` here, not `--extern crate_name`.
-                if s.iter().any(|l| l.is_some()) {
+                if entry.locations.iter().any(|l| l.is_some()) {
                     return self.find_commandline_library(
-                        s.iter().filter_map(|l| l.as_ref()),
+                        entry.locations.iter().filter_map(|l| l.as_ref()),
                     );
                 }
             }

src/librustc_privacy/lib.rs

@@ -1540,7 +1540,6 @@ struct SearchInterfaceForPrivateItemsVisitor<'a, 'tcx: 'a> {
     has_pub_restricted: bool,
     has_old_errors: bool,
     in_assoc_ty: bool,
-    private_crates: FxHashSet<CrateNum>
 }
 
 impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
@@ -1622,7 +1621,7 @@ impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
     /// 2. It comes from a private crate
     fn leaks_private_dep(&self, item_id: DefId) -> bool {
         let ret = self.required_visibility == ty::Visibility::Public &&
-            self.private_crates.contains(&item_id.krate);
+            self.tcx.is_private_dep(item_id.krate);
 
         log::debug!("leaks_private_dep(item_id={:?})={}", item_id, ret);
         return ret;
@@ -1640,7 +1639,6 @@ struct PrivateItemsInPublicInterfacesVisitor<'a, 'tcx: 'a> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
     has_pub_restricted: bool,
     old_error_set: &'a HirIdSet,
-    private_crates: FxHashSet<CrateNum>
 }
 
 impl<'a, 'tcx> PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {
@@ -1678,7 +1676,6 @@ impl<'a, 'tcx> PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {
             has_pub_restricted: self.has_pub_restricted,
             has_old_errors,
             in_assoc_ty: false,
-            private_crates: self.private_crates.clone()
         }
     }
 
@@ -1876,17 +1873,11 @@ fn check_private_in_public<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>, krate: CrateNum) {
         pub_restricted_visitor.has_pub_restricted
     };
 
-    let private_crates: FxHashSet<CrateNum> = tcx.sess.opts.extern_private.iter()
-        .flat_map(|c| {
-            tcx.crates().iter().find(|&&krate| &tcx.crate_name(krate) == c).cloned()
-        }).collect();
-
     // Check for private types and traits in public interfaces.
     let mut visitor = PrivateItemsInPublicInterfacesVisitor {
         tcx,
         has_pub_restricted,
         old_error_set: &visitor.old_error_set,
-        private_crates
     };
     krate.visit_all_item_likes(&mut DeepVisitor::new(&mut visitor));
 }

src/librustdoc/config.rs

@@ -1,4 +1,4 @@
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::BTreeMap;
 use std::fmt;
 use std::path::PathBuf;
 
@@ -9,7 +9,7 @@ use rustc::lint::Level;
 use rustc::session::early_error;
 use rustc::session::config::{CodegenOptions, DebuggingOptions, ErrorOutputType, Externs};
 use rustc::session::config::{nightly_options, build_codegen_options, build_debugging_options,
-                             get_cmd_lint_options};
+                             get_cmd_lint_options, ExternEntry};
 use rustc::session::search_paths::SearchPath;
 use rustc_driver;
 use rustc_target::spec::TargetTriple;
@@ -578,7 +578,7 @@ fn parse_extern_html_roots(
 /// error message.
 // FIXME(eddyb) This shouldn't be duplicated with `rustc::session`.
 fn parse_externs(matches: &getopts::Matches) -> Result<Externs, String> {
-    let mut externs: BTreeMap<_, BTreeSet<_>> = BTreeMap::new();
+    let mut externs: BTreeMap<_, ExternEntry> = BTreeMap::new();
     for arg in &matches.opt_strs("extern") {
         let mut parts = arg.splitn(2, '=');
         let name = parts.next().ok_or("--extern value must not be empty".to_string())?;
@@ -588,7 +588,10 @@ fn parse_externs(matches: &getopts::Matches) -> Result<Externs, String> {
                         enable `--extern crate_name` without `=path`".to_string());
         }
         let name = name.to_string();
-        externs.entry(name).or_default().insert(location);
+        // For Rustdoc purposes, we can treat all externs as public
+        externs.entry(name)
+            .or_default()
+            .locations.insert(location.clone());
     }
     Ok(Externs::new(externs))
 }