From 683197975c119e4c03588fa729dee4f87902f534 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 31 Mar 2014 18:13:44 -0700 Subject: [PATCH] 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 --- src/librustc/metadata/csearch.rs | 8 ++ src/librustc/metadata/decoder.rs | 11 +- src/librustc/middle/privacy.rs | 119 ++++++++++++++---- src/libsyntax/ast.rs | 12 +- src/test/auxiliary/privacy-tuple-struct.rs | 14 +++ src/test/compile-fail/privacy5.rs | 137 +++++++++++++++++++++ 6 files changed, 270 insertions(+), 31 deletions(-) create mode 100644 src/test/auxiliary/privacy-tuple-struct.rs create mode 100644 src/test/compile-fail/privacy5.rs 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 +{ + 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, /// 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 { + node_id: ast::NodeId) + -> Option +{ 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> , Vec<@Expr> ), - ExprTup(Vec<@Expr> ), + ExprVec(Vec<@Expr>, Mutability), + ExprCall(@Expr, Vec<@Expr>), + ExprMethodCall(Ident, Vec>, 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 ExprLoop(P, Option), - ExprMatch(@Expr, Vec ), + ExprMatch(@Expr, Vec), ExprFnBlock(P, P), ExprProc(P, P), ExprBlock(P), ExprAssign(@Expr, @Expr), ExprAssignOp(BinOp, @Expr, @Expr), - ExprField(@Expr, Ident, Vec> ), + ExprField(@Expr, Ident, Vec>), 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 or the MIT license +// , 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 or the MIT license +// , 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() {}