From 082cc9609076faecaa3aad805bdb5e04e1aa855e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 14 Oct 2013 15:12:40 -0700 Subject: [PATCH] Refine privacy error messages to be more accurate This stops labeling everything as "is private" when in fact the destination may be public. Instead, the clause "is inaccessible" is used and the private part of the flag is called out with a "is private" message. Closes #9793 --- src/librustc/middle/privacy.rs | 121 ++++++++++++++------ src/test/compile-fail/export-tag-variant.rs | 2 +- src/test/compile-fail/privacy1.rs | 17 ++- 3 files changed, 95 insertions(+), 45 deletions(-) diff --git a/src/librustc/middle/privacy.rs b/src/librustc/middle/privacy.rs index 6f52ba5db4e1..b8c432c3f26a 100644 --- a/src/librustc/middle/privacy.rs +++ b/src/librustc/middle/privacy.rs @@ -21,7 +21,7 @@ use middle::typeck::{method_static, method_object}; use syntax::ast; use syntax::ast_map; -use syntax::ast_util::is_local; +use syntax::ast_util::{is_local, def_id_of_def}; use syntax::attr; use syntax::codemap::Span; use syntax::parse::token; @@ -250,6 +250,12 @@ struct PrivacyVisitor<'self> { last_private_map: resolve::LastPrivateMap, } +enum PrivacyResult { + Allowable, + ExternallyDenied, + DisallowedBy(ast::NodeId), +} + impl<'self> PrivacyVisitor<'self> { // used when debugging fn nodestr(&self, id: ast::NodeId) -> ~str { @@ -258,11 +264,11 @@ impl<'self> PrivacyVisitor<'self> { // Determines whether the given definition is public from the point of view // of the current item. - fn def_public(&self, did: ast::DefId) -> bool { + fn def_privacy(&self, did: ast::DefId) -> PrivacyResult { if !is_local(did) { if self.external_exports.contains(&did) { debug2!("privacy - {:?} was externally exported", did); - return true; + return Allowable; } debug2!("privacy - is {:?} a public method", did); return match self.tcx.methods.find(&did) { @@ -271,18 +277,22 @@ impl<'self> PrivacyVisitor<'self> { match meth.container { ty::TraitContainer(id) => { debug2!("privacy - recursing on trait {:?}", id); - self.def_public(id) + self.def_privacy(id) } ty::ImplContainer(id) => { match ty::impl_trait_ref(self.tcx, id) { Some(t) => { debug2!("privacy - impl of trait {:?}", id); - self.def_public(t.def_id) + self.def_privacy(t.def_id) } None => { debug2!("privacy - found a method {:?}", meth.vis); - meth.vis == ast::public + if meth.vis == ast::public { + Allowable + } else { + ExternallyDenied + } } } } @@ -290,19 +300,19 @@ impl<'self> PrivacyVisitor<'self> { } None => { debug2!("privacy - nope, not even a method"); - false + ExternallyDenied } }; } else if self.exported_items.contains(&did.node) { debug2!("privacy - exported item {}", self.nodestr(did.node)); - return true; + return Allowable; } debug2!("privacy - local {:?} not public all the way down", did); // return quickly for things in the same module if self.parents.find(&did.node) == self.parents.find(&self.curitem) { debug2!("privacy - same parent, we're done here"); - return true; + return Allowable; } // We now know that there is at least one private member between the @@ -330,7 +340,11 @@ impl<'self> PrivacyVisitor<'self> { assert!(closest_private_id != ast::DUMMY_NODE_ID); } debug2!("privacy - closest priv {}", self.nodestr(closest_private_id)); - return self.private_accessible(closest_private_id); + if self.private_accessible(closest_private_id) { + Allowable + } else { + DisallowedBy(closest_private_id) + } } /// For a local private node in the AST, this function will determine @@ -365,12 +379,51 @@ impl<'self> PrivacyVisitor<'self> { } } + /// Guarantee that a particular definition is public, possibly emitting an + /// error message if it's not. + fn ensure_public(&self, span: Span, to_check: ast::DefId, + source_did: Option, msg: &str) -> bool { + match self.def_privacy(to_check) { + ExternallyDenied => { + self.tcx.sess.span_err(span, format!("{} is private", msg)) + } + DisallowedBy(id) => { + if id == source_did.unwrap_or(to_check).node { + self.tcx.sess.span_err(span, format!("{} is private", msg)); + return false; + } else { + self.tcx.sess.span_err(span, format!("{} is inaccessible", + msg)); + } + match self.tcx.items.find(&id) { + Some(&ast_map::node_item(item, _)) => { + let desc = match item.node { + ast::item_mod(*) => "module", + ast::item_trait(*) => "trait", + _ => return false, + }; + let msg = format!("{} `{}` is private", desc, + token::ident_to_str(&item.ident)); + self.tcx.sess.span_note(span, msg); + } + Some(*) | None => {} + } + } + Allowable => return true + } + return false; + } + // Checks that a dereference of a univariant enum can occur. fn check_variant(&self, span: Span, enum_id: ast::DefId) { let variant_info = ty::enum_variants(self.tcx, enum_id)[0]; - if !self.def_public(variant_info.id) { - self.tcx.sess.span_err(span, "can only dereference enums \ - with a single, public variant"); + + match self.def_privacy(variant_info.id) { + Allowable => {} + ExternallyDenied | DisallowedBy(*) => { + self.tcx.sess.span_err(span, "can only dereference enums \ + with a single, public variant"); + } } } @@ -399,29 +452,24 @@ impl<'self> PrivacyVisitor<'self> { let method_id = ty::method(self.tcx, method_id).provided_source .unwrap_or(method_id); - if !self.def_public(method_id) { - debug2!("private: {:?}", method_id); - self.tcx.sess.span_err(span, format!("method `{}` is private", - token::ident_to_str(name))); - } + self.ensure_public(span, method_id, None, + format!("method `{}`", token::ident_to_str(name))); } // Checks that a path is in scope. fn check_path(&mut self, span: Span, path_id: ast::NodeId, path: &ast::Path) { debug2!("privacy - path {}", self.nodestr(path_id)); + let def = self.tcx.def_map.get_copy(&path_id); let ck = |tyname: &str| { - let last_private = *self.last_private_map.get(&path_id); - debug2!("privacy - {:?}", last_private); - let public = match last_private { - resolve::AllPublic => true, - resolve::DependsOn(def) => self.def_public(def), - }; - if !public { - debug2!("denying {:?}", path); - let name = token::ident_to_str(&path.segments.last() - .identifier); - self.tcx.sess.span_err(span, - format!("{} `{}` is private", tyname, name)); + let origdid = def_id_of_def(def); + match *self.last_private_map.get(&path_id) { + resolve::AllPublic => {}, + resolve::DependsOn(def) => { + let name = token::ident_to_str(&path.segments.last() + .identifier); + self.ensure_public(span, def, Some(origdid), + format!("{} `{}`", tyname, name)); + } } }; match self.tcx.def_map.get_copy(&path_id) { @@ -456,9 +504,8 @@ impl<'self> PrivacyVisitor<'self> { method_num: method_num, _ }) => { - if !self.def_public(trait_id) { - self.tcx.sess.span_err(span, "source trait is private"); - return; + if !self.ensure_public(span, trait_id, None, "source trait") { + return } match self.tcx.items.find(&trait_id.node) { Some(&ast_map::node_item(item, _)) => { @@ -470,12 +517,10 @@ impl<'self> PrivacyVisitor<'self> { node: method.id, crate: trait_id.crate, }; - if self.def_public(def) { return } - let msg = format!("method `{}` is \ - private", + self.ensure_public(span, def, None, + format!("method `{}`", token::ident_to_str( - &method.ident)); - self.tcx.sess.span_err(span, msg); + &method.ident))); } ast::required(_) => { // Required methods can't be private. diff --git a/src/test/compile-fail/export-tag-variant.rs b/src/test/compile-fail/export-tag-variant.rs index d92cd2048508..859cbd8e50bd 100644 --- a/src/test/compile-fail/export-tag-variant.rs +++ b/src/test/compile-fail/export-tag-variant.rs @@ -14,4 +14,4 @@ mod foo { enum y { y1, } } -fn main() { let z = foo::y1; } //~ ERROR: is private +fn main() { let z = foo::y1; } //~ ERROR: is inaccessible diff --git a/src/test/compile-fail/privacy1.rs b/src/test/compile-fail/privacy1.rs index 0d4dbc86dce4..a17e689f444f 100644 --- a/src/test/compile-fail/privacy1.rs +++ b/src/test/compile-fail/privacy1.rs @@ -100,11 +100,14 @@ mod foo { ::bar::A::bar(); //~ ERROR: method `bar` is private ::bar::A.foo2(); ::bar::A.bar2(); //~ ERROR: method `bar2` is private - ::bar::baz::A::foo(); //~ ERROR: method `foo` is private + ::bar::baz::A::foo(); //~ ERROR: method `foo` is inaccessible + //~^ NOTE: module `baz` is private ::bar::baz::A::bar(); //~ ERROR: method `bar` is private - ::bar::baz::A.foo2(); //~ ERROR: struct `A` is private - ::bar::baz::A.bar2(); //~ ERROR: struct `A` is private + ::bar::baz::A.foo2(); //~ ERROR: struct `A` is inaccessible + //~^ NOTE: module `baz` is private + ::bar::baz::A.bar2(); //~ ERROR: struct `A` is inaccessible //~^ ERROR: method `bar2` is private + //~^^ NOTE: module `baz` is private ::lol(); ::bar::Priv; //~ ERROR: variant `Priv` is private @@ -120,13 +123,14 @@ mod foo { ::bar::gpub(); - ::bar::baz::foo(); //~ ERROR: function `foo` is private + ::bar::baz::foo(); //~ ERROR: function `foo` is inaccessible + //~^ NOTE: module `baz` is private ::bar::baz::bar(); //~ ERROR: function `bar` is private } fn test2() { use bar::baz::{foo, bar}; - //~^ ERROR: function `foo` is private + //~^ ERROR: function `foo` is inaccessible //~^^ ERROR: function `bar` is private foo(); bar(); @@ -155,7 +159,8 @@ pub mod mytest { // external crates through `foo::foo`, it should not be accessible through // its definition path (which has the private `i` module). use self::foo::foo; - use self::foo::i::A; //~ ERROR: type `A` is private + use self::foo::i::A; //~ ERROR: type `A` is inaccessible + //~^ NOTE: module `i` is private pub mod foo { pub use foo = self::i::A;