librustc: Enforce cross-crate method privacy

Author: pcwalton

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) {

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,

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;

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

src/librustc/metadata/encoder.rs

@@ -383,14 +383,26 @@ 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',
         inherited => 'N'
     });
 }
 
+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")

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);
                                 }
                             }
                         }

src/libstd/oldmap.rs

@@ -134,7 +134,9 @@ pub mod chained {
             }
             self.chains = new_chains;
         }
+    }
 
+    pub impl<K:Eq + IterBytes + Hash,V> T<K, V> {
         pure fn each_entry(blk: fn(@Entry<K,V>) -> bool) {
             // n.b. we can't use vec::iter() here because self.chains
             // is stored in a mutable location.

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 {

src/test/compile-fail/private-method-cross-crate.rs

@@ -8,14 +8,12 @@
 // 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;
 use cci_class_5::kitties::*;
 
 fn main() {
   let nyan : cat = cat(52, 99);
-  nyan.nap();
+  nyan.nap();   //~ ERROR method `nap` is private
 }