Skip to content

Commit

Permalink
Auto merge of #41332 - petrochenkov:privti, r=eddyb
Browse files Browse the repository at this point in the history 
Check privacy of trait items in all contexts

Fixes #28514

This is a sufficiently rare scenario and it's currently guarded by `private_in_public` lint, so it shouldn't be a [breaking-change] in practice.
  • Loading branch information
@bors
bors committed Apr 25, 2017
2 parents 2b4c911 + 4bd417e commit 7e7114f
Show file tree
Hide file tree
Showing 10 changed files with 227 additions and 248 deletions.
25 changes: 11 additions & 14 deletions src/librustc/ty/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2156,6 +2156,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {

fn associated_item_from_trait_item_ref(self,
parent_def_id: DefId,
parent_vis: &hir::Visibility,
trait_item_ref: &hir::TraitItemRef)
-> AssociatedItem {
let def_id = self.hir.local_def_id(trait_item_ref.id.node_id);
Expand All @@ -2170,7 +2171,8 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
AssociatedItem {
name: trait_item_ref.name,
kind: kind,
vis: Visibility::from_hir(&hir::Inherited, trait_item_ref.id.node_id, self),
// Visibility of trait items is inherited from their traits.
vis: Visibility::from_hir(parent_vis, trait_item_ref.id.node_id, self),
defaultness: trait_item_ref.defaultness,
def_id: def_id,
container: TraitContainer(parent_def_id),
Expand All @@ -2180,7 +2182,6 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {

fn associated_item_from_impl_item_ref(self,
parent_def_id: DefId,
from_trait_impl: bool,
impl_item_ref: &hir::ImplItemRef)
-> AssociatedItem {
let def_id = self.hir.local_def_id(impl_item_ref.id.node_id);
Expand All @@ -2192,14 +2193,11 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
hir::AssociatedItemKind::Type => (ty::AssociatedKind::Type, false),
};

// Trait impl items are always public.
let public = hir::Public;
let vis = if from_trait_impl { &public } else { &impl_item_ref.vis };

ty::AssociatedItem {
name: impl_item_ref.name,
kind: kind,
vis: ty::Visibility::from_hir(vis, impl_item_ref.id.node_id, self),
// Visibility of trait impl items doesn't matter.
vis: ty::Visibility::from_hir(&impl_item_ref.vis, impl_item_ref.id.node_id, self),
defaultness: impl_item_ref.defaultness,
def_id: def_id,
container: ImplContainer(parent_def_id),
Expand Down Expand Up @@ -2639,21 +2637,20 @@ fn associated_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
let parent_def_id = tcx.hir.local_def_id(parent_id);
let parent_item = tcx.hir.expect_item(parent_id);
match parent_item.node {
hir::ItemImpl(.., ref impl_trait_ref, _, ref impl_item_refs) => {
hir::ItemImpl(.., ref impl_item_refs) => {
if let Some(impl_item_ref) = impl_item_refs.iter().find(|i| i.id.node_id == id) {
let assoc_item =
tcx.associated_item_from_impl_item_ref(parent_def_id,
impl_trait_ref.is_some(),
impl_item_ref);
let assoc_item = tcx.associated_item_from_impl_item_ref(parent_def_id,
impl_item_ref);
debug_assert_eq!(assoc_item.def_id, def_id);
return assoc_item;
}
}

hir::ItemTrait(.., ref trait_item_refs) => {
if let Some(trait_item_ref) = trait_item_refs.iter().find(|i| i.id.node_id == id) {
let assoc_item =
tcx.associated_item_from_trait_item_ref(parent_def_id, trait_item_ref);
let assoc_item = tcx.associated_item_from_trait_item_ref(parent_def_id,
&parent_item.vis,
trait_item_ref);
debug_assert_eq!(assoc_item.def_id, def_id);
return assoc_item;
}
Expand Down
135 changes: 38 additions & 97 deletions src/librustc_privacy/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
#![crate_type = "dylib"]
#![crate_type = "rlib"]
#![doc(html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
html_root_url = "https://doc.rust-lang.org/nightly/")]
html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
html_root_url = "https://doc.rust-lang.org/nightly/")]
#![deny(warnings)]

#![feature(rustc_diagnostic_macros)]
Expand All @@ -30,7 +30,6 @@ use rustc::hir::def::Def;
use rustc::hir::def_id::{CRATE_DEF_INDEX, LOCAL_CRATE, CrateNum, DefId};
use rustc::hir::intravisit::{self, Visitor, NestedVisitorMap};
use rustc::hir::itemlikevisit::DeepVisitor;
use rustc::hir::pat_util::EnumerateAndAdjustIterator;
use rustc::lint;
use rustc::middle::privacy::{AccessLevel, AccessLevels};
use rustc::ty::{self, TyCtxt, Ty, TypeFoldable};
Expand Down Expand Up @@ -415,97 +414,69 @@ impl<'b, 'a, 'tcx> TypeVisitor<'tcx> for ReachEverythingInTheInterfaceVisitor<'b
}
}

////////////////////////////////////////////////////////////////////////////////
/// The privacy visitor, where privacy checks take place (violations reported)
////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////
/// Name privacy visitor, checks privacy and reports violations.
/// Most of name privacy checks are performed during the main resolution phase,
/// or later in type checking when field accesses and associated items are resolved.
/// This pass performs remaining checks for fields in struct expressions and patterns.
//////////////////////////////////////////////////////////////////////////////////////

struct PrivacyVisitor<'a, 'tcx: 'a> {
struct NamePrivacyVisitor<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
curitem: DefId,
in_foreign: bool,
tables: &'a ty::TypeckTables<'tcx>,
current_item: DefId,
}

impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
fn item_is_accessible(&self, did: DefId) -> bool {
match self.tcx.hir.as_local_node_id(did) {
Some(node_id) =>
ty::Visibility::from_hir(&self.tcx.hir.expect_item(node_id).vis, node_id, self.tcx),
None => self.tcx.sess.cstore.visibility(did),
}.is_accessible_from(self.curitem, self.tcx)
}

// Checks that a field is in scope.
impl<'a, 'tcx> NamePrivacyVisitor<'a, 'tcx> {
// Checks that a field is accessible.
fn check_field(&mut self, span: Span, def: &'tcx ty::AdtDef, field: &'tcx ty::FieldDef) {
if !def.is_enum() && !field.vis.is_accessible_from(self.curitem, self.tcx) {
if !def.is_enum() && !field.vis.is_accessible_from(self.current_item, self.tcx) {
struct_span_err!(self.tcx.sess, span, E0451, "field `{}` of {} `{}` is private",
field.name, def.variant_descr(), self.tcx.item_path_str(def.did))
field.name, def.variant_descr(), self.tcx.item_path_str(def.did))
.span_label(span, &format!("field `{}` is private", field.name))
.emit();
}
}

// Checks that a method is in scope.
fn check_method(&mut self, span: Span, method_def_id: DefId) {
match self.tcx.associated_item(method_def_id).container {
// Trait methods are always all public. The only controlling factor
// is whether the trait itself is accessible or not.
ty::TraitContainer(trait_def_id) if !self.item_is_accessible(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);
}
_ => {}
}
}
}

impl<'a, 'tcx> Visitor<'tcx> for PrivacyVisitor<'a, 'tcx> {
impl<'a, 'tcx> Visitor<'tcx> for NamePrivacyVisitor<'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 nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::All(&self.tcx.hir)
}

fn visit_nested_body(&mut self, body: hir::BodyId) {
let old_tables = self.tables;
self.tables = self.tcx.body_tables(body);
let orig_tables = replace(&mut self.tables, self.tcx.body_tables(body));
let body = self.tcx.hir.body(body);
self.visit_body(body);
self.tables = old_tables;
self.tables = orig_tables;
}

fn visit_item(&mut self, item: &'tcx hir::Item) {
let orig_curitem = replace(&mut self.curitem, self.tcx.hir.local_def_id(item.id));
let orig_current_item = replace(&mut self.current_item, self.tcx.hir.local_def_id(item.id));
intravisit::walk_item(self, item);
self.curitem = orig_curitem;
self.current_item = orig_current_item;
}

fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
match expr.node {
hir::ExprMethodCall(..) => {
let method_call = ty::MethodCall::expr(expr.id);
let method = self.tables.method_map[&method_call];
self.check_method(expr.span, method.def_id);
}
hir::ExprStruct(ref qpath, ref expr_fields, _) => {
hir::ExprStruct(ref qpath, ref fields, ref base) => {
let def = self.tables.qpath_def(qpath, expr.id);
let adt = self.tables.expr_ty(expr).ty_adt_def().unwrap();
let variant = adt.variant_of_def(def);
// RFC 736: ensure all unmentioned fields are visible.
// Rather than computing the set of unmentioned fields
// (i.e. `all_fields - fields`), just check them all,
// unless the ADT is a union, then unmentioned fields
// are not checked.
if adt.is_union() {
for expr_field in expr_fields {
self.check_field(expr.span, adt, variant.field_named(expr_field.name.node));
if let Some(ref base) = *base {
// If the expression uses FRU we need to make sure all the unmentioned fields
// are checked for privacy (RFC 736). Rather than computing the set of
// unmentioned fields, just check them all.
for variant_field in &variant.fields {
let field = fields.iter().find(|f| f.name.node == variant_field.name);
let span = if let Some(f) = field { f.span } else { base.span };
self.check_field(span, adt, variant_field);
}
} else {
for field in &variant.fields {
let expr_field = expr_fields.iter().find(|f| f.name.node == field.name);
let span = if let Some(f) = expr_field { f.span } else { expr.span };
self.check_field(span, adt, field);
for field in fields {
self.check_field(field.span, adt, variant.field_named(field.name.node));
}
}
}
Expand All @@ -515,47 +486,20 @@ impl<'a, 'tcx> Visitor<'tcx> for PrivacyVisitor<'a, 'tcx> {
intravisit::walk_expr(self, expr);
}

fn visit_pat(&mut self, pattern: &'tcx hir::Pat) {
// Foreign functions do not have their patterns mapped in the def_map,
// and there's nothing really relevant there anyway, so don't bother
// checking privacy. If you can name the type then you can pass it to an
// external C function anyway.
if self.in_foreign { return }

match pattern.node {
fn visit_pat(&mut self, pat: &'tcx hir::Pat) {
match pat.node {
PatKind::Struct(ref qpath, ref fields, _) => {
let def = self.tables.qpath_def(qpath, pattern.id);
let adt = self.tables.pat_ty(pattern).ty_adt_def().unwrap();
let def = self.tables.qpath_def(qpath, pat.id);
let adt = self.tables.pat_ty(pat).ty_adt_def().unwrap();
let variant = adt.variant_of_def(def);
for field in fields {
self.check_field(field.span, adt, variant.field_named(field.node.name));
}
}
PatKind::TupleStruct(_, ref fields, ddpos) => {
match self.tables.pat_ty(pattern).sty {
// enum fields have no privacy at this time
ty::TyAdt(def, _) if !def.is_enum() => {
let expected_len = def.struct_variant().fields.len();
for (i, field) in fields.iter().enumerate_and_adjust(expected_len, ddpos) {
if let PatKind::Wild = field.node {
continue
}
self.check_field(field.span, def, &def.struct_variant().fields[i]);
}
}
_ => {}
}
}
_ => {}
}

intravisit::walk_pat(self, pattern);
}

fn visit_foreign_item(&mut self, fi: &'tcx hir::ForeignItem) {
self.in_foreign = true;
intravisit::walk_foreign_item(self, fi);
self.in_foreign = false;
intravisit::walk_pat(self, pat);
}
}

Expand Down Expand Up @@ -1233,17 +1177,14 @@ fn privacy_access_levels<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

let krate = tcx.hir.krate();

// Use the parent map to check the privacy of everything
let mut visitor = PrivacyVisitor {
curitem: DefId::local(CRATE_DEF_INDEX),
in_foreign: false,
// Check privacy of names not checked in previous compilation stages.
let mut visitor = NamePrivacyVisitor {
tcx: tcx,
tables: &ty::TypeckTables::empty(),
current_item: DefId::local(CRATE_DEF_INDEX),
};
intravisit::walk_crate(&mut visitor, krate);

tcx.sess.abort_if_errors();

// Build up a set of all exported items in the AST. This is a set of all
// items which are reachable from external crates based on visibility.
let mut visitor = EmbargoVisitor {
Expand Down
14 changes: 10 additions & 4 deletions src/librustc_typeck/astconv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -903,10 +903,16 @@ impl<'o, 'gcx: 'tcx, 'tcx> AstConv<'gcx, 'tcx>+'o {
let ty = self.projected_ty_from_poly_trait_ref(span, bound, assoc_name);
let ty = self.normalize_ty(span, ty);

let item = tcx.associated_items(trait_did).find(|i| i.name == assoc_name);
let def_id = item.expect("missing associated type").def_id;
tcx.check_stability(def_id, ref_id, span);
(ty, Def::AssociatedTy(def_id))
let item = tcx.associated_items(trait_did).find(|i| i.name == assoc_name)
.expect("missing associated type");
let def = Def::AssociatedTy(item.def_id);
if !tcx.vis_is_accessible_from(item.vis, ref_id) {
let msg = format!("{} `{}` is private", def.kind_name(), assoc_name);
tcx.sess.span_err(span, &msg);
}
tcx.check_stability(item.def_id, ref_id, span);

(ty, def)
}

fn qpath_to_ty(&self,
Expand Down
7 changes: 0 additions & 7 deletions src/librustc_typeck/check/method/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,15 +349,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}

let def = pick.item.def();

self.tcx.check_stability(def.def_id(), expr_id, span);

if let probe::InherentImplPick = pick.kind {
if !self.tcx.vis_is_accessible_from(pick.item.vis, self.body_id) {
let msg = format!("{} `{}` is private", def.kind_name(), method_name);
self.tcx.sess.span_err(span, &msg);
}
}
Ok(def)
}

Expand Down
Loading

0 comments on commit 7e7114f

Please sign in to comment.