From 2859c1ac6d760876282471ae57fee2e5731f85d5 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 27 Feb 2013 13:45:37 -0800 Subject: [PATCH] librustc: Enforce cross-crate method privacy --- doc/tutorial.md | 3 +- src/librustc/metadata/common.rs | 1 + src/librustc/metadata/csearch.rs | 8 +++ src/librustc/metadata/decoder.rs | 19 +++++- src/librustc/metadata/encoder.rs | 63 ++++++++++++++++--- src/librustc/middle/privacy.rs | 35 ++++++++--- src/libstd/oldmap.rs | 2 + src/test/auxiliary/cci_class_5.rs | 4 +- .../private-method-cross-crate.rs | 4 +- 9 files changed, 112 insertions(+), 27 deletions(-) diff --git a/doc/tutorial.md b/doc/tutorial.md index f9408761877..d31fbbb0c07 100644 --- a/doc/tutorial.md +++ b/doc/tutorial.md @@ -2304,11 +2304,10 @@ mod farm { farmer: Human } - // Note - visibility modifiers on impls currently have no effect impl Farm { priv fn feed_chickens(&self) { ... } priv fn feed_cows(&self) { ... } - fn add_chicken(&self, c: Chicken) { ... } + pub fn add_chicken(&self, c: Chicken) { ... } } pub fn feed_animals(farm: &Farm) { diff --git a/src/librustc/metadata/common.rs b/src/librustc/metadata/common.rs index 877098ed49f..90d8dcdc235 100644 --- a/src/librustc/metadata/common.rs +++ b/src/librustc/metadata/common.rs @@ -155,6 +155,7 @@ pub const tag_lang_items_item_node_id: uint = 0x75; pub const tag_item_unnamed_field: uint = 0x76; pub const tag_items_data_item_struct_ctor: uint = 0x77; +pub const tag_items_data_item_visibility: uint = 0x78; pub struct LinkMeta { name: @str, diff --git a/src/librustc/metadata/csearch.rs b/src/librustc/metadata/csearch.rs index eda7362b9c1..ae4a223c1ae 100644 --- a/src/librustc/metadata/csearch.rs +++ b/src/librustc/metadata/csearch.rs @@ -234,6 +234,14 @@ pub fn struct_dtor(cstore: @mut cstore::CStore, def: ast::def_id) let cdata = cstore::get_crate_data(cstore, def.crate); decoder::struct_dtor(cdata, def.node) } + +pub fn get_method_visibility(cstore: @mut cstore::CStore, + def_id: ast::def_id) + -> ast::visibility { + let cdata = cstore::get_crate_data(cstore, def_id.crate); + decoder::get_method_visibility(cdata, def_id.node) +} + // Local Variables: // mode: rust // fill-column: 78; diff --git a/src/librustc/metadata/decoder.rs b/src/librustc/metadata/decoder.rs index ca55c8a4072..5963d878060 100644 --- a/src/librustc/metadata/decoder.rs +++ b/src/librustc/metadata/decoder.rs @@ -151,6 +151,16 @@ fn item_family(item: ebml::Doc) -> Family { } } +fn item_visibility(item: ebml::Doc) -> ast::visibility { + let visibility = reader::get_doc(item, tag_items_data_item_visibility); + match reader::doc_as_u8(visibility) as char { + 'y' => ast::public, + 'n' => ast::private, + 'i' => ast::inherited, + _ => fail!(~"unknown visibility character"), + } +} + fn item_method_sort(item: ebml::Doc) -> char { for reader::tagged_docs(item, tag_item_trait_method_sort) |doc| { return str::from_bytes(reader::doc_data(doc))[0] as char; @@ -860,7 +870,7 @@ pub fn get_item_attrs(cdata: cmd, } } -pure fn family_to_visibility(family: Family) -> ast::visibility { +pure fn struct_field_family_to_visibility(family: Family) -> ast::visibility { match family { PublicField => ast::public, PrivateField => ast::private, @@ -883,7 +893,7 @@ pub fn get_struct_fields(intr: @ident_interner, cdata: cmd, id: ast::node_id) result.push(ty::field_ty { ident: name, id: did, vis: - family_to_visibility(f), + struct_field_family_to_visibility(f), mutability: mt, }); } @@ -900,6 +910,11 @@ pub fn get_struct_fields(intr: @ident_interner, cdata: cmd, id: ast::node_id) result } +pub fn get_method_visibility(cdata: cmd, id: ast::node_id) + -> ast::visibility { + item_visibility(lookup_item(id, cdata.data)) +} + fn family_has_type_params(fam: Family) -> bool { match fam { Const | ForeignType | Mod | ForeignMod | PublicField | PrivateField diff --git a/src/librustc/metadata/encoder.rs b/src/librustc/metadata/encoder.rs index 95973aaaf90..a950cd04d67 100644 --- a/src/librustc/metadata/encoder.rs +++ b/src/librustc/metadata/encoder.rs @@ -383,7 +383,8 @@ fn encode_info_for_mod(ecx: @EncodeContext, ebml_w: writer::Encoder, ebml_w.end_tag(); } -fn encode_visibility(ebml_w: writer::Encoder, visibility: visibility) { +fn encode_struct_field_family(ebml_w: writer::Encoder, + visibility: visibility) { encode_family(ebml_w, match visibility { public => 'g', private => 'j', @@ -391,6 +392,17 @@ fn encode_visibility(ebml_w: writer::Encoder, visibility: visibility) { }); } +fn encode_visibility(ebml_w: writer::Encoder, visibility: visibility) { + ebml_w.start_tag(tag_items_data_item_visibility); + let ch = match visibility { + public => 'y', + private => 'n', + inherited => 'i', + }; + ebml_w.wr_str(str::from_char(ch)); + ebml_w.end_tag(); +} + fn encode_self_type(ebml_w: writer::Encoder, self_type: ast::self_ty_) { ebml_w.start_tag(tag_item_trait_method_self_ty); @@ -456,7 +468,7 @@ fn encode_info_for_struct(ecx: @EncodeContext, ebml_w: writer::Encoder, ebml_w.start_tag(tag_items_data_item); debug!("encode_info_for_struct: doing %s %d", *tcx.sess.str_of(nm), id); - encode_visibility(ebml_w, vis); + encode_struct_field_family(ebml_w, vis); encode_name(ecx, ebml_w, nm); encode_path(ecx, ebml_w, path, ast_map::path_name(nm)); encode_type(ecx, ebml_w, node_id_to_type(tcx, id)); @@ -525,6 +537,7 @@ fn encode_info_for_method(ecx: @EncodeContext, should_inline: bool, parent_id: node_id, m: @method, + parent_visibility: ast::visibility, owner_generics: &ast::Generics, method_generics: &ast::Generics) { debug!("encode_info_for_method: %d %s %u %u", m.id, @@ -533,6 +546,7 @@ fn encode_info_for_method(ecx: @EncodeContext, method_generics.ty_params.len()); ebml_w.start_tag(tag_items_data_item); encode_def_id(ebml_w, local_def(m.id)); + match m.self_ty.node { ast::sty_static => { encode_family(ebml_w, purity_static_method_family(m.purity)); @@ -550,6 +564,14 @@ fn encode_info_for_method(ecx: @EncodeContext, encode_name(ecx, ebml_w, m.ident); encode_path(ecx, ebml_w, impl_path, ast_map::path_name(m.ident)); encode_self_type(ebml_w, m.self_ty.node); + + // Combine parent visibility and this visibility. + let visibility = match m.vis { + ast::inherited => parent_visibility, + vis => vis, + }; + encode_visibility(ebml_w, visibility); + if len > 0u || should_inline { (ecx.encode_inlined_item)( ecx, ebml_w, impl_path, @@ -568,6 +590,7 @@ fn purity_fn_family(p: purity) -> char { extern_fn => 'e' } } + fn purity_static_method_family(p: purity) -> char { match p { unsafe_fn => 'U', @@ -757,7 +780,7 @@ fn encode_info_for_item(ecx: @EncodeContext, ebml_w: writer::Encoder, match f.node.kind { named_field(ident, _, vis) => { ebml_w.start_tag(tag_item_field); - encode_visibility(ebml_w, vis); + encode_struct_field_family(ebml_w, vis); encode_name(ecx, ebml_w, ident); encode_def_id(ebml_w, local_def(f.node.id)); ebml_w.end_tag(); @@ -808,12 +831,28 @@ fn encode_info_for_item(ecx: @EncodeContext, ebml_w: writer::Encoder, let mut impl_path = vec::append(~[], path); impl_path += ~[ast_map::path_name(item.ident)]; + // If there is a trait reference, treat the methods as always public. + // This is to work around some incorrect behavior in privacy checking: + // when the method belongs to a trait, it should acquire the privacy + // from the trait, not the impl. Forcing the visibility to be public + // makes things sorta work. + let parent_visibility = if opt_trait.is_some() { + ast::public + } else { + item.vis + }; + for methods.each |m| { index.push(entry {val: m.id, pos: ebml_w.writer.tell()}); - encode_info_for_method(ecx, ebml_w, impl_path, + encode_info_for_method(ecx, + ebml_w, + impl_path, should_inline(m.attrs), - item.id, *m, - generics, &m.generics); + item.id, + *m, + parent_visibility, + generics, + &m.generics); } } item_trait(ref generics, ref traits, ref ms) => { @@ -902,9 +941,15 @@ fn encode_info_for_item(ecx: @EncodeContext, ebml_w: writer::Encoder, // of provided methods. I am not sure why this is. -ndm let owner_generics = ast_util::empty_generics(); - encode_info_for_method(ecx, ebml_w, /*bad*/copy path, - true, item.id, *m, - &owner_generics, &m.generics); + encode_info_for_method(ecx, + ebml_w, + /*bad*/copy path, + true, + item.id, + *m, + item.vis, + &owner_generics, + &m.generics); } } item_mac(*) => fail!(~"item macros unimplemented") diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs index 712037720d5..e60069e05da 100644 --- a/src/librustc/middle/privacy.rs +++ b/src/librustc/middle/privacy.rs @@ -14,10 +14,11 @@ use core::prelude::*; +use metadata::csearch; use middle::ty::{ty_struct, ty_enum}; use middle::ty; -use middle::typeck::{method_map, method_origin, method_param, method_self, - method_super}; +use middle::typeck::{method_map, method_origin, method_param, method_self}; +use middle::typeck::{method_super}; use middle::typeck::{method_static, method_trait}; use core::dvec::DVec; @@ -100,8 +101,10 @@ pub fn check_crate(tcx: ty::ctxt, }; // Checks that a private method is in scope. - let check_method: @fn(span: span, origin: &method_origin) = - |span, origin| { + let check_method: @fn(span: span, + origin: &method_origin, + ident: ast::ident) = + |span, origin, ident| { match *origin { method_static(method_id) => { if method_id.crate == local_crate { @@ -110,6 +113,8 @@ pub fn check_crate(tcx: ty::ctxt, let mut is_private = false; if method.vis == private { is_private = true; + } else if method.vis == public { + is_private = false; } else { // Look up the enclosing impl. if impl_id.crate != local_crate { @@ -121,7 +126,7 @@ pub fn check_crate(tcx: ty::ctxt, match tcx.items.find(&impl_id.node) { Some(node_item(item, _)) => { match item.node { - item_impl(_, None, _, _) + item_impl(_, None, _, _) if item.vis != public => { is_private = true; } @@ -165,7 +170,15 @@ pub fn check_crate(tcx: ty::ctxt, } } } else { - // FIXME #4732: External crates. + let visibility = + csearch::get_method_visibility(tcx.sess.cstore, + method_id); + if visibility != public { + tcx.sess.span_err(span, + fmt!("method `%s` is private", + *tcx.sess.parse_sess.interner + .get(ident))); + } } } method_param(method_param { @@ -264,14 +277,16 @@ pub fn check_crate(tcx: ty::ctxt, Some(ref entry) => { debug!("(privacy checking) checking \ impl method"); - check_method(expr.span, &(*entry).origin); + check_method(expr.span, + &entry.origin, + ident); } } } _ => {} } } - expr_method_call(base, _, _, _, _) => { + expr_method_call(base, ident, _, _, _) => { // Ditto match ty::get(ty::type_autoderef(tcx, ty::expr_ty(tcx, base))).sty { @@ -287,7 +302,9 @@ pub fn check_crate(tcx: ty::ctxt, Some(ref entry) => { debug!("(privacy checking) checking \ impl method"); - check_method(expr.span, &(*entry).origin); + check_method(expr.span, + &entry.origin, + ident); } } } diff --git a/src/libstd/oldmap.rs b/src/libstd/oldmap.rs index c1227f6b077..1d21f749b32 100644 --- a/src/libstd/oldmap.rs +++ b/src/libstd/oldmap.rs @@ -134,7 +134,9 @@ pub mod chained { } self.chains = new_chains; } + } + pub impl T { pure fn each_entry(blk: fn(@Entry) -> bool) { // n.b. we can't use vec::iter() here because self.chains // is stored in a mutable location. diff --git a/src/test/auxiliary/cci_class_5.rs b/src/test/auxiliary/cci_class_5.rs index e2564a55279..c04cdbcab1a 100644 --- a/src/test/auxiliary/cci_class_5.rs +++ b/src/test/auxiliary/cci_class_5.rs @@ -10,12 +10,12 @@ pub mod kitties { pub struct cat { - priv mut meows : uint, + priv meows : uint, how_hungry : int, } pub impl cat { - priv fn nap() { for uint::range(1, 10000u) |_i|{}} + priv fn nap(&self) { for uint::range(1, 10000u) |_i|{}} } pub fn cat(in_x : uint, in_y : int) -> cat { diff --git a/src/test/compile-fail/private-method-cross-crate.rs b/src/test/compile-fail/private-method-cross-crate.rs index 7897ccb23e6..7414dc57216 100644 --- a/src/test/compile-fail/private-method-cross-crate.rs +++ b/src/test/compile-fail/private-method-cross-crate.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// error-pattern:attempted access of field `nap` on type -// xfail-test Cross-crate impl method privacy doesn't work // xfail-fast // aux-build:cci_class_5.rs extern mod cci_class_5; @@ -17,5 +15,5 @@ use cci_class_5::kitties::*; fn main() { let nyan : cat = cat(52, 99); - nyan.nap(); + nyan.nap(); //~ ERROR method `nap` is private }