Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

privacy: Mark reachable but unnameable items as reachable #31641

Merged
merged 3 commits into from Feb 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
145 changes: 134 additions & 11 deletions src/librustc_privacy/lib.rs
Expand Up @@ -169,6 +169,10 @@ struct EmbargoVisitor<'a, 'tcx: 'a> {
changed: bool,
}

struct ReachEverythingInTheInterfaceVisitor<'b, 'a: 'b, 'tcx: 'a> {
ev: &'b mut EmbargoVisitor<'a, 'tcx>,
}

impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> {
fn ty_level(&self, ty: &hir::Ty) -> Option<AccessLevel> {
if let hir::TyPath(..) = ty.node {
Expand Down Expand Up @@ -214,6 +218,10 @@ impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> {
old_level
}
}

fn reach<'b>(&'b mut self) -> ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
ReachEverythingInTheInterfaceVisitor { ev: self }
}
}

impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
Expand Down Expand Up @@ -245,10 +253,10 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
}
};

// Update id of the item itself
// Update level of the item itself
let item_level = self.update(item.id, inherited_item_level);

// Update ids of nested things
// Update levels of nested things
match item.node {
hir::ItemEnum(ref def, _) => {
for variant in &def.variants {
Expand Down Expand Up @@ -292,19 +300,72 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
}
}
}
hir::ItemTy(ref ty, _) if item_level.is_some() => {
if let hir::TyPath(..) = ty.node {
match self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def() {
Def::PrimTy(..) | Def::SelfTy(..) | Def::TyParam(..) => {},
def => {
if let Some(node_id) = self.tcx.map.as_local_node_id(def.def_id()) {
self.update(node_id, Some(AccessLevel::Reachable));
}
_ => {}
}

// Mark all items in interfaces of reachable items as reachable
match item.node {
// The interface is empty
hir::ItemExternCrate(..) => {}
// All nested items are checked by visit_item
hir::ItemMod(..) => {}
// Reexports are handled in visit_mod
hir::ItemUse(..) => {}
// Visit everything
hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
hir::ItemTrait(..) | hir::ItemTy(..) | hir::ItemImpl(_, _, _, Some(..), _, _) => {
if item_level.is_some() {
self.reach().visit_item(item);
}
}
// Visit everything, but enum variants have their own levels
hir::ItemEnum(ref def, ref generics) => {
if item_level.is_some() {
self.reach().visit_generics(generics);
}
for variant in &def.variants {
if self.get(variant.node.data.id()).is_some() {
for field in variant.node.data.fields() {
self.reach().visit_struct_field(field);
}
// Corner case: if the variant is reachable, but its
// enum is not, make the enum reachable as well.
self.update(item.id, Some(AccessLevel::Reachable));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that you should go back and call visit_generics too (i.e., as would have happened on line 323?) If so, maybe we should promote this special case upward somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we have a regression test for this scenario? (Seems like a good one). If so, you should cite it in the comment here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see this scenario is covered in the test you added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that you should go back and call visit_generics too (i.e., as would have happened on line 323?)

The whole EmbargoVisitor::visit_crate runs in a loop until it saturates and nothing is updated anymore (yeah, it should probably be optimized into some DFS or BFS, but it's a separate task).
So, the worst thing can happen here is one more loop cycle in exceedingly rare case of unnameable enum with nameable variant.

}
}
}
// Visit everything, but foreign items have their own levels
hir::ItemForeignMod(ref foreign_mod) => {
for foreign_item in &foreign_mod.items {
if self.get(foreign_item.id).is_some() {
self.reach().visit_foreign_item(foreign_item);
}
}
}
// Visit everything except for private fields
hir::ItemStruct(ref struct_def, ref generics) => {
if item_level.is_some() {
self.reach().visit_generics(generics);
for field in struct_def.fields() {
if self.get(field.node.id).is_some() {
self.reach().visit_struct_field(field);
}
}
}
}
// The interface is empty
hir::ItemDefaultImpl(..) => {}
// Visit everything except for private impl items
hir::ItemImpl(_, _, ref generics, None, _, ref impl_items) => {
if item_level.is_some() {
self.reach().visit_generics(generics);
for impl_item in impl_items {
if self.get(impl_item.id).is_some() {
self.reach().visit_impl_item(impl_item);
}
}
}
}
_ => {}
}

let orig_level = self.prev_level;
Expand Down Expand Up @@ -347,6 +408,68 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
}
}

impl<'b, 'a, 'tcx: 'a> ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
// Make the type hidden under a type alias reachable
fn reach_aliased_type(&mut self, item: &hir::Item, path: &hir::Path) {
if let hir::ItemTy(ref ty, ref generics) = item.node {
// See `fn is_public_type_alias` for details
self.visit_ty(ty);
let provided_params = path.segments.last().unwrap().parameters.types().len();
for ty_param in &generics.ty_params[provided_params..] {
if let Some(ref default_ty) = ty_param.default {
self.visit_ty(default_ty);
}
}
}
}
}

impl<'b, 'a, 'tcx: 'a, 'v> Visitor<'v> for ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
fn visit_ty(&mut self, ty: &hir::Ty) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so another thing I'm a little confused about is why two visitors are needed? It looks like the EmbargoVisitor should walk the entire AST, right? In that case, couldn't this just be part of that?

I guess to me this algorithm should look like:

  1. Initialize a reachable set with all public items
  2. While the set has an item in it:
    1. Walk the reachable interface of the item
    2. For each item encountered, add it to the global reachable

This is then repeated iteratively until the set is saturated. There's only one "recurse the interface" step, however, so I'm not entirely sure where this second visitor fits in?

I may and probably am missing something though! Could you help clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR merely extends the existing pretty inefficient implementation of EmbargoVisito which runs walk_crate several times in a loop without any working set and graph-like traversal. I plan to rewrite it eventually, but don't have time to do it right now.

Regardless of that, having two visitors with different purposes is just convenient. One big visitor walks items and recurses into them, another small and shallow visitor walks item's surfaces without going too deep. Private-in-public visitor has the same separation of concerns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that sounds good to me, thanks for the clarification!

if let hir::TyPath(_, ref path) = ty.node {
let def = self.ev.tcx.def_map.borrow().get(&ty.id).unwrap().full_def();
match def {
Def::Struct(def_id) | Def::Enum(def_id) | Def::TyAlias(def_id) |
Def::Trait(def_id) | Def::AssociatedTy(def_id, _) => {
if let Some(node_id) = self.ev.tcx.map.as_local_node_id(def_id) {
let item = self.ev.tcx.map.expect_item(node_id);
if let Def::TyAlias(..) = def {
// Type aliases are substituted. Associated type aliases are not
// substituted yet, but ideally they should be.
if self.ev.get(item.id).is_none() {
self.reach_aliased_type(item, path);
}
} else {
self.ev.update(item.id, Some(AccessLevel::Reachable));
}
}
}

_ => {}
}
}

intravisit::walk_ty(self, ty);
}

fn visit_trait_ref(&mut self, trait_ref: &hir::TraitRef) {
let def_id = self.ev.tcx.trait_ref_to_def_id(trait_ref);
if let Some(node_id) = self.ev.tcx.map.as_local_node_id(def_id) {
let item = self.ev.tcx.map.expect_item(node_id);
self.ev.update(item.id, Some(AccessLevel::Reachable));
}

intravisit::walk_trait_ref(self, trait_ref);
}

// Don't recurse into function bodies
fn visit_block(&mut self, _: &hir::Block) {}
// Don't recurse into expressions in array sizes or const initializers
fn visit_expr(&mut self, _: &hir::Expr) {}
// Don't recurse into patterns in function arguments
fn visit_pat(&mut self, _: &hir::Pat) {}
}

////////////////////////////////////////////////////////////////////////////////
/// The privacy visitor, where privacy checks take place (violations reported)
////////////////////////////////////////////////////////////////////////////////
Expand Down
116 changes: 116 additions & 0 deletions src/test/auxiliary/reachable-unnameable-items.rs
@@ -0,0 +1,116 @@
// 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use inner_private_module::*;

mod inner_private_module {
pub struct Unnameable1;
pub struct Unnameable2;
#[derive(Clone, Copy)]
pub struct Unnameable3;
pub struct Unnameable4;
pub struct Unnameable5;
pub struct Unnameable6;
pub struct Unnameable7;
#[derive(Default)]
pub struct Unnameable8;
pub enum UnnameableEnum {
NameableVariant
}
pub trait UnnameableTrait {
type Alias: Default;
}

impl Unnameable1 {
pub fn method_of_unnameable_type1(&self) -> &'static str {
"Hello1"
}
}
impl Unnameable2 {
pub fn method_of_unnameable_type2(&self) -> &'static str {
"Hello2"
}
}
impl Unnameable3 {
pub fn method_of_unnameable_type3(&self) -> &'static str {
"Hello3"
}
}
impl Unnameable4 {
pub fn method_of_unnameable_type4(&self) -> &'static str {
"Hello4"
}
}
impl Unnameable5 {
pub fn method_of_unnameable_type5(&self) -> &'static str {
"Hello5"
}
}
impl Unnameable6 {
pub fn method_of_unnameable_type6(&self) -> &'static str {
"Hello6"
}
}
impl Unnameable7 {
pub fn method_of_unnameable_type7(&self) -> &'static str {
"Hello7"
}
}
impl Unnameable8 {
pub fn method_of_unnameable_type8(&self) -> &'static str {
"Hello8"
}
}
impl UnnameableEnum {
pub fn method_of_unnameable_enum(&self) -> &'static str {
"HelloEnum"
}
}
}

pub fn function_returning_unnameable_type() -> Unnameable1 {
Unnameable1
}

pub const CONSTANT_OF_UNNAMEABLE_TYPE: Unnameable2 =
Unnameable2;

pub fn function_accepting_unnameable_type(_: Option<Unnameable3>) {}

pub type AliasOfUnnameableType = Unnameable4;

impl Unnameable1 {
pub fn inherent_method_returning_unnameable_type(&self) -> Unnameable5 {
Unnameable5
}
}

pub trait Tr {
fn trait_method_returning_unnameable_type(&self) -> Unnameable6 {
Unnameable6
}
}
impl Tr for Unnameable1 {}

pub use inner_private_module::UnnameableEnum::NameableVariant;

pub struct Struct {
pub field_of_unnameable_type: Unnameable7
}

pub static STATIC: Struct = Struct { field_of_unnameable_type: Unnameable7 } ;

impl UnnameableTrait for AliasOfUnnameableType {
type Alias = Unnameable8;
}

pub fn generic_function<T: UnnameableTrait>() -> T::Alias {
Default::default()
}
42 changes: 42 additions & 0 deletions src/test/run-pass/reachable-unnameable-items.rs
@@ -0,0 +1,42 @@
// 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// aux-build:reachable-unnameable-items.rs

#![feature(braced_empty_structs)]
#![feature(recover)]

extern crate reachable_unnameable_items;
use reachable_unnameable_items::*;

fn main() {
let res1 = function_returning_unnameable_type().method_of_unnameable_type1();
let res2 = CONSTANT_OF_UNNAMEABLE_TYPE.method_of_unnameable_type2();
let res4 = AliasOfUnnameableType{}.method_of_unnameable_type4();
let res5 = function_returning_unnameable_type().inherent_method_returning_unnameable_type().
method_of_unnameable_type5();
let res6 = function_returning_unnameable_type().trait_method_returning_unnameable_type().
method_of_unnameable_type6();
let res7 = STATIC.field_of_unnameable_type.method_of_unnameable_type7();
let res8 = generic_function::<AliasOfUnnameableType>().method_of_unnameable_type8();
let res_enum = NameableVariant.method_of_unnameable_enum();
assert_eq!(res1, "Hello1");
assert_eq!(res2, "Hello2");
assert_eq!(res4, "Hello4");
assert_eq!(res5, "Hello5");
assert_eq!(res6, "Hello6");
assert_eq!(res7, "Hello7");
assert_eq!(res8, "Hello8");
assert_eq!(res_enum, "HelloEnum");

let none = None;
function_accepting_unnameable_type(none);
let _guard = std::panic::recover(|| none.unwrap().method_of_unnameable_type3());
}
24 changes: 24 additions & 0 deletions src/test/run-pass/reachable-unnameable-type-alias.rs
@@ -0,0 +1,24 @@
// 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(staged_api)]
#![stable(feature = "a", since = "b")]

mod inner_private_module {
// UnnameableTypeAlias isn't marked as reachable, so no stability annotation is required here
pub type UnnameableTypeAlias = u8;
}

#[stable(feature = "a", since = "b")]
pub fn f() -> inner_private_module::UnnameableTypeAlias {
0
}

fn main() {}