Change private structs to have private fields by default

This was the original intention of the privacy of structs, and it was
erroneously implemented before. A pub struct will now have default-pub fields,
and a non-pub struct will have default-priv fields. This essentially brings
struct fields in line with enum variants in terms of inheriting visibility.

As usual, extraneous modifiers to visibility are disallowed depend on the case
that you're dealing with.

Closes #11522

3 files changed, 136 insertions, 32 deletions

diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs
index 526fb9e8d86..052a6ad898f 100644
--- a/src/librustc/middle/privacy.rs
+++ b/src/librustc/middle/privacy.rs
@@ -15,6 +15,7 @@
 use std::hashmap::{HashSet, HashMap};
 use std::util;
 
+use metadata::csearch;
 use middle::resolve;
 use middle::ty;
 use middle::typeck::{method_map, method_origin, method_param};
@@ -123,22 +124,7 @@ impl Visitor<()> for ParentVisitor {
         // While we have the id of the struct definition, go ahead and parent
         // all the fields.
         for field in s.fields.iter() {
-            let vis = match field.node.kind {
-                ast::NamedField(_, vis) => vis,
-                ast::UnnamedField => continue
-            };
-
-            // Private fields are scoped to this module, so parent them directly
-            // to the module instead of the struct. This is similar to the case
-            // of private enum variants.
-            if vis == ast::Private {
-                self.parents.insert(field.node.id, self.curparent);
-
-            // Otherwise public fields are scoped to the visibility of the
-            // struct itself
-            } else {
-                self.parents.insert(field.node.id, n);
-            }
+            self.parents.insert(field.node.id, self.curparent);
         }
         visit::walk_struct_def(self, s, i, g, n, ())
     }
@@ -558,12 +544,48 @@ impl<'a> PrivacyVisitor<'a> {
 
     // Checks that a field is in scope.
     // FIXME #6993: change type (and name) from Ident to Name
-    fn check_field(&mut self, span: Span, id: ast::DefId, ident: ast::Ident) {
+    fn check_field(&mut self, span: Span, id: ast::DefId, ident: ast::Ident,
+                   enum_id: Option<ast::DefId>) {
         let fields = ty::lookup_struct_fields(self.tcx, id);
+        let struct_vis = if is_local(id) {
+            match self.tcx.items.get(id.node) {
+                ast_map::NodeItem(ref it, _) => it.vis,
+                ast_map::NodeVariant(ref v, ref it, _) => {
+                    if v.node.vis == ast::Inherited {it.vis} else {v.node.vis}
+                }
+                _ => {
+                    self.tcx.sess.span_bug(span,
+                                           format!("not an item or variant def"));
+                }
+            }
+        } else {
+            let cstore = self.tcx.sess.cstore;
+            match enum_id {
+                Some(enum_id) => {
+                    let v = csearch::get_enum_variants(self.tcx, enum_id);
+                    match v.iter().find(|v| v.id == id) {
+                        Some(variant) => {
+                            if variant.vis == ast::Inherited {
+                                csearch::get_item_visibility(cstore, enum_id)
+                            } else {
+                                variant.vis
+                            }
+                        }
+                        None => {
+                            self.tcx.sess.span_bug(span, "no xcrate variant");
+                        }
+                    }
+                }
+                None => csearch::get_item_visibility(cstore, id)
+            }
+        };
+
         for field in fields.iter() {
             if field.name != ident.name { continue; }
-            // public fields are public everywhere
-            if field.vis != ast::Private { break }
+            // public structs have public fields by default, and private structs
+            // have private fields by default.
+            if struct_vis == ast::Public && field.vis != ast::Private { break }
+            if struct_vis != ast::Public && field.vis == ast::Public { break }
             if !is_local(field.id) ||
                !self.private_accessible(field.id.node) {
                 self.tcx.sess.span_err(span, format!("field `{}` is private",
@@ -661,7 +683,7 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                 let t = ty::type_autoderef(ty::expr_ty(self.tcx, base));
                 match ty::get(t).sty {
                     ty::ty_struct(id, _) => {
-                        self.check_field(expr.span, id, ident);
+                        self.check_field(expr.span, id, ident, None);
                     }
                     _ => {}
                 }
@@ -690,16 +712,18 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                 match ty::get(ty::expr_ty(self.tcx, expr)).sty {
                     ty::ty_struct(id, _) => {
                         for field in (*fields).iter() {
-                            self.check_field(expr.span, id, field.ident.node);
+                            self.check_field(expr.span, id, field.ident.node,
+                                             None);
                         }
                     }
                     ty::ty_enum(_, _) => {
                         let def_map = self.tcx.def_map.borrow();
                         match def_map.get().get_copy(&expr.id) {
-                            ast::DefVariant(_, variant_id, _) => {
+                            ast::DefVariant(enum_id, variant_id, _) => {
                                 for field in fields.iter() {
                                     self.check_field(expr.span, variant_id,
-                                                     field.ident.node);
+                                                     field.ident.node,
+                                                     Some(enum_id));
                                 }
                             }
                             _ => self.tcx.sess.span_bug(expr.span,
@@ -763,16 +787,17 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                 match ty::get(ty::pat_ty(self.tcx, pattern)).sty {
                     ty::ty_struct(id, _) => {
                         for field in fields.iter() {
-                            self.check_field(pattern.span, id, field.ident);
+                            self.check_field(pattern.span, id, field.ident,
+                                             None);
                         }
                     }
                     ty::ty_enum(_, _) => {
                         let def_map = self.tcx.def_map.borrow();
                         match def_map.get().find(&pattern.id) {
-                            Some(&ast::DefVariant(_, variant_id, _)) => {
+                            Some(&ast::DefVariant(enum_id, variant_id, _)) => {
                                 for field in fields.iter() {
                                     self.check_field(pattern.span, variant_id,
-                                                     field.ident);
+                                                     field.ident, Some(enum_id));
                                 }
                             }
                             _ => self.tcx.sess.span_bug(pattern.span,
@@ -888,15 +913,22 @@ impl SanePrivacyVisitor {
                 }
             }
         };
-        let check_struct = |def: &@ast::StructDef| {
+        let check_struct = |def: &@ast::StructDef,
+                            vis: ast::Visibility,
+                            parent_vis: Option<ast::Visibility>| {
+            let public_def = match vis {
+                ast::Public => true,
+                ast::Inherited | ast::Private => parent_vis == Some(ast::Public),
+            };
             for f in def.fields.iter() {
                match f.node.kind {
-                    ast::NamedField(_, ast::Public) => {
+                    ast::NamedField(_, ast::Public) if public_def => {
                         tcx.sess.span_err(f.span, "unnecessary `pub` \
                                                    visibility");
                     }
-                    ast::NamedField(_, ast::Private) => {
-                        // Fields should really be private by default...
+                    ast::NamedField(_, ast::Private) if !public_def => {
+                        tcx.sess.span_err(f.span, "unnecessary `priv` \
+                                                   visibility");
                     }
                     ast::NamedField(..) | ast::UnnamedField => {}
                 }
@@ -951,13 +983,15 @@ impl SanePrivacyVisitor {
                     }
 
                     match v.node.kind {
-                        ast::StructVariantKind(ref s) => check_struct(s),
+                        ast::StructVariantKind(ref s) => {
+                            check_struct(s, v.node.vis, Some(item.vis));
+                        }
                         ast::TupleVariantKind(..) => {}
                     }
                 }
             }
 
-            ast::ItemStruct(ref def, _) => check_struct(def),
+            ast::ItemStruct(ref def, _) => check_struct(def, item.vis, None),
 
             ast::ItemTrait(_, _, ref methods) => {
                 for m in methods.iter() {
diff --git a/src/test/auxiliary/struct-field-privacy.rs b/src/test/auxiliary/struct-field-privacy.rs
new file mode 100644
index 00000000000..497d50a2390
--- /dev/null
+++ b/src/test/auxiliary/struct-field-privacy.rs
@@ -0,0 +1,19 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+struct A {
+    a: int,
+    pub b: int,
+}
+
+pub struct B {
+    a: int,
+    priv b: int,
+}
diff --git a/src/test/compile-fail/struct-field-privacy.rs b/src/test/compile-fail/struct-field-privacy.rs
new file mode 100644
index 00000000000..f3169004785
--- /dev/null
+++ b/src/test/compile-fail/struct-field-privacy.rs
@@ -0,0 +1,51 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// aux-build:struct-field-privacy.rs
+
+extern mod xc = "struct-field-privacy";
+
+struct A {
+    a: int,
+}
+
+mod inner {
+    struct A {
+        a: int,
+        pub b: int,
+        priv c: int, //~ ERROR: unnecessary `priv` visibility
+    }
+    pub struct B {
+        a: int,
+        priv b: int,
+        pub c: int, //~ ERROR: unnecessary `pub` visibility
+    }
+}
+
+fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) {
+    //~^ ERROR: type `A` is private
+    //~^^ ERROR: struct `A` is private
+
+    a.a;
+    b.a; //~ ERROR: field `a` is private
+    b.b;
+    b.c; //~ ERROR: field `c` is private
+    c.a;
+    c.b; //~ ERROR: field `b` is private
+    c.c;
+
+    d.a; //~ ERROR: field `a` is private
+    d.b;
+
+    e.a;
+    e.b; //~ ERROR: field `b` is private
+}
+
+fn main() {}