From 62d181f474f5d3bb99ff157a9f27ec9f7e87938f Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 24 Feb 2016 10:58:35 +0000 Subject: [PATCH 1/7] Autoderef privacy for fields --- src/librustc_typeck/check/mod.rs | 83 ++++++++++++++++++-------------- 1 file changed, 48 insertions(+), 35 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index c5a0657594eb2..294ebb9915b94 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -2938,9 +2938,8 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, base: &'tcx hir::Expr, field: &Spanned) { check_expr_with_lvalue_pref(fcx, base, lvalue_pref); - let expr_t = structurally_resolved_type(fcx, expr.span, - fcx.expr_ty(base)); - // FIXME(eddyb) #12808 Integrate privacy into this auto-deref loop. + let expr_t = structurally_resolved_type(fcx, expr.span, fcx.expr_ty(base)); + let mut private_candidate = None; let (_, autoderefs, field_ty) = autoderef(fcx, expr.span, expr_t, @@ -2948,15 +2947,17 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, UnresolvedTypeAction::Error, lvalue_pref, |base_t, _| { - match base_t.sty { - ty::TyStruct(base_def, substs) => { - debug!("struct named {:?}", base_t); - base_def.struct_variant() - .find_field_named(field.node) - .map(|f| fcx.field_ty(expr.span, f, substs)) + if let ty::TyStruct(base_def, substs) = base_t.sty { + debug!("struct named {:?}", base_t); + if let Some(field) = base_def.struct_variant().find_field_named(field.node) { + let field_ty = fcx.field_ty(expr.span, field, substs); + if field.vis == hir::Public || fcx.private_item_is_visible(base_def.did) { + return Some(field_ty); + } + private_candidate = Some((base_def.did, field_ty)); } - _ => None } + None }); match field_ty { Some(field_ty) => { @@ -2967,12 +2968,14 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, None => {} } - if field.node == special_idents::invalid.name { + if let Some((did, field_ty)) = private_candidate { + let struct_path = fcx.tcx().item_path_str(did); + let msg = format!("field `{}` of struct `{}` is private", field.node, struct_path); + fcx.tcx().sess.span_err(expr.span, &msg); + fcx.write_ty(expr.id, field_ty); + } else if field.node == special_idents::invalid.name { fcx.write_error(expr.id); - return; - } - - if method::exists(fcx, field.span, field.node, expr_t, expr.id) { + } else if method::exists(fcx, field.span, field.node, expr_t, expr.id) { fcx.type_error_struct(field.span, |actual| { format!("attempted to take value of method `{}` on type \ @@ -2983,6 +2986,7 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, "maybe a `()` to call it is missing? \ If not, try an anonymous function") .emit(); + fcx.write_error(expr.id); } else { let mut err = fcx.type_error_struct( expr.span, @@ -2998,9 +3002,8 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, suggest_field_names(&mut err, def.struct_variant(), field, vec![]); } err.emit(); + fcx.write_error(expr.id); } - - fcx.write_error(expr.id); } // displays hints about the closest matches in field names @@ -3035,10 +3038,9 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, base: &'tcx hir::Expr, idx: codemap::Spanned) { check_expr_with_lvalue_pref(fcx, base, lvalue_pref); - let expr_t = structurally_resolved_type(fcx, expr.span, - fcx.expr_ty(base)); + let expr_t = structurally_resolved_type(fcx, expr.span, fcx.expr_ty(base)); + let mut private_candidate = None; let mut tuple_like = false; - // FIXME(eddyb) #12808 Integrate privacy into this auto-deref loop. let (_, autoderefs, field_ty) = autoderef(fcx, expr.span, expr_t, @@ -3046,25 +3048,27 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, UnresolvedTypeAction::Error, lvalue_pref, |base_t, _| { - match base_t.sty { - ty::TyStruct(base_def, substs) => { - tuple_like = base_def.struct_variant().is_tuple_struct(); - if tuple_like { - debug!("tuple struct named {:?}", base_t); - base_def.struct_variant() - .fields - .get(idx.node) - .map(|f| fcx.field_ty(expr.span, f, substs)) - } else { - None - } - } + let (base_def, substs) = match base_t.sty { + ty::TyStruct(base_def, substs) => (base_def, substs), ty::TyTuple(ref v) => { tuple_like = true; - if idx.node < v.len() { Some(v[idx.node]) } else { None } + return if idx.node < v.len() { Some(v[idx.node]) } else { None } } - _ => None + _ => return None, + }; + + tuple_like = base_def.struct_variant().is_tuple_struct(); + if !tuple_like { return None } + + debug!("tuple struct named {:?}", base_t); + if let Some(field) = base_def.struct_variant().fields.get(idx.node) { + let field_ty = fcx.field_ty(expr.span, field, substs); + if field.vis == hir::Public || fcx.private_item_is_visible(base_def.did) { + return Some(field_ty); + } + private_candidate = Some((base_def.did, field_ty)); } + None }); match field_ty { Some(field_ty) => { @@ -3074,6 +3078,15 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, } None => {} } + + if let Some((did, field_ty)) = private_candidate { + let struct_path = fcx.tcx().item_path_str(did); + let msg = format!("field `{}` of struct `{}` is private", idx.node, struct_path); + fcx.tcx().sess.span_err(expr.span, &msg); + fcx.write_ty(expr.id, field_ty); + return; + } + fcx.type_error_message( expr.span, |actual| { From 6f16c5c81f86be4b4b50ef5396c6412844235dbf Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 25 Feb 2016 02:11:29 +0000 Subject: [PATCH 2/7] Autoderef privacy for methods --- src/librustc_typeck/check/method/mod.rs | 4 +++ src/librustc_typeck/check/method/probe.rs | 18 +++++++++++- src/librustc_typeck/check/method/suggest.rs | 7 ++++- src/librustc_typeck/check/mod.rs | 31 +++++++++++++-------- 4 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index 11f7feba86b32..8c8d02bd3e6d6 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/src/librustc_typeck/check/method/mod.rs @@ -43,6 +43,9 @@ pub enum MethodError<'tcx> { // Using a `Fn`/`FnMut`/etc method on a raw closure type before we have inferred its kind. ClosureAmbiguity(/* DefId of fn trait */ DefId), + + // Found an applicable method, but it is not visible. + PrivateMatch(Def), } // Contains a list of static methods that may apply, a list of unsatisfied trait predicates which @@ -90,6 +93,7 @@ pub fn exists<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, Err(NoMatch(..)) => false, Err(Ambiguity(..)) => true, Err(ClosureAmbiguity(..)) => true, + Err(PrivateMatch(..)) => true, } } diff --git a/src/librustc_typeck/check/method/probe.rs b/src/librustc_typeck/check/method/probe.rs index 4b42846297b16..477b46ce4cef4 100644 --- a/src/librustc_typeck/check/method/probe.rs +++ b/src/librustc_typeck/check/method/probe.rs @@ -16,6 +16,7 @@ use super::suggest; use check; use check::{FnCtxt, UnresolvedTypeAction}; use middle::def_id::DefId; +use middle::def::Def; use rustc::ty::subst; use rustc::ty::subst::Subst; use rustc::traits; @@ -47,6 +48,9 @@ struct ProbeContext<'a, 'tcx:'a> { /// used for error reporting static_candidates: Vec, + /// Some(candidate) if there is a private candidate + private_candidate: Option, + /// Collects near misses when trait bounds for type parameters are unsatisfied and is only used /// for error reporting unsatisfied_predicates: Vec> @@ -247,6 +251,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { steps: Rc::new(steps), opt_simplified_steps: opt_simplified_steps, static_candidates: Vec::new(), + private_candidate: None, unsatisfied_predicates: Vec::new(), } } @@ -256,6 +261,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { self.extension_candidates.clear(); self.impl_dups.clear(); self.static_candidates.clear(); + self.private_candidate = None; } fn tcx(&self) -> &'a TyCtxt<'tcx> { @@ -407,6 +413,11 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { return self.record_static_candidate(ImplSource(impl_def_id)); } + if item.vis() != hir::Public && !self.fcx.private_item_is_visible(item.def_id()) { + self.private_candidate = Some(item.def()); + return + } + let (impl_ty, impl_substs) = self.impl_ty_and_substs(impl_def_id); let impl_ty = impl_ty.subst(self.tcx(), &impl_substs); @@ -846,6 +857,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { } let static_candidates = mem::replace(&mut self.static_candidates, vec![]); + let private_candidate = mem::replace(&mut self.private_candidate, None); let unsatisfied_predicates = mem::replace(&mut self.unsatisfied_predicates, vec![]); // things failed, so lets look at all traits, for diagnostic purposes now: @@ -879,9 +891,13 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> { // this error only occurs when assembling candidates tcx.sess.span_bug(span, "encountered ClosureAmbiguity from pick_core"); } - None => vec![], + _ => vec![], }; + if let Some(def) = private_candidate { + return Err(MethodError::PrivateMatch(def)); + } + Err(MethodError::NoMatch(NoMatchData::new(static_candidates, unsatisfied_predicates, out_of_scope_traits, self.mode))) } diff --git a/src/librustc_typeck/check/method/suggest.rs b/src/librustc_typeck/check/method/suggest.rs index aae7912cace40..f1d67883117ec 100644 --- a/src/librustc_typeck/check/method/suggest.rs +++ b/src/librustc_typeck/check/method/suggest.rs @@ -91,7 +91,7 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, MethodError::NoMatch(NoMatchData { static_candidates: static_sources, unsatisfied_predicates, out_of_scope_traits, - mode }) => { + mode, .. }) => { let cx = fcx.tcx(); let mut err = fcx.type_error_struct( @@ -208,6 +208,11 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, }; fcx.sess().span_err(span, &msg); } + + MethodError::PrivateMatch(def) => { + let msg = format!("{} `{}` is private", def.kind_name(), item_name); + fcx.tcx().sess.span_err(span, &msg); + } } fn report_candidates(fcx: &FnCtxt, diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 294ebb9915b94..78694b2e96f8a 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -3757,23 +3757,30 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>, &ty_segments[base_ty_end..]); let item_segment = path.segments.last().unwrap(); let item_name = item_segment.identifier.name; - match method::resolve_ufcs(fcx, span, item_name, ty, node_id) { - Ok(def) => { - // Write back the new resolution. - fcx.ccx.tcx.def_map.borrow_mut() - .insert(node_id, def::PathResolution { - base_def: def, - depth: 0 - }); - Some((Some(ty), slice::ref_slice(item_segment), def)) - } + let def = match method::resolve_ufcs(fcx, span, item_name, ty, node_id) { + Ok(def) => Some(def), Err(error) => { + let def = match error { + method::MethodError::PrivateMatch(def) => Some(def), + _ => None, + }; if item_name != special_idents::invalid.name { method::report_error(fcx, span, ty, item_name, None, error); } - fcx.write_error(node_id); - None + def } + }; + + if let Some(def) = def { + // Write back the new resolution. + fcx.ccx.tcx.def_map.borrow_mut().insert(node_id, def::PathResolution { + base_def: def, + depth: 0, + }); + Some((Some(ty), slice::ref_slice(item_segment), def)) + } else { + fcx.write_error(node_id); + None } } } From 09af5036da4087569a033eea91c8b9dd63a079a0 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 3 Mar 2016 02:32:55 +0000 Subject: [PATCH 3/7] Fix fallout in tests --- src/test/compile-fail/privacy1.rs | 1 + src/test/compile-fail/regions-glb-free-free.rs | 2 +- src/test/parse-fail/pub-method-macro.rs | 4 +--- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/test/compile-fail/privacy1.rs b/src/test/compile-fail/privacy1.rs index 9b11eafaa63c3..afe8c2fda3d4c 100644 --- a/src/test/compile-fail/privacy1.rs +++ b/src/test/compile-fail/privacy1.rs @@ -102,6 +102,7 @@ mod foo { //~^ ERROR: method `bar` is private ::bar::baz::A.foo2(); //~ ERROR: module `baz` is private ::bar::baz::A.bar2(); //~ ERROR: module `baz` is private + //~^ ERROR: method `bar2` is private let _: isize = ::bar::B::foo(); //~ ERROR: trait `B` is private diff --git a/src/test/compile-fail/regions-glb-free-free.rs b/src/test/compile-fail/regions-glb-free-free.rs index c2e4fbac3c941..586a8a183a4e6 100644 --- a/src/test/compile-fail/regions-glb-free-free.rs +++ b/src/test/compile-fail/regions-glb-free-free.rs @@ -11,7 +11,7 @@ mod argparse { pub struct Flag<'a> { name: &'a str, - desc: &'a str, + pub desc: &'a str, max_count: usize, value: usize } diff --git a/src/test/parse-fail/pub-method-macro.rs b/src/test/parse-fail/pub-method-macro.rs index 198fa5b9aca0b..83db24b8c01ef 100644 --- a/src/test/parse-fail/pub-method-macro.rs +++ b/src/test/parse-fail/pub-method-macro.rs @@ -29,6 +29,4 @@ mod bleh { } } -fn main() { - bleh::S.f(); -} +fn main() {} From 8762fc31db55c641083bf31539da3f079877c8b1 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Fri, 26 Feb 2016 18:58:45 +0000 Subject: [PATCH 4/7] Add tests for #12808 and #22684 --- src/test/compile-fail/autoderef-privacy.rs | 49 ++++++++++++++++++++++ src/test/compile-fail/issue-22684.rs | 28 +++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 src/test/compile-fail/autoderef-privacy.rs create mode 100644 src/test/compile-fail/issue-22684.rs diff --git a/src/test/compile-fail/autoderef-privacy.rs b/src/test/compile-fail/autoderef-privacy.rs new file mode 100644 index 0000000000000..359cee760db1c --- /dev/null +++ b/src/test/compile-fail/autoderef-privacy.rs @@ -0,0 +1,49 @@ +// 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. + +// Check we do not select a private method or field when computing autoderefs + +#![feature(rustc_attrs)] +#![allow(unused)] + +pub struct Bar2 { i: i32 } +pub struct Baz2(i32); + +impl Bar2 { + fn f(&self) {} +} + +mod foo { + pub struct Bar { i: i32 } + pub struct Baz(i32); + + impl Bar { + fn f(&self) {} + } + + impl ::std::ops::Deref for Bar { + type Target = ::Bar2; + fn deref(&self) -> &::Bar2 { unimplemented!() } + } + + impl ::std::ops::Deref for Baz { + type Target = ::Baz2; + fn deref(&self) -> &::Baz2 { unimplemented!() } + } +} + +fn f(bar: foo::Bar, baz: foo::Baz) { + let _ = bar.i; + let _ = baz.0; + let _ = bar.f(); +} + +#[rustc_error] +fn main() {} //~ ERROR compilation successful diff --git a/src/test/compile-fail/issue-22684.rs b/src/test/compile-fail/issue-22684.rs new file mode 100644 index 0000000000000..b7ffbefba6a08 --- /dev/null +++ b/src/test/compile-fail/issue-22684.rs @@ -0,0 +1,28 @@ +// 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. + +mod foo { + pub struct Foo; + impl Foo { + fn bar(&self) {} + } + + pub trait Baz { + fn bar(&self) -> bool {} + } + impl Baz for Foo {} +} + +fn main() { + use foo::Baz; + + // Check that `bar` resolves to the trait method, not the inherent impl method. + let _: () = foo::Foo.bar(); //~ ERROR mismatched types +} From 48b048deb732b31d6768876074f7bc9d0e036645 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 16 Mar 2016 02:50:34 +0000 Subject: [PATCH 5/7] Clean up the privacy visitor --- src/librustc/front/map/mod.rs | 8 - src/librustc_privacy/lib.rs | 431 ++-------------------------------- 2 files changed, 25 insertions(+), 414 deletions(-) diff --git a/src/librustc/front/map/mod.rs b/src/librustc/front/map/mod.rs index 6d6f20c70ec4d..3605de44495b1 100644 --- a/src/librustc/front/map/mod.rs +++ b/src/librustc/front/map/mod.rs @@ -581,14 +581,6 @@ impl<'ast> Map<'ast> { } } - pub fn get_foreign_vis(&self, id: NodeId) -> Visibility { - let vis = self.expect_foreign_item(id).vis; // read recorded by `expect_foreign_item` - match self.find(self.get_parent(id)) { // read recorded by `find` - Some(NodeItem(i)) => vis.inherit_from(i.vis), - _ => vis - } - } - pub fn expect_item(&self, id: NodeId) -> &'ast Item { match self.find(id) { // read recorded by `find` Some(NodeItem(item)) => item, diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 304932df412ad..5bef33af45210 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -27,7 +27,6 @@ extern crate rustc; extern crate rustc_front; -use self::PrivacyResult::*; use self::FieldName::*; use std::cmp; @@ -43,7 +42,7 @@ use rustc::middle::def::{self, Def}; use rustc::middle::def_id::DefId; use rustc::middle::privacy::{AccessLevel, AccessLevels}; use rustc::ty::{self, TyCtxt}; -use rustc::util::nodemap::{NodeMap, NodeSet}; +use rustc::util::nodemap::NodeSet; use rustc::front::map as ast_map; use syntax::ast; @@ -58,98 +57,6 @@ type Context<'a, 'tcx> = (&'a ty::MethodMap<'tcx>, &'a def::ExportMap); /// optionally the same for a note about the error. type CheckResult = Option<(Span, String, Option<(Span, String)>)>; -//////////////////////////////////////////////////////////////////////////////// -/// The parent visitor, used to determine what's the parent of what (node-wise) -//////////////////////////////////////////////////////////////////////////////// - -struct ParentVisitor<'a, 'tcx:'a> { - tcx: &'a TyCtxt<'tcx>, - parents: NodeMap, - curparent: ast::NodeId, -} - -impl<'a, 'tcx, 'v> Visitor<'v> for ParentVisitor<'a, 'tcx> { - /// We want to visit items in the context of their containing - /// module and so forth, so supply a crate for doing a deep walk. - fn visit_nested_item(&mut self, item: hir::ItemId) { - self.visit_item(self.tcx.map.expect_item(item.id)) - } - fn visit_item(&mut self, item: &hir::Item) { - self.parents.insert(item.id, self.curparent); - - let prev = self.curparent; - match item.node { - hir::ItemMod(..) => { self.curparent = item.id; } - // Enum variants are parented to the enum definition itself because - // they inherit privacy - hir::ItemEnum(ref def, _) => { - for variant in &def.variants { - // The parent is considered the enclosing enum because the - // enum will dictate the privacy visibility of this variant - // instead. - self.parents.insert(variant.node.data.id(), item.id); - } - } - - // Trait methods are always considered "public", but if the trait is - // private then we need some private item in the chain from the - // method to the root. In this case, if the trait is private, then - // parent all the methods to the trait to indicate that they're - // private. - hir::ItemTrait(_, _, _, ref trait_items) if item.vis != hir::Public => { - for trait_item in trait_items { - self.parents.insert(trait_item.id, item.id); - } - } - - _ => {} - } - intravisit::walk_item(self, item); - self.curparent = prev; - } - - fn visit_foreign_item(&mut self, a: &hir::ForeignItem) { - self.parents.insert(a.id, self.curparent); - intravisit::walk_foreign_item(self, a); - } - - fn visit_fn(&mut self, a: intravisit::FnKind<'v>, b: &'v hir::FnDecl, - c: &'v hir::Block, d: Span, id: ast::NodeId) { - // We already took care of some trait methods above, otherwise things - // like impl methods and pub trait methods are parented to the - // containing module, not the containing trait. - if !self.parents.contains_key(&id) { - self.parents.insert(id, self.curparent); - } - intravisit::walk_fn(self, a, b, c, d); - } - - fn visit_impl_item(&mut self, ii: &'v hir::ImplItem) { - // visit_fn handles methods, but associated consts have to be handled - // here. - if !self.parents.contains_key(&ii.id) { - self.parents.insert(ii.id, self.curparent); - } - intravisit::walk_impl_item(self, ii); - } - - fn visit_variant_data(&mut self, s: &hir::VariantData, _: ast::Name, - _: &'v hir::Generics, item_id: ast::NodeId, _: Span) { - // Struct constructors are parented to their struct definitions because - // they essentially are the struct definitions. - if !s.is_struct() { - self.parents.insert(s.id(), item_id); - } - - // While we have the id of the struct definition, go ahead and parent - // all the fields. - for field in s.fields() { - self.parents.insert(field.id, self.curparent); - } - intravisit::walk_struct_def(self, s) - } -} - //////////////////////////////////////////////////////////////////////////////// /// The embargo visitor, used to determine the exports of the ast //////////////////////////////////////////////////////////////////////////////// @@ -475,14 +382,6 @@ struct PrivacyVisitor<'a, 'tcx: 'a> { tcx: &'a TyCtxt<'tcx>, curitem: ast::NodeId, in_foreign: bool, - parents: NodeMap, -} - -#[derive(Debug)] -enum PrivacyResult { - Allowable, - ExternallyDenied, - DisallowedBy(ast::NodeId), } enum FieldName { @@ -491,257 +390,22 @@ enum FieldName { } impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { - // Determines whether the given definition is public from the point of view - // of the current item. - fn def_privacy(&self, did: DefId) -> PrivacyResult { - let node_id = if let Some(node_id) = self.tcx.map.as_local_node_id(did) { - node_id - } else { - if self.tcx.sess.cstore.visibility(did) == hir::Public { - debug!("privacy - {:?} was externally exported", did); - return Allowable; - } - debug!("privacy - is {:?} a public method", did); - - return match self.tcx.impl_or_trait_items.borrow().get(&did) { - Some(&ty::ConstTraitItem(ref ac)) => { - debug!("privacy - it's a const: {:?}", *ac); - match ac.container { - ty::TraitContainer(id) => { - debug!("privacy - recursing on trait {:?}", id); - self.def_privacy(id) - } - ty::ImplContainer(id) => { - match self.tcx.impl_trait_ref(id) { - Some(t) => { - debug!("privacy - impl of trait {:?}", id); - self.def_privacy(t.def_id) - } - None => { - debug!("privacy - found inherent \ - associated constant {:?}", - ac.vis); - if ac.vis == hir::Public { - Allowable - } else { - ExternallyDenied - } - } - } - } - } - } - Some(&ty::MethodTraitItem(ref meth)) => { - debug!("privacy - well at least it's a method: {:?}", - *meth); - match meth.container { - ty::TraitContainer(id) => { - debug!("privacy - recursing on trait {:?}", id); - self.def_privacy(id) - } - ty::ImplContainer(id) => { - match self.tcx.impl_trait_ref(id) { - Some(t) => { - debug!("privacy - impl of trait {:?}", id); - self.def_privacy(t.def_id) - } - None => { - debug!("privacy - found a method {:?}", - meth.vis); - if meth.vis == hir::Public { - Allowable - } else { - ExternallyDenied - } - } - } - } - } - } - Some(&ty::TypeTraitItem(ref typedef)) => { - match typedef.container { - ty::TraitContainer(id) => { - debug!("privacy - recursing on trait {:?}", id); - self.def_privacy(id) - } - ty::ImplContainer(id) => { - match self.tcx.impl_trait_ref(id) { - Some(t) => { - debug!("privacy - impl of trait {:?}", id); - self.def_privacy(t.def_id) - } - None => { - debug!("privacy - found a typedef {:?}", - typedef.vis); - if typedef.vis == hir::Public { - Allowable - } else { - ExternallyDenied - } - } - } - } - } - } - None => { - debug!("privacy - nope, not even a method"); - ExternallyDenied - } - }; - }; - - debug!("privacy - local {} not public all the way down", - self.tcx.map.node_to_string(node_id)); - // return quickly for things in the same module - if self.parents.get(&node_id) == self.parents.get(&self.curitem) { - debug!("privacy - same parent, we're done here"); - return Allowable; - } - - 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)) => { - 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, - - // 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, + fn item_is_visible(&self, did: DefId) -> bool { + let visibility = match self.tcx.map.as_local_node_id(did) { + Some(node_id) => self.tcx.map.expect_item(node_id).vis, + None => self.tcx.sess.cstore.visibility(did), }; - if vis == hir::Public { return Allowable } - - if self.private_accessible(node_id) { - Allowable - } else { - DisallowedBy(node_id) - } + visibility == hir::Public || self.private_accessible(did) } - /// True if `id` is both local and private-accessible - fn local_private_accessible(&self, did: DefId) -> bool { - if let Some(node_id) = self.tcx.map.as_local_node_id(did) { - self.private_accessible(node_id) - } else { - false + /// True if `did` is private-accessible + fn private_accessible(&self, did: DefId) -> bool { + match self.tcx.map.as_local_node_id(did) { + Some(node_id) => self.tcx.map.private_item_is_visible_from(node_id, self.curitem), + None => false, } } - /// For a local private node in the AST, this function will determine - /// whether the node is accessible by the current module that iteration is - /// inside. - fn private_accessible(&self, id: ast::NodeId) -> bool { - self.tcx.map.private_item_is_visible_from(id, self.curitem) - } - - fn report_error(&self, result: CheckResult) -> bool { - match result { - None => true, - Some((span, msg, note)) => { - let mut err = self.tcx.sess.struct_span_err(span, &msg[..]); - if let Some((span, msg)) = note { - err.span_note(span, &msg[..]); - } - err.emit(); - false - }, - } - } - - /// Guarantee that a particular definition is public. Returns a CheckResult - /// which contains any errors found. These can be reported using `report_error`. - /// If the result is `None`, no errors were found. - fn ensure_public(&self, - span: Span, - to_check: DefId, - source_did: Option, - msg: &str) - -> CheckResult { - debug!("ensure_public(span={:?}, to_check={:?}, source_did={:?}, msg={:?})", - span, to_check, source_did, msg); - let def_privacy = self.def_privacy(to_check); - debug!("ensure_public: def_privacy={:?}", def_privacy); - let id = match def_privacy { - ExternallyDenied => { - return Some((span, format!("{} is private", msg), None)) - } - Allowable => return None, - DisallowedBy(id) => id, - }; - - // If we're disallowed by a particular id, then we attempt to - // give a nice error message to say why it was disallowed. It - // was either because the item itself is private or because - // its parent is private and its parent isn't in our - // ancestry. (Both the item being checked and its parent must - // be local.) - let def_id = source_did.unwrap_or(to_check); - let node_id = self.tcx.map.as_local_node_id(def_id); - - let (err_span, err_msg) = if Some(id) == node_id { - return Some((span, format!("{} is private", msg), None)); - } else { - (span, format!("{} is inaccessible", msg)) - }; - let item = match self.tcx.map.find(id) { - Some(ast_map::NodeItem(item)) => { - match item.node { - // If an impl disallowed this item, then this is resolve's - // way of saying that a struct/enum's static method was - // invoked, and the struct/enum itself is private. Crawl - // back up the chains to find the relevant struct/enum that - // was private. - hir::ItemImpl(_, _, _, _, ref ty, _) => { - match ty.node { - hir::TyPath(..) => {} - _ => return Some((err_span, err_msg, None)), - }; - let def = self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def(); - let did = def.def_id(); - let node_id = self.tcx.map.as_local_node_id(did).unwrap(); - match self.tcx.map.get(node_id) { - ast_map::NodeItem(item) => item, - _ => self.tcx.sess.span_bug(item.span, - "path is not an item") - } - } - _ => item - } - } - Some(..) | None => return Some((err_span, err_msg, None)), - }; - let desc = match item.node { - hir::ItemMod(..) => "module", - hir::ItemTrait(..) => "trait", - hir::ItemStruct(..) => "struct", - hir::ItemEnum(..) => "enum", - _ => return Some((err_span, err_msg, None)) - }; - let msg = format!("{} `{}` is private", desc, item.name); - Some((err_span, err_msg, Some((span, msg)))) - } - // Checks that a field is in scope. fn check_field(&mut self, span: Span, @@ -749,13 +413,10 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { v: ty::VariantDef<'tcx>, name: FieldName) { let field = match name { - NamedField(f_name) => { - debug!("privacy - check named field {} in struct {:?}", f_name, def); - v.field_named(f_name) - } + NamedField(f_name) => v.field_named(f_name), UnnamedField(idx) => &v.fields[idx] }; - if field.vis == hir::Public || self.local_private_accessible(def.did) { + if field.vis == hir::Public || self.private_accessible(def.did) { return; } @@ -766,40 +427,23 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { ty::AdtKind::Enum => return }; let msg = match name { - NamedField(name) => format!("field `{}` of {} is private", - name, struct_desc), - UnnamedField(idx) => format!("field #{} of {} is private", - idx, struct_desc), + NamedField(name) => format!("field `{}` of {} is private", name, struct_desc), + UnnamedField(idx) => format!("field #{} of {} is private", idx, struct_desc), }; - span_err!(self.tcx.sess, span, E0451, - "{}", &msg[..]); - } - - // Given the ID of a method, checks to ensure it's in scope. - fn check_static_method(&mut self, - span: Span, - method_id: DefId, - name: ast::Name) { - self.report_error(self.ensure_public(span, - method_id, - None, - &format!("method `{}`", - name))); + span_err!(self.tcx.sess, span, E0451, "{}", msg); } // Checks that a method is in scope. - fn check_method(&mut self, span: Span, method_def_id: DefId, - name: ast::Name) { + fn check_method(&mut self, span: Span, method_def_id: DefId) { match self.tcx.impl_or_trait_item(method_def_id).container() { - ty::ImplContainer(_) => { - self.check_static_method(span, method_def_id, name) - } // Trait methods are always all public. The only controlling factor // is whether the trait itself is accessible or not. - ty::TraitContainer(trait_def_id) => { - 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)); + ty::TraitContainer(trait_def_id) if !self.item_is_visible(trait_def_id) => { + let msg = format!("source trait `{}` is private", + self.tcx.item_path_str(trait_def_id)); + self.tcx.sess.span_err(span, &msg); } + _ => {} } } } @@ -819,27 +463,11 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &hir::Expr) { match expr.node { - hir::ExprField(ref base, name) => { - if let ty::TyStruct(def, _) = self.tcx.expr_ty_adjusted(&base).sty { - self.check_field(expr.span, - def, - def.struct_variant(), - NamedField(name.node)); - } - } - hir::ExprTupField(ref base, idx) => { - if let ty::TyStruct(def, _) = self.tcx.expr_ty_adjusted(&base).sty { - self.check_field(expr.span, - def, - def.struct_variant(), - UnnamedField(idx.node)); - } - } - hir::ExprMethodCall(name, _, _) => { + hir::ExprMethodCall(..) => { let method_call = ty::MethodCall::expr(expr.id); let method = self.tcx.tables.borrow().method_map[&method_call]; debug!("(privacy checking) checking impl method"); - self.check_method(expr.span, method.def_id, name.node); + self.check_method(expr.span, method.def_id); } hir::ExprStruct(..) => { let adt = self.tcx.expr_ty(expr).ty_adt_def().unwrap(); @@ -862,7 +490,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> { _ => expr_ty }.ty_adt_def().unwrap(); let any_priv = def.struct_variant().fields.iter().any(|f| { - f.vis != hir::Public && !self.local_private_accessible(def.did) + f.vis != hir::Public && !self.private_accessible(def.did) }); if any_priv { span_err!(self.tcx.sess, expr.span, E0450, @@ -1575,20 +1203,11 @@ pub fn check_crate(tcx: &TyCtxt, export_map: &def::ExportMap) -> AccessLevels { let mut visitor = SanePrivacyVisitor { tcx: tcx }; krate.visit_all_items(&mut visitor); - // Figure out who everyone's parent is - let mut visitor = ParentVisitor { - tcx: tcx, - parents: NodeMap(), - curparent: ast::DUMMY_NODE_ID, - }; - intravisit::walk_crate(&mut visitor, krate); - // Use the parent map to check the privacy of everything let mut visitor = PrivacyVisitor { curitem: ast::DUMMY_NODE_ID, in_foreign: false, tcx: tcx, - parents: visitor.parents, }; intravisit::walk_crate(&mut visitor, krate); From 38bef4365219dba7b82a3de19a2b84b1d322abf1 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 17 Mar 2016 00:34:18 +0300 Subject: [PATCH 6/7] privacy: Cleanup check_field --- src/librustc_privacy/lib.rs | 45 ++++++------------------------- src/test/compile-fail/privacy5.rs | 36 ++++++++++++------------- 2 files changed, 26 insertions(+), 55 deletions(-) diff --git a/src/librustc_privacy/lib.rs b/src/librustc_privacy/lib.rs index 5bef33af45210..d4b309fb039c1 100644 --- a/src/librustc_privacy/lib.rs +++ b/src/librustc_privacy/lib.rs @@ -27,8 +27,6 @@ extern crate rustc; extern crate rustc_front; -use self::FieldName::*; - use std::cmp; use std::mem::replace; @@ -384,11 +382,6 @@ struct PrivacyVisitor<'a, 'tcx: 'a> { in_foreign: bool, } -enum FieldName { - UnnamedField(usize), // index - NamedField(ast::Name), -} - impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { fn item_is_visible(&self, did: DefId) -> bool { let visibility = match self.tcx.map.as_local_node_id(did) { @@ -407,30 +400,12 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> { } // Checks that a field is in scope. - fn check_field(&mut self, - span: Span, - def: ty::AdtDef<'tcx>, - v: ty::VariantDef<'tcx>, - name: FieldName) { - let field = match name { - NamedField(f_name) => v.field_named(f_name), - UnnamedField(idx) => &v.fields[idx] - }; - if field.vis == hir::Public || self.private_accessible(def.did) { - return; + fn check_field(&mut self, span: Span, def: ty::AdtDef<'tcx>, field: ty::FieldDef<'tcx>) { + if def.adt_kind() == ty::AdtKind::Struct && + field.vis != hir::Public && !self.private_accessible(def.did) { + span_err!(self.tcx.sess, span, E0451, "field `{}` of struct `{}` is private", + field.name, self.tcx.item_path_str(def.did)); } - - let struct_desc = match def.adt_kind() { - ty::AdtKind::Struct => - format!("struct `{}`", self.tcx.item_path_str(def.did)), - // struct variant fields have inherited visibility - ty::AdtKind::Enum => return - }; - let msg = match name { - NamedField(name) => format!("field `{}` of {} is private", name, struct_desc), - UnnamedField(idx) => format!("field #{} of {} is private", idx, struct_desc), - }; - span_err!(self.tcx.sess, span, E0451, "{}", msg); } // Checks that a method is in scope. @@ -476,7 +451,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> { // Rather than computing the set of unmentioned fields // (i.e. `all_fields - fields`), just check them all. for field in &variant.fields { - self.check_field(expr.span, adt, variant, NamedField(field.name)); + self.check_field(expr.span, adt, field); } } hir::ExprPath(..) => { @@ -518,8 +493,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> { let def = self.tcx.def_map.borrow().get(&pattern.id).unwrap().full_def(); let variant = adt.variant_of_def(def); for field in fields { - self.check_field(pattern.span, adt, variant, - NamedField(field.node.name)); + self.check_field(pattern.span, adt, variant.field_named(field.node.name)); } } @@ -532,10 +506,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> { if let PatKind::Wild = field.node { continue } - self.check_field(field.span, - def, - def.struct_variant(), - UnnamedField(i)); + self.check_field(field.span, def, &def.struct_variant().fields[i]); } } ty::TyEnum(..) => { diff --git a/src/test/compile-fail/privacy5.rs b/src/test/compile-fail/privacy5.rs index 588c9be3065f8..9d6ae187cd381 100644 --- a/src/test/compile-fail/privacy5.rs +++ b/src/test/compile-fail/privacy5.rs @@ -63,25 +63,25 @@ fn this_crate() { let c = a::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor let d = a::D(4); - let a::A(()) = a; //~ ERROR: field #0 of struct `a::A` is private + let a::A(()) = a; //~ ERROR: field `0` of struct `a::A` is private let a::A(_) = a; - match a { a::A(()) => {} } //~ ERROR: field #0 of struct `a::A` is private + match a { a::A(()) => {} } //~ ERROR: field `0` of struct `a::A` is private match a { a::A(_) => {} } let a::B(_) = b; - let a::B(_b) = b; //~ ERROR: field #0 of struct `a::B` is private + let a::B(_b) = b; //~ ERROR: field `0` of struct `a::B` is private match b { a::B(_) => {} } - match b { a::B(_b) => {} } //~ ERROR: field #0 of struct `a::B` is private - match b { a::B(1) => {} a::B(_) => {} } //~ ERROR: field #0 of struct `a::B` is private + match b { a::B(_b) => {} } //~ ERROR: field `0` of struct `a::B` is private + match b { a::B(1) => {} a::B(_) => {} } //~ ERROR: field `0` of struct `a::B` is private let a::C(_, _) = c; let a::C(_a, _) = c; - let a::C(_, _b) = c; //~ ERROR: field #1 of struct `a::C` is private - let a::C(_a, _b) = c; //~ ERROR: field #1 of struct `a::C` is private + let a::C(_, _b) = c; //~ ERROR: field `1` of struct `a::C` is private + let a::C(_a, _b) = c; //~ ERROR: field `1` of struct `a::C` is private match c { a::C(_, _) => {} } match c { a::C(_a, _) => {} } - match c { a::C(_, _b) => {} } //~ ERROR: field #1 of struct `a::C` is private - match c { a::C(_a, _b) => {} } //~ ERROR: field #1 of struct `a::C` is private + match c { a::C(_, _b) => {} } //~ ERROR: field `1` of struct `a::C` is private + match c { a::C(_a, _b) => {} } //~ ERROR: field `1` of struct `a::C` is private let a::D(_) = d; let a::D(_d) = d; @@ -101,30 +101,30 @@ fn xcrate() { let c = other::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor let d = other::D(4); - let other::A(()) = a; //~ ERROR: field #0 of struct `other::A` is private + let other::A(()) = a; //~ ERROR: field `0` of struct `other::A` is private let other::A(_) = a; match a { other::A(()) => {} } - //~^ ERROR: field #0 of struct `other::A` is private + //~^ ERROR: field `0` of struct `other::A` is private match a { other::A(_) => {} } let other::B(_) = b; - let other::B(_b) = b; //~ ERROR: field #0 of struct `other::B` is private + let other::B(_b) = b; //~ ERROR: field `0` of struct `other::B` is private match b { other::B(_) => {} } match b { other::B(_b) => {} } - //~^ ERROR: field #0 of struct `other::B` is private + //~^ ERROR: field `0` of struct `other::B` is private match b { other::B(1) => {} other::B(_) => {} } - //~^ ERROR: field #0 of struct `other::B` is private + //~^ ERROR: field `0` of struct `other::B` is private let other::C(_, _) = c; let other::C(_a, _) = c; - let other::C(_, _b) = c; //~ ERROR: field #1 of struct `other::C` is private - let other::C(_a, _b) = c; //~ ERROR: field #1 of struct `other::C` is private + let other::C(_, _b) = c; //~ ERROR: field `1` of struct `other::C` is private + let other::C(_a, _b) = c; //~ ERROR: field `1` of struct `other::C` is private match c { other::C(_, _) => {} } match c { other::C(_a, _) => {} } match c { other::C(_, _b) => {} } - //~^ ERROR: field #1 of struct `other::C` is private + //~^ ERROR: field `1` of struct `other::C` is private match c { other::C(_a, _b) => {} } - //~^ ERROR: field #1 of struct `other::C` is private + //~^ ERROR: field `1` of struct `other::C` is private let other::D(_) = d; let other::D(_d) = d; From 48c20b0e73b083090c6dcf65ecd460eb073cc0b4 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Wed, 30 Mar 2016 00:35:53 +0000 Subject: [PATCH 7/7] Improve tests --- src/test/compile-fail/struct-field-privacy.rs | 6 ++- .../autoderef-privacy.rs | 39 ++++++++++++------- 2 files changed, 30 insertions(+), 15 deletions(-) rename src/test/{compile-fail => run-pass}/autoderef-privacy.rs (53%) diff --git a/src/test/compile-fail/struct-field-privacy.rs b/src/test/compile-fail/struct-field-privacy.rs index 1dd8ec0136ef5..f487ef62aa435 100644 --- a/src/test/compile-fail/struct-field-privacy.rs +++ b/src/test/compile-fail/struct-field-privacy.rs @@ -25,9 +25,10 @@ mod inner { pub a: isize, b: isize, } + pub struct Z(pub isize, isize); } -fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) { +fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B, z: inner::Z) { a.a; b.a; //~ ERROR: field `a` of struct `inner::A` is private b.b; @@ -39,6 +40,9 @@ fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) { e.a; e.b; //~ ERROR: field `b` of struct `xc::B` is private + + z.0; + z.1; //~ ERROR: field `1` of struct `inner::Z` is private } fn main() {} diff --git a/src/test/compile-fail/autoderef-privacy.rs b/src/test/run-pass/autoderef-privacy.rs similarity index 53% rename from src/test/compile-fail/autoderef-privacy.rs rename to src/test/run-pass/autoderef-privacy.rs index 359cee760db1c..e50f1bea0d3ba 100644 --- a/src/test/compile-fail/autoderef-privacy.rs +++ b/src/test/run-pass/autoderef-privacy.rs @@ -10,40 +10,51 @@ // Check we do not select a private method or field when computing autoderefs -#![feature(rustc_attrs)] #![allow(unused)] +#[derive(Default)] pub struct Bar2 { i: i32 } +#[derive(Default)] pub struct Baz2(i32); impl Bar2 { - fn f(&self) {} + fn f(&self) -> bool { true } } mod foo { - pub struct Bar { i: i32 } - pub struct Baz(i32); + #[derive(Default)] + pub struct Bar { i: ::Bar2 } + #[derive(Default)] + pub struct Baz(::Baz2); impl Bar { - fn f(&self) {} + fn f(&self) -> bool { false } } impl ::std::ops::Deref for Bar { type Target = ::Bar2; - fn deref(&self) -> &::Bar2 { unimplemented!() } + fn deref(&self) -> &::Bar2 { &self.i } } impl ::std::ops::Deref for Baz { type Target = ::Baz2; - fn deref(&self) -> &::Baz2 { unimplemented!() } + fn deref(&self) -> &::Baz2 { &self.0 } } -} -fn f(bar: foo::Bar, baz: foo::Baz) { - let _ = bar.i; - let _ = baz.0; - let _ = bar.f(); + pub fn f(bar: &Bar, baz: &Baz) { + // Since the private fields and methods are visible here, there should be no autoderefs. + let _: &::Bar2 = &bar.i; + let _: &::Baz2 = &baz.0; + assert!(!bar.f()); + } } -#[rustc_error] -fn main() {} //~ ERROR compilation successful +fn main() { + let bar = foo::Bar::default(); + let baz = foo::Baz::default(); + foo::f(&bar, &baz); + + let _: i32 = bar.i; + let _: i32 = baz.0; + assert!(bar.f()); +}