Improve diagnostics for inaccessible constructors

petrochenkov · petrochenkov · commit d38a8ad48804 · 2017-01-29T02:57:14.000+03:00
diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs
@@ -345,9 +345,7 @@ impl<'a> Resolver<'a> {
                     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));
-                    if !ctor_vis.is_at_least(vis, &*self) {
-                        self.legacy_ctor_visibilities.insert(def.def_id(), (ctor_def, ctor_vis));
-                    }
+                    self.struct_constructors.insert(def.def_id(), (ctor_def, ctor_vis));
                 }
             }
 
@@ -441,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);
diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs
@@ -1132,8 +1132,9 @@ pub struct Resolver<'a> {
 
     potentially_unused_imports: Vec<&'a ImportDirective<'a>>,
 
-    // Auxiliary map used only for reporting `legacy_constructor_visibility` lint.
-    legacy_ctor_visibilities: DefIdMap<(Def, ty::Visibility)>,
+    // 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> {
@@ -1313,7 +1314,7 @@ impl<'a> Resolver<'a> {
             proc_macro_enabled: features.proc_macro,
             warned_proc_macros: FxHashSet(),
             potentially_unused_imports: Vec::new(),
-            legacy_ctor_visibilities: DefIdMap(),
+            struct_constructors: DefIdMap(),
         }
     }
 
@@ -2209,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;
@@ -2244,7 +2254,7 @@ impl<'a> Resolver<'a> {
                     let mut res = None;
                     if let Def::Struct(def_id) = resolution.base_def {
                         if let Some((ctor_def, ctor_vis))
-                                = self.legacy_ctor_visibilities.get(&def_id).cloned() {
+                                = 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,
diff --git a/src/test/compile-fail/issue-38412.rs b/src/test/compile-fail/issue-38412.rs
@@ -11,6 +11,7 @@
 fn main() {
     let Box(a) = loop { };
     //~^ ERROR expected tuple struct/variant, found struct `Box`
+    //~| ERROR expected tuple struct/variant, found struct `Box`
 
     // (The below is a trick to allow compiler to infer a type for
     // variable `a` without attempting to ascribe a type to the
diff --git a/src/test/ui/resolve/auxiliary/privacy-struct-ctor.rs b/src/test/ui/resolve/auxiliary/privacy-struct-ctor.rs
diff --git a/src/test/ui/resolve/privacy-struct-ctor.rs b/src/test/ui/resolve/privacy-struct-ctor.rs
@@ -25,18 +25,24 @@ mod m {
 
     fn f() {
         n::Z; //~ ERROR tuple struct `Z` is private
-        Z; //~ ERROR expected value, found struct `Z`
+        Z;
+        //~^ ERROR expected value, found struct `Z`
+        //~| NOTE tuple struct constructors with private fields are invisible outside of their mod
     }
 }
 
 use m::S; // OK, only the type is imported
 
 fn main() {
     m::S; //~ ERROR tuple struct `S` is private
-    S; //~ ERROR expected value, found struct `S`
+    S;
+    //~^ ERROR expected value, found struct `S`
+    //~| NOTE constructor is not visible here due to private fields
     m::n::Z; //~ ERROR tuple struct `Z` is private
 
     xcrate::m::S; //~ ERROR tuple struct `S` is private
-    xcrate::S; //~ ERROR expected value, found struct `xcrate::S`
+    xcrate::S;
+    //~^ ERROR expected value, found struct `xcrate::S`
+    //~| NOTE constructor is not visible here due to private fields
     xcrate::m::n::Z; //~ ERROR tuple struct `Z` is private
 }
diff --git a/src/test/ui/resolve/privacy-struct-ctor.stderr b/src/test/ui/resolve/privacy-struct-ctor.stderr
@@ -0,0 +1,68 @@
+error[E0423]: expected value, found struct `Z`
+  --> $DIR/privacy-struct-ctor.rs:28:9
+   |
+28 |         Z;
+   |         ^
+   |         |
+   |         did you mean `Z { /* fields */ }`?
+   |         constructor is not visible here due to private fields
+   |
+   = help: possible better candidate is found in another module, you can import it into scope:
+             `use m::n::Z;`
+
+error[E0423]: expected value, found struct `S`
+  --> $DIR/privacy-struct-ctor.rs:38:5
+   |
+38 |     S;
+   |     ^
+   |     |
+   |     did you mean `S { /* fields */ }`?
+   |     constructor is not visible here due to private fields
+   |
+   = help: possible better candidate is found in another module, you can import it into scope:
+             `use m::S;`
+
+error[E0423]: expected value, found struct `xcrate::S`
+  --> $DIR/privacy-struct-ctor.rs:44:5
+   |
+44 |     xcrate::S;
+   |     ^^^^^^^^^
+   |     |
+   |     did you mean `xcrate::S { /* fields */ }`?
+   |     constructor is not visible here due to private fields
+   |
+   = help: possible better candidate is found in another module, you can import it into scope:
+             `use m::S;`
+
+error: tuple struct `Z` is private
+  --> $DIR/privacy-struct-ctor.rs:27:9
+   |
+27 |         n::Z; //~ ERROR tuple struct `Z` is private
+   |         ^^^^
+
+error: tuple struct `S` is private
+  --> $DIR/privacy-struct-ctor.rs:37:5
+   |
+37 |     m::S; //~ ERROR tuple struct `S` is private
+   |     ^^^^
+
+error: tuple struct `Z` is private
+  --> $DIR/privacy-struct-ctor.rs:41:5
+   |
+41 |     m::n::Z; //~ ERROR tuple struct `Z` is private
+   |     ^^^^^^^
+
+error: tuple struct `S` is private
+  --> $DIR/privacy-struct-ctor.rs:43:5
+   |
+43 |     xcrate::m::S; //~ ERROR tuple struct `S` is private
+   |     ^^^^^^^^^^^^
+
+error: tuple struct `Z` is private
+  --> $DIR/privacy-struct-ctor.rs:47:5
+   |
+47 |     xcrate::m::n::Z; //~ ERROR tuple struct `Z` is private
+   |     ^^^^^^^^^^^^^^^
+
+error: aborting due to 8 previous errors
+
