Skip to content

Commit

Permalink
auto merge of #9791 : alexcrichton/rust/reachable, r=catamorphism
Browse files Browse the repository at this point in the history 
This fixes a bug in which the visibility rules were approximated by
reachability, but forgot to cover the case where a 'pub use' reexports a private
item. This fixes the commit by instead using the results of the privacy pass of
the compiler to create the initial working set of the reachability pass.

This may have the side effect of increasing the size of metadata, but it's
difficult to avoid for correctness purposes sadly.

Closes #9790
  • Loading branch information
@bors
bors committed Oct 10, 2013
2 parents 0ede2ea + caf7b67 commit 8015f9c
Show file tree
Hide file tree
Showing 15 changed files with 111 additions and 160 deletions.
10 changes: 6 additions & 4 deletions src/librustc/driver/driver.rs
View file
Expand Up @@ -263,9 +263,10 @@ pub fn phase_3_run_analysis_passes(sess: Session,
method_map, ty_cx));

let maps = (external_exports, last_private_map);
time(time_passes, "privacy checking", maps, |(a, b)|
middle::privacy::check_crate(ty_cx, &method_map, &exp_map2,
a, b, crate));
let exported_items =
time(time_passes, "privacy checking", maps, |(a, b)|
middle::privacy::check_crate(ty_cx, &method_map, &exp_map2,
a, b, crate));

time(time_passes, "effect checking", (), |_|
middle::effect::check_crate(ty_cx, method_map, crate));
Expand Down Expand Up @@ -300,7 +301,8 @@ pub fn phase_3_run_analysis_passes(sess: Session,

let reachable_map =
time(time_passes, "reachability checking", (), |_|
reachable::find_reachable(ty_cx, method_map, crate));
reachable::find_reachable(ty_cx, method_map, exp_map2,
&exported_items));

time(time_passes, "lint checking", (), |_|
lint::check_crate(ty_cx, crate));
Expand Down
13 changes: 9 additions & 4 deletions src/librustc/middle/privacy.rs
View file
Expand Up @@ -31,6 +31,10 @@ use syntax::visit::Visitor;

type Context<'self> = (&'self method_map, &'self resolve::ExportMap2);

// A set of all nodes in the ast which can be considered "publicly exported" in
// the sense that they are accessible from anywhere in any hierarchy.
pub type ExportedItems = HashSet<ast::NodeId>;

// This visitor is used to determine the parent of all nodes in question when it
// comes to privacy. This is used to determine later on if a usage is actually
// valid or not.
Expand Down Expand Up @@ -137,7 +141,7 @@ impl<'self> Visitor<()> for ParentVisitor<'self> {
// This visitor is used to determine which items of the ast are embargoed,
// otherwise known as not exported.
struct EmbargoVisitor<'self> {
exported_items: &'self mut HashSet<ast::NodeId>,
exported_items: &'self mut ExportedItems,
exp_map2: &'self resolve::ExportMap2,
path_all_public: bool,
}
Expand Down Expand Up @@ -239,7 +243,7 @@ struct PrivacyVisitor<'self> {
curitem: ast::NodeId,

// Results of previous analyses necessary for privacy checking.
exported_items: &'self HashSet<ast::NodeId>,
exported_items: &'self ExportedItems,
method_map: &'self method_map,
parents: &'self HashMap<ast::NodeId, ast::NodeId>,
external_exports: resolve::ExternalExports,
Expand Down Expand Up @@ -785,7 +789,7 @@ pub fn check_crate(tcx: ty::ctxt,
exp_map2: &resolve::ExportMap2,
external_exports: resolve::ExternalExports,
last_private_map: resolve::LastPrivateMap,
crate: &ast::Crate) {
crate: &ast::Crate) -> ExportedItems {
let mut parents = HashMap::new();
let mut exported_items = HashSet::new();

Expand All @@ -802,7 +806,7 @@ pub fn check_crate(tcx: ty::ctxt,
{
// Initialize the exported items with resolve's id for the "root crate"
// to resolve references to `super` leading to the root and such.
exported_items.insert(0);
exported_items.insert(ast::CRATE_NODE_ID);
let mut visitor = EmbargoVisitor {
exported_items: &mut exported_items,
exp_map2: exp_map2,
Expand All @@ -824,4 +828,5 @@ pub fn check_crate(tcx: ty::ctxt,
};
visit::walk_crate(&mut visitor, crate, ());
}
return exported_items;
}
188 changes: 50 additions & 138 deletions src/librustc/middle/reachable.rs
View file
Expand Up @@ -17,11 +17,13 @@

use middle::ty;
use middle::typeck;
use middle::privacy;
use middle::resolve;

use std::hashmap::HashSet;
use syntax::ast::*;
use syntax::ast_map;
use syntax::ast_util::def_id_of_def;
use syntax::ast_util::{def_id_of_def, is_local};
use syntax::attr;
use syntax::parse::token;
use syntax::visit::Visitor;
Expand Down Expand Up @@ -71,15 +73,6 @@ fn trait_method_might_be_inlined(trait_method: &trait_method) -> bool {
}
}

// The context we're in. If we're in a public context, then public symbols are
// marked reachable. If we're in a private context, then only trait
// implementations are marked reachable.
#[deriving(Clone, Eq)]
enum PrivacyContext {
PublicContext,
PrivateContext,
}

// Information needed while computing reachability.
struct ReachableContext {
// The type context.
Expand All @@ -92,108 +85,8 @@ struct ReachableContext {
// A worklist of item IDs. Each item ID in this worklist will be inlined
// and will be scanned for further references.
worklist: @mut ~[NodeId],
}

struct ReachableVisitor {
reachable_symbols: @mut HashSet<NodeId>,
worklist: @mut ~[NodeId],
}

impl Visitor<PrivacyContext> for ReachableVisitor {

fn visit_item(&mut self, item:@item, privacy_context:PrivacyContext) {

match item.node {
item_fn(*) => {
if privacy_context == PublicContext {
self.reachable_symbols.insert(item.id);
}
if item_might_be_inlined(item) {
self.worklist.push(item.id)
}
}
item_struct(ref struct_def, _) => {
match struct_def.ctor_id {
Some(ctor_id) if
privacy_context == PublicContext => {
self.reachable_symbols.insert(ctor_id);
}
Some(_) | None => {}
}
}
item_enum(ref enum_def, _) => {
if privacy_context == PublicContext {
for variant in enum_def.variants.iter() {
self.reachable_symbols.insert(variant.node.id);
}
}
}
item_impl(ref generics, ref trait_ref, _, ref methods) => {
// XXX(pcwalton): We conservatively assume any methods
// on a trait implementation are reachable, when this
// is not the case. We could be more precise by only
// treating implementations of reachable or cross-
// crate traits as reachable.

let should_be_considered_public = |method: @method| {
(method.vis == public &&
privacy_context == PublicContext) ||
trait_ref.is_some()
};

// Mark all public methods as reachable.
for &method in methods.iter() {
if should_be_considered_public(method) {
self.reachable_symbols.insert(method.id);
}
}

if generics_require_inlining(generics) {
// If the impl itself has generics, add all public
// symbols to the worklist.
for &method in methods.iter() {
if should_be_considered_public(method) {
self.worklist.push(method.id)
}
}
} else {
// Otherwise, add only public methods that have
// generics to the worklist.
for method in methods.iter() {
let generics = &method.generics;
let attrs = &method.attrs;
if generics_require_inlining(generics) ||
attributes_specify_inlining(*attrs) ||
should_be_considered_public(*method) {
self.worklist.push(method.id)
}
}
}
}
item_trait(_, _, ref trait_methods) => {
// Mark all provided methods as reachable.
if privacy_context == PublicContext {
for trait_method in trait_methods.iter() {
match *trait_method {
provided(method) => {
self.reachable_symbols.insert(method.id);
self.worklist.push(method.id)
}
required(_) => {}
}
}
}
}
_ => {}
}

if item.vis == public && privacy_context == PublicContext {
visit::walk_item(self, item, PublicContext)
} else {
visit::walk_item(self, item, PrivateContext)
}
}

// Known reexports of modules
exp_map2: resolve::ExportMap2,
}

struct MarkSymbolVisitor {
Expand Down Expand Up @@ -256,31 +149,17 @@ impl Visitor<()> for MarkSymbolVisitor {

impl ReachableContext {
// Creates a new reachability computation context.
fn new(tcx: ty::ctxt, method_map: typeck::method_map)
-> ReachableContext {
fn new(tcx: ty::ctxt, method_map: typeck::method_map,
exp_map2: resolve::ExportMap2) -> ReachableContext {
ReachableContext {
tcx: tcx,
method_map: method_map,
reachable_symbols: @mut HashSet::new(),
worklist: @mut ~[],
exp_map2: exp_map2,
}
}

// Step 1: Mark all public symbols, and add all public symbols that might
// be inlined to a worklist.
fn mark_public_symbols(&self, crate: &Crate) {
let reachable_symbols = self.reachable_symbols;
let worklist = self.worklist;

let mut visitor = ReachableVisitor {
reachable_symbols: reachable_symbols,
worklist: worklist,
};


visit::walk_crate(&mut visitor, crate, PublicContext);
}

// Returns true if the given def ID represents a local item that is
// eligible for inlining and false otherwise.
fn def_id_represents_local_inlined_item(tcx: ty::ctxt, def_id: DefId)
Expand Down Expand Up @@ -352,6 +231,19 @@ impl ReachableContext {
}
}

fn propagate_mod(&self, id: NodeId) {
match self.exp_map2.find(&id) {
Some(l) => {
for reexport in l.iter() {
if reexport.reexport && is_local(reexport.def_id) {
self.worklist.push(reexport.def_id.node);
}
}
}
None => {}
}
}

// Step 2: Mark all symbols that the symbols on the worklist touch.
fn propagate(&self) {
let mut visitor = self.init_visitor();
Expand All @@ -373,6 +265,18 @@ impl ReachableContext {
item_fn(_, _, _, _, ref search_block) => {
visit::walk_block(&mut visitor, search_block, ())
}
// Our recursion into modules involves looking up their
// public reexports and the destinations of those
// exports. Privacy will put them in the worklist, but
// we won't find them in the ast_map, so this is where
// we deal with publicly re-exported items instead.
item_mod(*) => { self.propagate_mod(item.id); }
// These are normal, nothing reachable about these
// inherently and their children are already in the
// worklist
item_struct(*) | item_impl(*) | item_static(*) |
item_enum(*) | item_ty(*) | item_trait(*) |
item_foreign_mod(*) => {}
_ => {
self.tcx.sess.span_bug(item.span,
"found non-function item \
Expand All @@ -382,10 +286,8 @@ impl ReachableContext {
}
Some(&ast_map::node_trait_method(trait_method, _, _)) => {
match *trait_method {
required(ref ty_method) => {
self.tcx.sess.span_bug(ty_method.span,
"found required method in \
worklist?!")
required(*) => {
// Keep going, nothing to get exported
}
provided(ref method) => {
visit::walk_block(&mut visitor, &method.body, ())
Expand All @@ -395,6 +297,10 @@ impl ReachableContext {
Some(&ast_map::node_method(ref method, _, _)) => {
visit::walk_block(&mut visitor, &method.body, ())
}
// Nothing to recurse on for these
Some(&ast_map::node_foreign_item(*)) |
Some(&ast_map::node_variant(*)) |
Some(&ast_map::node_struct_ctor(*)) => {}
Some(_) => {
let ident_interner = token::get_ident_interner();
let desc = ast_map::node_id_to_str(self.tcx.items,
Expand All @@ -404,6 +310,9 @@ impl ReachableContext {
worklist: {}",
desc))
}
None if search_item == CRATE_NODE_ID => {
self.propagate_mod(search_item);
}
None => {
self.tcx.sess.bug(format!("found unmapped ID in worklist: \
{}",
Expand All @@ -429,7 +338,8 @@ impl ReachableContext {

pub fn find_reachable(tcx: ty::ctxt,
method_map: typeck::method_map,
crate: &Crate)
exp_map2: resolve::ExportMap2,
exported_items: &privacy::ExportedItems)
-> @mut HashSet<NodeId> {
// XXX(pcwalton): We only need to mark symbols that are exported. But this
// is more complicated than just looking at whether the symbol is `pub`,
Expand All @@ -442,11 +352,13 @@ pub fn find_reachable(tcx: ty::ctxt,
// is to have the name resolution pass mark all targets of a `pub use` as
// "must be reachable".

let reachable_context = ReachableContext::new(tcx, method_map);
let reachable_context = ReachableContext::new(tcx, method_map, exp_map2);

// Step 1: Mark all public symbols, and add all public symbols that might
// be inlined to a worklist.
reachable_context.mark_public_symbols(crate);
// Step 1: Seed the worklist with all nodes which were found to be public as
// a result of the privacy pass
for &id in exported_items.iter() {
reachable_context.worklist.push(id);
}

// Step 2: Mark all symbols that the symbols on the worklist touch.
reachable_context.propagate();
Expand Down
1 change: 0 additions & 1 deletion src/librustc/middle/trans/base.rs
View file
Expand Up @@ -2535,7 +2535,6 @@ pub fn get_item_val(ccx: @mut CrateContext, id: ast::NodeId) -> ValueRef {
let (v, inlineable) = consts::const_expr(ccx, expr);
ccx.const_values.insert(id, v);
let mut inlineable = inlineable;
exprt = true;

unsafe {
let llty = llvm::LLVMTypeOf(v);
Expand Down
15 changes: 15 additions & 0 deletions src/test/auxiliary/reexport-should-still-link.rs
View file
@@ -0,0 +1,15 @@
// Copyright 2013 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.

pub use foo::bar;

mod foo {
pub fn bar() {}
}
2 changes: 1 addition & 1 deletion src/test/codegen/iterate-over-array.rs
View file
Expand Up @@ -9,7 +9,7 @@
// except according to those terms.

#[no_mangle]
fn test(x: &[int]) -> int {
pub fn test(x: &[int]) -> int {
let mut y = 0;
let mut i = 0;
while (i < x.len()) {
Expand Down
2 changes: 1 addition & 1 deletion src/test/codegen/scalar-function-call.rs
View file
Expand Up @@ -13,6 +13,6 @@ fn foo(x: int) -> int {
}

#[no_mangle]
fn test() {
pub fn test() {
let _x = foo(10);
}

0 comments on commit 8015f9c

Please sign in to comment.