Skip to content

Commit 3520402

Browse files
jseyfriedjseyfried
committed
Improve PrivateItemsInPublicInterfacesVisitor 
1 parent 0d34c5d commit 3520402

File tree

2 files changed

+109
-85
lines changed

2 files changed

+109
-85
lines changed

‎src/librustc/ty/mod.rs

+12-1
Original file line numberDiff line numberDiff line change
@@ -302,14 +302,25 @@ impl Visibility {
302302
Visibility::Restricted(module) => module,
303303
};
304304

305-
let mut block_ancestor = map.get_module_parent(block);
305+
let mut block_ancestor = block;
306306
loop {
307307
if block_ancestor == restriction { return true }
308308
let block_ancestor_parent = map.get_module_parent(block_ancestor);
309309
if block_ancestor_parent == block_ancestor { return false }
310310
block_ancestor = block_ancestor_parent;
311311
}
312312
}
313+
314+
/// Returns true if this visibility is at least as accessible as the given visibility
315+
pub fn is_at_least(self, vis: Visibility, map: &ast_map::Map) -> bool {
316+
let vis_restriction = match vis {
317+
Visibility::Public => return self == Visibility::Public,
318+
Visibility::PrivateExternal => return true,
319+
Visibility::Restricted(module) => module,
320+
};
321+
322+
self.is_accessible_from(vis_restriction, map)
323+
}
313324
}
314325

315326
#[derive(Clone, Debug)]

‎src/librustc_privacy/lib.rs

+97-84
Original file line numberDiff line numberDiff line change
@@ -936,27 +936,41 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx>
936936

937937
struct SearchInterfaceForPrivateItemsVisitor<'a, 'tcx: 'a> {
938938
tcx: &'a TyCtxt<'tcx>,
939-
// Do not report an error when a private type is found
940-
is_quiet: bool,
941-
// Is private component found?
942-
is_public: bool,
939+
/// The visitor checks that each component type is at least this visible
940+
required_visibility: ty::Visibility,
941+
/// The visibility of the least visible component that has been visited
942+
min_visibility: ty::Visibility,
943943
old_error_set: &'a NodeSet,
944944
}
945945

946946
impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
947-
// Check if the type alias contain private types when substituted
948-
fn is_public_type_alias(&self, item: &hir::Item, path: &hir::Path) -> bool {
947+
fn new(tcx: &'a TyCtxt<'tcx>, old_error_set: &'a NodeSet) -> Self {
948+
SearchInterfaceForPrivateItemsVisitor {
949+
tcx: tcx,
950+
min_visibility: ty::Visibility::Public,
951+
required_visibility: ty::Visibility::PrivateExternal,
952+
old_error_set: old_error_set,
953+
}
954+
}
955+
}
956+
957+
impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
958+
// Return the visibility of the type alias's least visible component type when substituted
959+
fn substituted_alias_visibility(&self, item: &hir::Item, path: &hir::Path)
960+
-> Option<ty::Visibility> {
949961
// We substitute type aliases only when determining impl publicity
950962
// FIXME: This will probably change and all type aliases will be substituted,
951963
// requires an amendment to RFC 136.
952-
if !self.is_quiet {
953-
return false
964+
if self.required_visibility != ty::Visibility::PrivateExternal {
965+
return None;
954966
}
955967
// Type alias is considered public if the aliased type is
956968
// public, even if the type alias itself is private. So, something
957969
// like `type A = u8; pub fn f() -> A {...}` doesn't cause an error.
958970
if let hir::ItemTy(ref ty, ref generics) = item.node {
959-
let mut check = SearchInterfaceForPrivateItemsVisitor { is_public: true, ..*self };
971+
let mut check = SearchInterfaceForPrivateItemsVisitor {
972+
min_visibility: ty::Visibility::Public, ..*self
973+
};
960974
check.visit_ty(ty);
961975
// If a private type alias with default type parameters is used in public
962976
// interface we must ensure, that the defaults are public if they are actually used.
@@ -970,26 +984,23 @@ impl<'a, 'tcx: 'a> SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
970984
check.visit_ty(default_ty);
971985
}
972986
}
973-
check.is_public
987+
Some(check.min_visibility)
974988
} else {
975-
false
989+
None
976990
}
977991
}
978992
}
979993

980994
impl<'a, 'tcx: 'a, 'v> Visitor<'v> for SearchInterfaceForPrivateItemsVisitor<'a, 'tcx> {
981995
fn visit_ty(&mut self, ty: &hir::Ty) {
982-
if self.is_quiet && !self.is_public {
983-
// We are in quiet mode and a private type is already found, no need to proceed
984-
return
985-
}
986996
if let hir::TyPath(_, ref path) = ty.node {
987997
let def = self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def();
988998
match def {
989999
Def::PrimTy(..) | Def::SelfTy(..) | Def::TyParam(..) => {
9901000
// Public
9911001
}
992-
Def::AssociatedTy(..) if self.is_quiet => {
1002+
Def::AssociatedTy(..)
1003+
if self.required_visibility == ty::Visibility::PrivateExternal => {
9931004
// Conservatively approximate the whole type alias as public without
9941005
// recursing into its components when determining impl publicity.
9951006
// For example, `impl <Type as Trait>::Alias {...}` may be a public impl
@@ -1003,21 +1014,24 @@ impl<'a, 'tcx: 'a, 'v> Visitor<'v> for SearchInterfaceForPrivateItemsVisitor<'a,
10031014
// Non-local means public (private items can't leave their crate, modulo bugs)
10041015
if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
10051016
let item = self.tcx.map.expect_item(node_id);
1006-
if item.vis != hir::Public && !self.is_public_type_alias(item, path) {
1007-
if !self.is_quiet {
1008-
if self.old_error_set.contains(&ty.id) {
1009-
span_err!(self.tcx.sess, ty.span, E0446,
1010-
"private type in public interface");
1011-
} else {
1012-
self.tcx.sess.add_lint (
1013-
lint::builtin::PRIVATE_IN_PUBLIC,
1014-
node_id,
1015-
ty.span,
1016-
format!("private type in public interface"),
1017-
);
1018-
}
1017+
let vis = match self.substituted_alias_visibility(item, path) {
1018+
Some(vis) => vis,
1019+
None => ty::Visibility::from_hir(&item.vis, node_id, &self.tcx),
1020+
};
1021+
1022+
if !vis.is_at_least(self.min_visibility, &self.tcx.map) {
1023+
self.min_visibility = vis;
1024+
}
1025+
if !vis.is_at_least(self.required_visibility, &self.tcx.map) {
1026+
if self.old_error_set.contains(&ty.id) {
1027+
span_err!(self.tcx.sess, ty.span, E0446,
1028+
"private type in public interface");
1029+
} else {
1030+
self.tcx.sess.add_lint(lint::builtin::PRIVATE_IN_PUBLIC,
1031+
node_id,
1032+
ty.span,
1033+
format!("private type in public interface"));
10191034
}
1020-
self.is_public = false;
10211035
}
10221036
}
10231037
}
@@ -1029,28 +1043,26 @@ impl<'a, 'tcx: 'a, 'v> Visitor<'v> for SearchInterfaceForPrivateItemsVisitor<'a,
10291043
}
10301044

10311045
fn visit_trait_ref(&mut self, trait_ref: &hir::TraitRef) {
1032-
if self.is_quiet && !self.is_public {
1033-
// We are in quiet mode and a private type is already found, no need to proceed
1034-
return
1035-
}
10361046
// Non-local means public (private items can't leave their crate, modulo bugs)
10371047
let def_id = self.tcx.trait_ref_to_def_id(trait_ref);
10381048
if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
10391049
let item = self.tcx.map.expect_item(node_id);
1040-
if item.vis != hir::Public {
1041-
if !self.is_quiet {
1042-
if self.old_error_set.contains(&trait_ref.ref_id) {
1043-
span_err!(self.tcx.sess, trait_ref.path.span, E0445,
1044-
"private trait in public interface");
1045-
} else {
1046-
self.tcx.sess.add_lint(lint::builtin::PRIVATE_IN_PUBLIC,
1047-
node_id,
1048-
trait_ref.path.span,
1049-
"private trait in public interface (error E0445)"
1050-
.to_string());
1051-
}
1050+
let vis = ty::Visibility::from_hir(&item.vis, node_id, &self.tcx);
1051+
1052+
if !vis.is_at_least(self.min_visibility, &self.tcx.map) {
1053+
self.min_visibility = vis;
1054+
}
1055+
if !vis.is_at_least(self.required_visibility, &self.tcx.map) {
1056+
if self.old_error_set.contains(&trait_ref.ref_id) {
1057+
span_err!(self.tcx.sess, trait_ref.path.span, E0445,
1058+
"private trait in public interface");
1059+
} else {
1060+
self.tcx.sess.add_lint(lint::builtin::PRIVATE_IN_PUBLIC,
1061+
node_id,
1062+
trait_ref.path.span,
1063+
"private trait in public interface (error E0445)"
1064+
.to_string());
10521065
}
1053-
self.is_public = false;
10541066
}
10551067
}
10561068

@@ -1072,29 +1084,29 @@ struct PrivateItemsInPublicInterfacesVisitor<'a, 'tcx: 'a> {
10721084

10731085
impl<'a, 'tcx> PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {
10741086
// A type is considered public if it doesn't contain any private components
1075-
fn is_public_ty(&self, ty: &hir::Ty) -> bool {
1076-
let mut check = SearchInterfaceForPrivateItemsVisitor {
1077-
tcx: self.tcx, is_quiet: true, is_public: true, old_error_set: self.old_error_set
1078-
};
1087+
fn ty_visibility(&self, ty: &hir::Ty) -> ty::Visibility {
1088+
let mut check = SearchInterfaceForPrivateItemsVisitor::new(self.tcx, self.old_error_set);
10791089
check.visit_ty(ty);
1080-
check.is_public
1090+
check.min_visibility
10811091
}
10821092

10831093
// A trait reference is considered public if it doesn't contain any private components
1084-
fn is_public_trait_ref(&self, trait_ref: &hir::TraitRef) -> bool {
1085-
let mut check = SearchInterfaceForPrivateItemsVisitor {
1086-
tcx: self.tcx, is_quiet: true, is_public: true, old_error_set: self.old_error_set
1087-
};
1094+
fn trait_ref_visibility(&self, trait_ref: &hir::TraitRef) -> ty::Visibility {
1095+
let mut check = SearchInterfaceForPrivateItemsVisitor::new(self.tcx, self.old_error_set);
10881096
check.visit_trait_ref(trait_ref);
1089-
check.is_public
1097+
check.min_visibility
10901098
}
10911099
}
10921100

10931101
impl<'a, 'tcx, 'v> Visitor<'v> for PrivateItemsInPublicInterfacesVisitor<'a, 'tcx> {
10941102
fn visit_item(&mut self, item: &hir::Item) {
1095-
let mut check = SearchInterfaceForPrivateItemsVisitor {
1096-
tcx: self.tcx, is_quiet: false, is_public: true, old_error_set: self.old_error_set
1103+
let min = |vis1: ty::Visibility, vis2| {
1104+
if vis1.is_at_least(vis2, &self.tcx.map) { vis2 } else { vis1 }
10971105
};
1106+
1107+
let mut check = SearchInterfaceForPrivateItemsVisitor::new(self.tcx, self.old_error_set);
1108+
let item_visibility = ty::Visibility::from_hir(&item.vis, item.id, &self.tcx);
1109+
10981110
match item.node {
10991111
// Crates are always public
11001112
hir::ItemExternCrate(..) => {}
@@ -1105,51 +1117,52 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivateItemsInPublicInterfacesVisitor<'a, 'tc
11051117
// Subitems of these items have inherited publicity
11061118
hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
11071119
hir::ItemEnum(..) | hir::ItemTrait(..) | hir::ItemTy(..) => {
1108-
if item.vis == hir::Public {
1109-
check.visit_item(item);
1110-
}
1120+
check.required_visibility = item_visibility;
1121+
check.visit_item(item);
11111122
}
11121123
// Subitems of foreign modules have their own publicity
11131124
hir::ItemForeignMod(ref foreign_mod) => {
11141125
for foreign_item in &foreign_mod.items {
1115-
if foreign_item.vis == hir::Public {
1116-
check.visit_foreign_item(foreign_item);
1117-
}
1126+
check.required_visibility =
1127+
ty::Visibility::from_hir(&foreign_item.vis, item.id, &self.tcx);
1128+
check.visit_foreign_item(foreign_item);
11181129
}
11191130
}
11201131
// Subitems of structs have their own publicity
11211132
hir::ItemStruct(ref struct_def, ref generics) => {
1122-
if item.vis == hir::Public {
1123-
check.visit_generics(generics);
1124-
for field in struct_def.fields() {
1125-
if field.vis == hir::Public {
1126-
check.visit_struct_field(field);
1127-
}
1128-
}
1133+
check.required_visibility = item_visibility;
1134+
check.visit_generics(generics);
1135+
1136+
for field in struct_def.fields() {
1137+
let field_visibility = ty::Visibility::from_hir(&field.vis, item.id, &self.tcx);
1138+
check.required_visibility = min(item_visibility, field_visibility);
1139+
check.visit_struct_field(field);
11291140
}
11301141
}
11311142
// The interface is empty
11321143
hir::ItemDefaultImpl(..) => {}
11331144
// An inherent impl is public when its type is public
11341145
// Subitems of inherent impls have their own publicity
11351146
hir::ItemImpl(_, _, ref generics, None, ref ty, ref impl_items) => {
1136-
if self.is_public_ty(ty) {
1137-
check.visit_generics(generics);
1138-
for impl_item in impl_items {
1139-
if impl_item.vis == hir::Public {
1140-
check.visit_impl_item(impl_item);
1141-
}
1142-
}
1147+
let ty_vis = self.ty_visibility(ty);
1148+
check.required_visibility = ty_vis;
1149+
check.visit_generics(generics);
1150+
1151+
for impl_item in impl_items {
1152+
let impl_item_vis =
1153+
ty::Visibility::from_hir(&impl_item.vis, item.id, &self.tcx);
1154+
check.required_visibility = min(impl_item_vis, ty_vis);
1155+
check.visit_impl_item(impl_item);
11431156
}
11441157
}
11451158
// A trait impl is public when both its type and its trait are public
11461159
// Subitems of trait impls have inherited publicity
11471160
hir::ItemImpl(_, _, ref generics, Some(ref trait_ref), ref ty, ref impl_items) => {
1148-
if self.is_public_ty(ty) && self.is_public_trait_ref(trait_ref) {
1149-
check.visit_generics(generics);
1150-
for impl_item in impl_items {
1151-
check.visit_impl_item(impl_item);
1152-
}
1161+
let vis = min(self.ty_visibility(ty), self.trait_ref_visibility(trait_ref));
1162+
check.required_visibility = vis;
1163+
check.visit_generics(generics);
1164+
for impl_item in impl_items {
1165+
check.visit_impl_item(impl_item);
11531166
}
11541167
}
11551168
}

0 commit comments

Comments
 (0)