rustc: Switch tuple structs to have private fields

This is a continuation of the work done in #13184 to make struct fields private
by default. This commit finishes RFC 4 by making all tuple structs have private
fields by default. Note that enum variants are not affected.

A tuple struct having a private field means that it cannot be matched on in a
pattern match (both refutable and irrefutable), and it also cannot have a value
specified to be constructed. Similarly to private fields, switching the type of
a private field in a tuple struct should be able to be done in a backwards
compatible way.

The one snag that I ran into which wasn't mentioned in the RFC is that this
commit also forbids taking the value of a tuple struct constructor. For example,
this code now fails to compile:

    mod a {
        pub struct A(int);
    }

    let a: fn(int) -> a::A = a::A; //~ ERROR: first field is private

Although no fields are bound in this example, it exposes implementation details
through the type itself. For this reason, taking the value of a struct
constructor with private fields is forbidden (outside the containing module).

RFC: 0004-private-fields

6 files changed, 270 insertions, 31 deletions

diff --git a/src/librustc/metadata/csearch.rs b/src/librustc/metadata/csearch.rs
index d840ca32938..8845a2acbd6 100644
--- a/src/librustc/metadata/csearch.rs
+++ b/src/librustc/metadata/csearch.rs
@@ -311,3 +311,11 @@ pub fn get_exported_macros(cstore: &cstore::CStore,
     let cdata = cstore.get_crate_data(crate_num);
     decoder::get_exported_macros(cdata)
 }
+
+pub fn get_tuple_struct_definition_if_ctor(cstore: &cstore::CStore,
+                                           def_id: ast::DefId)
+    -> Option<ast::DefId>
+{
+    let cdata = cstore.get_crate_data(def_id.krate);
+    decoder::get_tuple_struct_definition_if_ctor(cdata, def_id.node)
+}
diff --git a/src/librustc/metadata/decoder.rs b/src/librustc/metadata/decoder.rs
index 94941913a8b..300b8ff2da4 100644
--- a/src/librustc/metadata/decoder.rs
+++ b/src/librustc/metadata/decoder.rs
@@ -962,23 +962,26 @@ pub fn get_static_methods_if_impl(intr: Rc<IdentInterner>,
 /// If node_id is the constructor of a tuple struct, retrieve the NodeId of
 /// the actual type definition, otherwise, return None
 pub fn get_tuple_struct_definition_if_ctor(cdata: Cmd,
-                                           node_id: ast::NodeId) -> Option<ast::NodeId> {
+                                           node_id: ast::NodeId)
+    -> Option<ast::DefId>
+{
     let item = lookup_item(node_id, cdata.data());
     let mut ret = None;
     reader::tagged_docs(item, tag_items_data_item_is_tuple_struct_ctor, |_| {
         ret = Some(item_reqd_and_translated_parent_item(cdata.cnum, item));
         false
     });
-    ret.map(|x| x.node)
+    ret
 }
 
 pub fn get_item_attrs(cdata: Cmd,
-                      node_id: ast::NodeId,
+                      orig_node_id: ast::NodeId,
                       f: |Vec<@ast::MetaItem> |) {
     // The attributes for a tuple struct are attached to the definition, not the ctor;
     // we assume that someone passing in a tuple struct ctor is actually wanting to
     // look at the definition
-    let node_id = get_tuple_struct_definition_if_ctor(cdata, node_id).unwrap_or(node_id);
+    let node_id = get_tuple_struct_definition_if_ctor(cdata, orig_node_id);
+    let node_id = node_id.map(|x| x.node).unwrap_or(orig_node_id);
     let item = lookup_item(node_id, cdata.data());
     reader::tagged_docs(item, tag_attributes, |attributes| {
         reader::tagged_docs(attributes, tag_attribute, |attribute| {
diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs
index e5644dbd246..9d9faee3645 100644
--- a/src/librustc/middle/privacy.rs
+++ b/src/librustc/middle/privacy.rs
@@ -14,6 +14,7 @@
 
 use std::mem::replace;
 
+use metadata::csearch;
 use middle::lint;
 use middle::resolve;
 use middle::ty;
@@ -358,6 +359,12 @@ enum PrivacyResult {
     DisallowedBy(ast::NodeId),
 }
 
+enum FieldName {
+    UnnamedField(uint), // index
+    // FIXME #6993: change type (and name) from Ident to Name
+    NamedField(ast::Ident),
+}
+
 impl<'a> PrivacyVisitor<'a> {
     // used when debugging
     fn nodestr(&self, id: ast::NodeId) -> ~str {
@@ -560,18 +567,23 @@ 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) {
-        for field in ty::lookup_struct_fields(self.tcx, id).iter() {
-            if field.name != ident.name { continue; }
-            if 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",
-                                               token::get_ident(ident)))
+    fn check_field(&mut self, span: Span, id: ast::DefId,
+                   name: FieldName) {
+        let fields = ty::lookup_struct_fields(self.tcx, id);
+        let field = match name {
+            NamedField(ident) => {
+                fields.iter().find(|f| f.name == ident.name).unwrap()
             }
-            break;
+            UnnamedField(idx) => fields.get(idx)
+        };
+        if field.vis == ast::Public { return }
+        if !is_local(field.id) || !self.private_accessible(field.id.node) {
+            let msg = match name {
+                NamedField(name) => format!("field `{}` is private",
+                                            token::get_ident(name)),
+                UnnamedField(idx) => format!("field \\#{} is private", idx + 1),
+            };
+            self.tcx.sess.span_err(span, msg);
         }
     }
 
@@ -634,10 +646,11 @@ impl<'a> PrivacyVisitor<'a> {
                             _ => {},
                         }
                     }
-                    // If an import is not used in either namespace, we still want to check
-                    // that it could be legal. Therefore we check in both namespaces and only
-                    // report an error if both would be illegal. We only report one error,
-                    // even if it is illegal to import from both namespaces.
+                    // If an import is not used in either namespace, we still
+                    // want to check that it could be legal. Therefore we check
+                    // in both namespaces and only report an error if both would
+                    // be illegal. We only report one error, even if it is
+                    // illegal to import from both namespaces.
                     match (value_priv, check_value, type_priv, check_type) {
                         (Some(p), resolve::Unused, None, _) |
                         (None, _, Some(p), resolve::Unused) => {
@@ -701,7 +714,8 @@ impl<'a> PrivacyVisitor<'a> {
             // is whether the trait itself is accessible or not.
             MethodParam(MethodParam { trait_id: trait_id, .. }) |
             MethodObject(MethodObject { trait_id: trait_id, .. }) => {
-                self.report_error(self.ensure_public(span, trait_id, None, "source trait"));
+                self.report_error(self.ensure_public(span, trait_id, None,
+                                                     "source trait"));
             }
         }
     }
@@ -726,7 +740,7 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                 match ty::get(ty::expr_ty_adjusted(self.tcx, base,
                                                    &*self.method_map.borrow())).sty {
                     ty::ty_struct(id, _) => {
-                        self.check_field(expr.span, id, ident);
+                        self.check_field(expr.span, id, NamedField(ident));
                     }
                     _ => {}
                 }
@@ -749,7 +763,8 @@ 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,
+                                             NamedField(field.ident.node));
                         }
                     }
                     ty::ty_enum(_, _) => {
@@ -757,7 +772,7 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                             ast::DefVariant(_, variant_id, _) => {
                                 for field in fields.iter() {
                                     self.check_field(expr.span, variant_id,
-                                                     field.ident.node);
+                                                     NamedField(field.ident.node));
                                 }
                             }
                             _ => self.tcx.sess.span_bug(expr.span,
@@ -772,6 +787,46 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                                                             struct type?!"),
                 }
             }
+            ast::ExprPath(..) => {
+                let guard = |did: ast::DefId| {
+                    let fields = ty::lookup_struct_fields(self.tcx, did);
+                    let any_priv = fields.iter().any(|f| {
+                        f.vis != ast::Public && (
+                            !is_local(f.id) ||
+                            !self.private_accessible(f.id.node))
+                    });
+                    if any_priv {
+                        self.tcx.sess.span_err(expr.span,
+                            "cannot invoke tuple struct constructor \
+                             with private fields");
+                    }
+                };
+                match self.tcx.def_map.borrow().find(&expr.id) {
+                    Some(&ast::DefStruct(did)) => {
+                        guard(if is_local(did) {
+                            local_def(self.tcx.map.get_parent(did.node))
+                        } else {
+                            // "tuple structs" with zero fields (such as
+                            // `pub struct Foo;`) don't have a ctor_id, hence
+                            // the unwrap_or to the same struct id.
+                            let maybe_did =
+                                csearch::get_tuple_struct_definition_if_ctor(
+                                    &self.tcx.sess.cstore, did);
+                            maybe_did.unwrap_or(did)
+                        })
+                    }
+                    // Tuple struct constructors across crates are identified as
+                    // DefFn types, so we explicitly handle that case here.
+                    Some(&ast::DefFn(did, _)) if !is_local(did) => {
+                        match csearch::get_tuple_struct_definition_if_ctor(
+                                    &self.tcx.sess.cstore, did) {
+                            Some(did) => guard(did),
+                            None => {}
+                        }
+                    }
+                    _ => {}
+                }
+            }
             _ => {}
         }
 
@@ -821,7 +876,8 @@ 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,
+                                             NamedField(field.ident));
                         }
                     }
                     ty::ty_enum(_, _) => {
@@ -829,7 +885,7 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                             Some(&ast::DefVariant(_, variant_id, _)) => {
                                 for field in fields.iter() {
                                     self.check_field(pattern.span, variant_id,
-                                                     field.ident);
+                                                     NamedField(field.ident));
                                 }
                             }
                             _ => self.tcx.sess.span_bug(pattern.span,
@@ -844,6 +900,27 @@ impl<'a> Visitor<()> for PrivacyVisitor<'a> {
                                                  struct type?!"),
                 }
             }
+
+            // Patterns which bind no fields are allowable (the path is check
+            // elsewhere).
+            ast::PatEnum(_, Some(ref fields)) => {
+                match ty::get(ty::pat_ty(self.tcx, pattern)).sty {
+                    ty::ty_struct(id, _) => {
+                        for (i, field) in fields.iter().enumerate() {
+                            match field.node {
+                                ast::PatWild(..) => continue,
+                                _ => {}
+                            }
+                            self.check_field(field.span, id, UnnamedField(i));
+                        }
+                    }
+                    ty::ty_enum(..) => {
+                        // enum fields have no privacy at this time
+                    }
+                    _ => {}
+                }
+
+            }
             _ => {}
         }
 
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
index 3a9cdfb56e3..0f082c7e2f1 100644
--- a/src/libsyntax/ast.rs
+++ b/src/libsyntax/ast.rs
@@ -493,10 +493,10 @@ pub enum Expr_ {
     ExprVstore(@Expr, ExprVstore),
     // First expr is the place; second expr is the value.
     ExprBox(@Expr, @Expr),
-    ExprVec(Vec<@Expr> , Mutability),
-    ExprCall(@Expr, Vec<@Expr> ),
-    ExprMethodCall(Ident, Vec<P<Ty>> , Vec<@Expr> ),
-    ExprTup(Vec<@Expr> ),
+    ExprVec(Vec<@Expr>, Mutability),
+    ExprCall(@Expr, Vec<@Expr>),
+    ExprMethodCall(Ident, Vec<P<Ty>>, Vec<@Expr>),
+    ExprTup(Vec<@Expr>),
     ExprBinary(BinOp, @Expr, @Expr),
     ExprUnary(UnOp, @Expr),
     ExprLit(@Lit),
@@ -508,14 +508,14 @@ pub enum Expr_ {
     // Conditionless loop (can be exited with break, cont, or ret)
     // FIXME #6993: change to Option<Name>
     ExprLoop(P<Block>, Option<Ident>),
-    ExprMatch(@Expr, Vec<Arm> ),
+    ExprMatch(@Expr, Vec<Arm>),
     ExprFnBlock(P<FnDecl>, P<Block>),
     ExprProc(P<FnDecl>, P<Block>),
     ExprBlock(P<Block>),
 
     ExprAssign(@Expr, @Expr),
     ExprAssignOp(BinOp, @Expr, @Expr),
-    ExprField(@Expr, Ident, Vec<P<Ty>> ),
+    ExprField(@Expr, Ident, Vec<P<Ty>>),
     ExprIndex(@Expr, @Expr),
 
     /// Expression that looks like a "name". For example,
diff --git a/src/test/auxiliary/privacy-tuple-struct.rs b/src/test/auxiliary/privacy-tuple-struct.rs
new file mode 100644
index 00000000000..2fb9d9923cb
--- /dev/null
+++ b/src/test/auxiliary/privacy-tuple-struct.rs
@@ -0,0 +1,14 @@
+// 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.
+
+pub struct A(());
+pub struct B(int);
+pub struct C(pub int, int);
+pub struct D(pub int);
diff --git a/src/test/compile-fail/privacy5.rs b/src/test/compile-fail/privacy5.rs
new file mode 100644
index 00000000000..c057236265e
--- /dev/null
+++ b/src/test/compile-fail/privacy5.rs
@@ -0,0 +1,137 @@
+// 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:privacy-tuple-struct.rs
+// ignore-fast
+
+extern crate other = "privacy-tuple-struct";
+
+mod a {
+    pub struct A(());
+    pub struct B(int);
+    pub struct C(pub int, int);
+    pub struct D(pub int);
+
+    fn test() {
+        let a = A(());
+        let b = B(2);
+        let c = C(2, 3);
+        let d = D(4);
+
+        let A(()) = a;
+        let A(_) = a;
+        match a { A(()) => {} }
+        match a { A(_) => {} }
+
+        let B(_) = b;
+        let B(_b) = b;
+        match b { B(_) => {} }
+        match b { B(_b) => {} }
+        match b { B(1) => {} B(_) => {} }
+
+        let C(_, _) = c;
+        let C(_a, _) = c;
+        let C(_, _b) = c;
+        let C(_a, _b) = c;
+        match c { C(_, _) => {} }
+        match c { C(_a, _) => {} }
+        match c { C(_, _b) => {} }
+        match c { C(_a, _b) => {} }
+
+        let D(_) = d;
+        let D(_d) = d;
+        match d { D(_) => {} }
+        match d { D(_d) => {} }
+        match d { D(1) => {} D(_) => {} }
+
+        let a2 = A;
+        let b2 = B;
+        let c2 = C;
+        let d2 = D;
+    }
+}
+
+fn this_crate() {
+    let a = a::A(()); //~ ERROR: cannot invoke tuple struct constructor
+    let b = a::B(2); //~ ERROR: cannot invoke tuple struct constructor
+    let c = a::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor
+    let d = a::D(4);
+
+    let a::A(()) = a; //~ ERROR: field #1 is private
+    let a::A(_) = a;
+    match a { a::A(()) => {} } //~ ERROR: field #1 is private
+    match a { a::A(_) => {} }
+
+    let a::B(_) = b;
+    let a::B(_b) = b; //~ ERROR: field #1 is private
+    match b { a::B(_) => {} }
+    match b { a::B(_b) => {} } //~ ERROR: field #1 is private
+    match b { a::B(1) => {} a::B(_) => {} } //~ ERROR: field #1 is private
+
+    let a::C(_, _) = c;
+    let a::C(_a, _) = c;
+    let a::C(_, _b) = c; //~ ERROR: field #2 is private
+    let a::C(_a, _b) = c; //~ ERROR: field #2 is private
+    match c { a::C(_, _) => {} }
+    match c { a::C(_a, _) => {} }
+    match c { a::C(_, _b) => {} } //~ ERROR: field #2 is private
+    match c { a::C(_a, _b) => {} } //~ ERROR: field #2 is private
+
+    let a::D(_) = d;
+    let a::D(_d) = d;
+    match d { a::D(_) => {} }
+    match d { a::D(_d) => {} }
+    match d { a::D(1) => {} a::D(_) => {} }
+
+    let a2 = a::A; //~ ERROR: cannot invoke tuple struct constructor
+    let b2 = a::B; //~ ERROR: cannot invoke tuple struct constructor
+    let c2 = a::C; //~ ERROR: cannot invoke tuple struct constructor
+    let d2 = a::D;
+}
+
+fn xcrate() {
+    let a = other::A(()); //~ ERROR: cannot invoke tuple struct constructor
+    let b = other::B(2); //~ ERROR: cannot invoke tuple struct constructor
+    let c = other::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor
+    let d = other::D(4);
+
+    let other::A(()) = a; //~ ERROR: field #1 is private
+    let other::A(_) = a;
+    match a { other::A(()) => {} } //~ ERROR: field #1 is private
+    match a { other::A(_) => {} }
+
+    let other::B(_) = b;
+    let other::B(_b) = b; //~ ERROR: field #1 is private
+    match b { other::B(_) => {} }
+    match b { other::B(_b) => {} } //~ ERROR: field #1 is private
+    match b { other::B(1) => {} other::B(_) => {} } //~ ERROR: field #1 is private
+
+    let other::C(_, _) = c;
+    let other::C(_a, _) = c;
+    let other::C(_, _b) = c; //~ ERROR: field #2 is private
+    let other::C(_a, _b) = c; //~ ERROR: field #2 is private
+    match c { other::C(_, _) => {} }
+    match c { other::C(_a, _) => {} }
+    match c { other::C(_, _b) => {} } //~ ERROR: field #2 is private
+    match c { other::C(_a, _b) => {} } //~ ERROR: field #2 is private
+
+    let other::D(_) = d;
+    let other::D(_d) = d;
+    match d { other::D(_) => {} }
+    match d { other::D(_d) => {} }
+    match d { other::D(1) => {} other::D(_) => {} }
+
+    let a2 = other::A; //~ ERROR: cannot invoke tuple struct constructor
+    let b2 = other::B; //~ ERROR: cannot invoke tuple struct constructor
+    let c2 = other::C; //~ ERROR: cannot invoke tuple struct constructor
+    let d2 = other::D;
+}
+
+fn main() {}