From 13f5fca0f2c72b8af256f052b51bd636b6932c8a Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 26 Feb 2016 19:20:53 +0000 Subject: [PATCH 1/3] privacy: change def_privacy so that it checks for visiblity instead of nameability --- src/librustc_privacy/lib.rs | 106 +++++++++++++----------------------- 1 file changed, 38 insertions(+), 68 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 782ac593f4411..226539b8e804a 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -492,11 +492,6 @@ enum FieldName { } impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { - // used when debugging - fn nodestr(&self, id: ast::NodeId) -> String { - self.tcx.map.node_to_string(id).to_string() - } - // Determines whether the given definition is public from the point of view // of the current item. fn def_privacy(&self, did: DefId) -> PrivacyResult { @@ -604,75 +599,50 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { return Allowable; } - // We now know that there is at least one private member between the - // destination and the root. - let mut closest_private_id = node_id; - loop { - debug!("privacy - examining {}", self.nodestr(closest_private_id)); - let vis = match self.tcx.map.find(closest_private_id) { - // If this item is a method, then we know for sure that it's an - // actual method and not a static method. The reason for this is - // that these cases are only hit in the ExprMethodCall - // expression, and ExprCall will have its path checked later - // (the path of the trait/impl) if it's a static method. - // - // With this information, then we can completely ignore all - // trait methods. The privacy violation would be if the trait - // couldn't get imported, not if the method couldn't be used - // (all trait methods are public). - // - // However, if this is an impl method, then we dictate this - // decision solely based on the privacy of the method - // invocation. - // FIXME(#10573) is this the right behavior? Why not consider - // where the method was defined? - Some(ast_map::NodeImplItem(ii)) => { - match ii.node { - hir::ImplItemKind::Const(..) | - hir::ImplItemKind::Method(..) => { - let imp = self.tcx.map - .get_parent_did(closest_private_id); - match self.tcx.impl_trait_ref(imp) { - Some(..) => return Allowable, - _ if ii.vis == hir::Public => { - return Allowable - } - _ => ii.vis - } + let vis = match self.tcx.map.find(node_id) { + // If this item is a method, then we know for sure that it's an + // actual method and not a static method. The reason for this is + // that these cases are only hit in the ExprMethodCall + // expression, and ExprCall will have its path checked later + // (the path of the trait/impl) if it's a static method. + // + // With this information, then we can completely ignore all + // trait methods. The privacy violation would be if the trait + // couldn't get imported, not if the method couldn't be used + // (all trait methods are public). + // + // However, if this is an impl method, then we dictate this + // decision solely based on the privacy of the method + // invocation. + Some(ast_map::NodeImplItem(ii)) => { + match ii.node { + hir::ImplItemKind::Const(..) | + hir::ImplItemKind::Method(..) => { + let imp = self.tcx.map.get_parent_did(node_id); + match self.tcx.impl_trait_ref(imp) { + Some(..) => hir::Public, + _ => ii.vis } - hir::ImplItemKind::Type(_) => return Allowable, } + hir::ImplItemKind::Type(_) => hir::Public, } - Some(ast_map::NodeTraitItem(_)) => { - return Allowable; - } + } + Some(ast_map::NodeTraitItem(_)) => hir::Public, - // This is not a method call, extract the visibility as one - // would normally look at it - Some(ast_map::NodeItem(it)) => it.vis, - Some(ast_map::NodeForeignItem(_)) => { - self.tcx.map.get_foreign_vis(closest_private_id) - } - Some(ast_map::NodeVariant(..)) => { - hir::Public // need to move up a level (to the enum) - } - _ => hir::Public, - }; - if vis != hir::Public { break } - // if we've reached the root, then everything was allowable and this - // access is public. - if closest_private_id == ast::CRATE_NODE_ID { return Allowable } - closest_private_id = *self.parents.get(&closest_private_id).unwrap(); - - // If we reached the top, then we were public all the way down and - // we can allow this access. - if closest_private_id == ast::DUMMY_NODE_ID { return Allowable } - } - debug!("privacy - closest priv {}", self.nodestr(closest_private_id)); - if self.private_accessible(closest_private_id) { + // This is not a method call, extract the visibility as one + // would normally look at it + Some(ast_map::NodeItem(it)) => it.vis, + Some(ast_map::NodeForeignItem(_)) => { + self.tcx.map.get_foreign_vis(node_id) + } + _ => hir::Public, + }; + if vis == hir::Public { return Allowable } + + if self.private_accessible(node_id) { Allowable } else { - DisallowedBy(closest_private_id) + DisallowedBy(node_id) } } From 32f251cc1c1ff8721b68b4afe5f86a6bb33c6822 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 26 Feb 2016 19:24:15 +0000 Subject: [PATCH 2/3] Add test --- src/test/compile-fail/trait-privacy.rs | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 src/test/compile-fail/trait-privacy.rs diff --git a/src/test/compile-fail/trait-privacy.rs b/src/test/compile-fail/trait-privacy.rs new file mode 100644 index 0000000000000..a0d0014a06471 --- /dev/null +++ b/src/test/compile-fail/trait-privacy.rs @@ -0,0 +1,30 @@ +// Copyright 2016 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. + +#![feature(rustc_attrs)] +#![allow(dead_code)] + +mod foo { + pub use self::bar::T; + mod bar { + pub trait T { + fn f(&self) {} + } + impl T for () {} + } +} + +fn g() { + use foo::T; + ().f(); // Check that this does not trigger a privacy error +} + +#[rustc_error] +fn main() {} //~ ERROR compilation successful From d908ff1759bf27a8a8a99f113a246b8abc61f425 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 26 Feb 2016 20:31:00 +0000 Subject: [PATCH 3/3] Don't treat associated types specially in `def_privacy` Improve trait privacy error message --- src/librustc_privacy/lib.rs | 18 ++++++------------ src/test/compile-fail/trait-not-accessible.rs | 2 +- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 226539b8e804a..214ac81ee5092 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -615,16 +615,10 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { // decision solely based on the privacy of the method // invocation. Some(ast_map::NodeImplItem(ii)) => { - match ii.node { - hir::ImplItemKind::Const(..) | - hir::ImplItemKind::Method(..) => { - let imp = self.tcx.map.get_parent_did(node_id); - match self.tcx.impl_trait_ref(imp) { - Some(..) => hir::Public, - _ => ii.vis - } - } - hir::ImplItemKind::Type(_) => hir::Public, + let imp = self.tcx.map.get_parent_did(node_id); + match self.tcx.impl_trait_ref(imp) { + Some(..) => hir::Public, + _ => ii.vis, } } Some(ast_map::NodeTraitItem(_)) => hir::Public, @@ -804,8 +798,8 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { // Trait methods are always all public. The only controlling factor // is whether the trait itself is accessible or not. ty::TraitContainer(trait_def_id) => { - self.report_error(self.ensure_public(span, trait_def_id, - None, "source trait")); + let msg = format!("source trait `{}`", self.tcx.item_path_str(trait_def_id)); + self.report_error(self.ensure_public(span, trait_def_id, None, &msg)); } } } diff --git a/src/test/compile-fail/trait-not-accessible.rs b/src/test/compile-fail/trait-not-accessible.rs index 21668fcfeae71..5feef0a24eb0e 100644 --- a/src/test/compile-fail/trait-not-accessible.rs +++ b/src/test/compile-fail/trait-not-accessible.rs @@ -20,7 +20,7 @@ struct S; impl m::Pub for S {} fn g(arg: T) { - arg.f(); //~ ERROR: source trait is private + arg.f(); //~ ERROR: source trait `m::Priv` is private } fn main() {