Auto merge of #38932 - petrochenkov:privctor, r=jseyfried

Privatize constructors of tuple structs with private fields

This PR implements the strictest version of such "privatization" - it just sets visibilities for struct constructors, this affects everything including imports.
```
visibility(struct_ctor) = min(visibility(struct), visibility(field_1), ..., visibility(field_N))
```
Needs crater run before proceeding.

Resolves rust-lang/rfcs#902

r? @nikomatsakis

Author: bors

src/librustc/lint/builtin.rs

@@ -223,6 +223,12 @@ declare_lint! {
     "detects names that resolve to ambiguous glob imports with RFC 1560"
 }
 
+declare_lint! {
+    pub LEGACY_CONSTRUCTOR_VISIBILITY,
+    Deny,
+    "detects use of struct constructors that would be invisible with new visibility rules"
+}
+
 declare_lint! {
     pub DEPRECATED,
     Warn,
@@ -271,6 +277,7 @@ impl LintPass for HardwiredLints {
             EXTRA_REQUIREMENT_IN_IMPL,
             LEGACY_DIRECTORY_OWNERSHIP,
             LEGACY_IMPORTS,
+            LEGACY_CONSTRUCTOR_VISIBILITY,
             DEPRECATED
         )
     }

src/librustc_lint/lib.rs

@@ -240,6 +240,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             id: LintId::of(LEGACY_IMPORTS),
             reference: "issue #38260 <https://github.com/rust-lang/rust/issues/38260>",
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(LEGACY_CONSTRUCTOR_VISIBILITY),
+            reference: "issue #39207 <https://github.com/rust-lang/rust/issues/39207>",
+        },
         ]);
 
     // Register renamed and removed lints

src/librustc_metadata/encoder.rs

@@ -396,10 +396,16 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
 
         let struct_id = tcx.hir.as_local_node_id(adt_def_id).unwrap();
         let struct_vis = &tcx.hir.expect_item(struct_id).vis;
+        let mut ctor_vis = ty::Visibility::from_hir(struct_vis, struct_id, tcx);
+        for field in &variant.fields {
+            if ctor_vis.is_at_least(field.vis, tcx) {
+                ctor_vis = field.vis;
+            }
+        }
 
         Entry {
             kind: EntryKind::Struct(self.lazy(&data)),
-            visibility: self.lazy(&ty::Visibility::from_hir(struct_vis, struct_id, tcx)),
+            visibility: self.lazy(&ctor_vis),
             span: self.lazy(&tcx.def_span(def_id)),
             attributes: LazySeq::empty(),
             children: LazySeq::empty(),

src/librustc_privacy/diagnostics.rs

@@ -115,45 +115,6 @@ pub enum Foo {
 ```
 "##,
 
-E0450: r##"
-A tuple constructor was invoked while some of its fields are private. Erroneous
-code example:
-
-```compile_fail,E0450
-mod Bar {
-    pub struct Foo(isize);
-}
-
-let f = Bar::Foo(0); // error: cannot invoke tuple struct constructor with
-                     //        private fields
-```
-
-To solve this issue, please ensure that all of the fields of the tuple struct
-are public. Alternatively, provide a `new()` method to the tuple struct to
-construct it from a given inner value. Example:
-
-```
-mod Bar {
-    pub struct Foo(pub isize); // we set its field to public
-}
-
-let f = Bar::Foo(0); // ok!
-
-// or:
-mod bar {
-    pub struct Foo(isize);
-
-    impl Foo {
-        pub fn new(x: isize) -> Foo {
-            Foo(x)
-        }
-    }
-}
-
-let f = bar::Foo::new(1);
-```
-"##,
-
 E0451: r##"
 A struct constructor with private fields was invoked. Erroneous code example:
 
@@ -204,3 +165,7 @@ let f = Bar::Foo::new(); // ok!
 "##,
 
 }
+
+register_diagnostics! {
+//  E0450, moved into resolve
+}

src/librustc_privacy/lib.rs

@@ -27,7 +27,7 @@ extern crate syntax_pos;
 
 use rustc::dep_graph::DepNode;
 use rustc::hir::{self, PatKind};
-use rustc::hir::def::{self, Def, CtorKind};
+use rustc::hir::def::{self, Def};
 use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId};
 use rustc::hir::intravisit::{self, Visitor, NestedVisitorMap};
 use rustc::hir::itemlikevisit::DeepVisitor;
@@ -478,33 +478,6 @@ impl<'a, 'tcx> Visitor<'tcx> for PrivacyVisitor<'a, 'tcx> {
                     }
                 }
             }
-            hir::ExprPath(hir::QPath::Resolved(_, ref path)) => {
-                if let Def::StructCtor(_, CtorKind::Fn) = path.def {
-                    let adt_def = self.tcx.expect_variant_def(path.def);
-                    let private_indexes = adt_def.fields.iter().enumerate().filter(|&(_, field)| {
-                        !field.vis.is_accessible_from(self.curitem, self.tcx)
-                    }).map(|(i, _)| i).collect::<Vec<_>>();
-
-                    if !private_indexes.is_empty() {
-                        let mut error = struct_span_err!(self.tcx.sess, expr.span, E0450,
-                                                         "cannot invoke tuple struct constructor \
-                                                          with private fields");
-                        error.span_label(expr.span,
-                                         &format!("cannot construct with a private field"));
-
-                        if let Some(node_id) = self.tcx.hir.as_local_node_id(adt_def.did) {
-                            let node = self.tcx.hir.find(node_id);
-                            if let Some(hir::map::NodeStructCtor(vdata)) = node {
-                                for i in private_indexes {
-                                    error.span_label(vdata.fields()[i].span,
-                                                     &format!("private field declared here"));
-                                }
-                            }
-                        }
-                        error.emit();
-                    }
-                }
-            }
             _ => {}
         }
 

src/librustc_resolve/build_reduced_graph.rs

@@ -327,21 +327,26 @@ impl<'a> Resolver<'a> {
                 let def = Def::Struct(self.definitions.local_def_id(item.id));
                 self.define(parent, ident, TypeNS, (def, vis, sp, expansion));
 
-                // If this is a tuple or unit struct, define a name
-                // in the value namespace as well.
-                if !struct_def.is_struct() {
-                    let ctor_def = Def::StructCtor(self.definitions.local_def_id(struct_def.id()),
-                                                   CtorKind::from_ast(struct_def));
-                    self.define(parent, ident, ValueNS, (ctor_def, vis, sp, expansion));
-                }
-
                 // Record field names for error reporting.
+                let mut ctor_vis = vis;
                 let field_names = struct_def.fields().iter().filter_map(|field| {
-                    self.resolve_visibility(&field.vis);
+                    let field_vis = self.resolve_visibility(&field.vis);
+                    if ctor_vis.is_at_least(field_vis, &*self) {
+                        ctor_vis = field_vis;
+                    }
                     field.ident.map(|ident| ident.name)
                 }).collect();
                 let item_def_id = self.definitions.local_def_id(item.id);
                 self.insert_field_names(item_def_id, field_names);
+
+                // If this is a tuple or unit struct, define a name
+                // in the value namespace as well.
+                if !struct_def.is_struct() {
+                    let ctor_def = Def::StructCtor(self.definitions.local_def_id(struct_def.id()),
+                                                   CtorKind::from_ast(struct_def));
+                    self.define(parent, ident, ValueNS, (ctor_def, ctor_vis, sp, expansion));
+                    self.struct_constructors.insert(def.def_id(), (ctor_def, ctor_vis));
+                }
             }
 
             ItemKind::Union(ref vdata, _) => {
@@ -434,9 +439,17 @@ impl<'a> Resolver<'a> {
             Def::Variant(..) | Def::TyAlias(..) => {
                 self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
             }
-            Def::Fn(..) | Def::Static(..) | Def::Const(..) |
-            Def::VariantCtor(..) | Def::StructCtor(..) => {
+            Def::Fn(..) | Def::Static(..) | Def::Const(..) | Def::VariantCtor(..) => {
+                self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
+            }
+            Def::StructCtor(..) => {
                 self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
+
+                if let Some(struct_def_id) =
+                        self.session.cstore.def_key(def_id).parent
+                            .map(|index| DefId { krate: def_id.krate, index: index }) {
+                    self.struct_constructors.insert(struct_def_id, (def, vis));
+                }
             }
             Def::Trait(..) => {
                 let module_kind = ModuleKind::Def(def, ident.name);

src/librustc_resolve/lib.rs

@@ -45,7 +45,7 @@ use rustc::hir::def::*;
 use rustc::hir::def_id::{CrateNum, CRATE_DEF_INDEX, LOCAL_CRATE, DefId};
 use rustc::ty;
 use rustc::hir::{Freevar, FreevarMap, TraitCandidate, TraitMap, GlobMap};
-use rustc::util::nodemap::{NodeMap, NodeSet, FxHashMap, FxHashSet};
+use rustc::util::nodemap::{NodeMap, NodeSet, FxHashMap, FxHashSet, DefIdMap};
 
 use syntax::ext::hygiene::{Mark, SyntaxContext};
 use syntax::ast::{self, Name, NodeId, Ident, SpannedIdent, FloatTy, IntTy, UintTy};
@@ -1131,6 +1131,10 @@ pub struct Resolver<'a> {
     warned_proc_macros: FxHashSet<Name>,
 
     potentially_unused_imports: Vec<&'a ImportDirective<'a>>,
+
+    // This table maps struct IDs into struct constructor IDs,
+    // it's not used during normal resolution, only for better error reporting.
+    struct_constructors: DefIdMap<(Def, ty::Visibility)>,
 }
 
 pub struct ResolverArenas<'a> {
@@ -1310,6 +1314,7 @@ impl<'a> Resolver<'a> {
             proc_macro_enabled: features.proc_macro,
             warned_proc_macros: FxHashSet(),
             potentially_unused_imports: Vec::new(),
+            struct_constructors: DefIdMap(),
         }
     }
 
@@ -2205,6 +2210,15 @@ impl<'a> Resolver<'a> {
                         _ => {}
                     },
                     _ if ns == ValueNS && is_struct_like(def) => {
+                        if let Def::Struct(def_id) = def {
+                            if let Some((ctor_def, ctor_vis))
+                                    = this.struct_constructors.get(&def_id).cloned() {
+                                if is_expected(ctor_def) && !this.is_accessible(ctor_vis) {
+                                    err.span_label(span, &format!("constructor is not visible \
+                                                                   here due to private fields"));
+                                }
+                            }
+                        }
                         err.span_label(span, &format!("did you mean `{} {{ /* fields */ }}`?",
                                                        path_str));
                         return err;
@@ -2235,7 +2249,23 @@ impl<'a> Resolver<'a> {
                 if is_expected(resolution.base_def) || resolution.base_def == Def::Err {
                     resolution
                 } else {
-                    report_errors(self, Some(resolution.base_def))
+                    // Add a temporary hack to smooth the transition to new struct ctor
+                    // visibility rules. See #38932 for more details.
+                    let mut res = None;
+                    if let Def::Struct(def_id) = resolution.base_def {
+                        if let Some((ctor_def, ctor_vis))
+                                = self.struct_constructors.get(&def_id).cloned() {
+                            if is_expected(ctor_def) && self.is_accessible(ctor_vis) {
+                                let lint = lint::builtin::LEGACY_CONSTRUCTOR_VISIBILITY;
+                                self.session.add_lint(lint, id, span,
+                                    "private struct constructors are not usable through \
+                                     reexports in outer modules".to_string());
+                                res = Some(PathResolution::new(ctor_def));
+                            }
+                        }
+                    }
+
+                    res.unwrap_or_else(|| report_errors(self, Some(resolution.base_def)))
                 }
             }
             Some(resolution) if source.defer_to_typeck() => {

src/librustdoc/html/markdown.rs

@@ -660,8 +660,6 @@ mod tests {
         t("no_run",                false,        true,   false,  true,  false, false, Vec::new());
         t("test_harness",          false,        false,  false,  true,  true,  false, Vec::new());
         t("compile_fail",          false,        true,   false,  true,  false, true,  Vec::new());
-        t("E0450",                 false,        false,  false,  true,  false, false,
-                                   vec!["E0450".to_owned()]);
         t("{.no_run .example}",    false,        true,   false,  true,  false, false, Vec::new());
         t("{.sh .should_panic}",   true,         false,  false,  true,  false, false, Vec::new());
         t("{.example .rust}",      false,        false,  false,  true,  false, false, Vec::new());

src/libsyntax/symbol.rs

@@ -140,7 +140,7 @@ macro_rules! declare_keywords {(
         $(
             #[allow(non_upper_case_globals)]
             pub const $konst: Keyword = Keyword {
-                ident: ast::Ident::with_empty_ctxt(ast::Name($index))
+                ident: ast::Ident::with_empty_ctxt(super::Symbol($index))
             };
         )*
     }
@@ -282,25 +282,24 @@ impl Encodable for InternedString {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use ast::Name;
 
     #[test]
     fn interner_tests() {
         let mut i: Interner = Interner::new();
         // first one is zero:
-        assert_eq!(i.intern("dog"), Name(0));
+        assert_eq!(i.intern("dog"), Symbol(0));
         // re-use gets the same entry:
-        assert_eq!(i.intern ("dog"), Name(0));
+        assert_eq!(i.intern ("dog"), Symbol(0));
         // different string gets a different #:
-        assert_eq!(i.intern("cat"), Name(1));
-        assert_eq!(i.intern("cat"), Name(1));
+        assert_eq!(i.intern("cat"), Symbol(1));
+        assert_eq!(i.intern("cat"), Symbol(1));
         // dog is still at zero
-        assert_eq!(i.intern("dog"), Name(0));
+        assert_eq!(i.intern("dog"), Symbol(0));
         // gensym gets 3
-        assert_eq!(i.gensym("zebra"), Name(2));
+        assert_eq!(i.gensym("zebra"), Symbol(2));
         // gensym of same string gets new number :
-        assert_eq!(i.gensym("zebra"), Name(3));
+        assert_eq!(i.gensym("zebra"), Symbol(3));
         // gensym of *existing* string gets new number:
-        assert_eq!(i.gensym("dog"), Name(4));
+        assert_eq!(i.gensym("dog"), Symbol(4));
     }
 }

src/test/compile-fail-fulldeps/explore-issue-38412.rs

@@ -25,21 +25,14 @@ use pub_and_stability::{Record, Trait, Tuple};
 fn main() {
     // Okay
     let Record { .. } = Record::new();
-    // Okay (for now; see RFC Issue #902)
-    let Tuple(..) = Tuple::new();
 
     // Okay
     let Record { a_stable_pub: _, a_unstable_declared_pub: _, .. } = Record::new();
-    // Okay (for now; see RFC Issue #902)
-    let Tuple(_, _, ..) = Tuple::new(); // analogous to above
 
     let Record { a_stable_pub: _, a_unstable_declared_pub: _, a_unstable_undeclared_pub: _, .. } =
         Record::new();
     //~^^ ERROR use of unstable library feature 'unstable_undeclared'
 
-    let Tuple(_, _, _, ..) = Tuple::new(); // analogous to previous
-    //~^ ERROR use of unstable library feature 'unstable_undeclared'
-
     let r = Record::new();
     let t = Tuple::new();