Skip to content

Purge #[!resolve_unexported] from the compiler #15847

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Purge #[!resolve_unexported] from the compiler #15847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5cc221
Remove useless RefCells
sfackler Jul 21, 2014
bca53e9
Restructure test harness
sfackler Jul 21, 2014
1d522a6
Purge !resolve_unexported
sfackler Jul 21, 2014
173dcec
Don't create reexport module if there are none
sfackler Jul 21, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 66 additions & 44 deletions src/librustc/front/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,17 @@
use driver::session::Session;
use front::config;

use std::cell::RefCell;
use std::gc::{Gc, GC};
use std::slice;
use std::mem;
use std::vec;
use syntax::ast_util::*;
use syntax::attr::AttrMetaMethods;
use syntax::attr;
use syntax::codemap::{DUMMY_SP, Span, ExpnInfo, NameAndSpan, MacroAttribute};
use syntax::codemap;
use syntax::ext::base::ExtCtxt;
use syntax::ext::build::AstBuilder;
use syntax::ext::expand::ExpansionConfig;
use syntax::fold::Folder;
use syntax::fold;
Expand All @@ -46,9 +47,11 @@ struct Test {

struct TestCtxt<'a> {
sess: &'a Session,
path: RefCell<Vec<ast::Ident>>,
path: Vec<ast::Ident>,
reexports: Vec<Vec<ast::Ident>>,
ext_cx: ExtCtxt<'a>,
testfns: RefCell<Vec<Test> >,
testfns: Vec<Test>,
reexport_mod_ident: ast::Ident,
is_test_crate: bool,
config: ast::CrateConfig,
}
Expand Down Expand Up @@ -86,9 +89,9 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
}

fn fold_item(&mut self, i: Gc<ast::Item>) -> SmallVector<Gc<ast::Item>> {
self.cx.path.borrow_mut().push(i.ident);
self.cx.path.push(i.ident);
debug!("current path: {}",
ast_util::path_name_i(self.cx.path.borrow().as_slice()));
ast_util::path_name_i(self.cx.path.as_slice()));

if is_test_fn(&self.cx, i) || is_bench_fn(&self.cx, i) {
match i.node {
Expand All @@ -102,31 +105,41 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
debug!("this is a test function");
let test = Test {
span: i.span,
path: self.cx.path.borrow().clone(),
path: self.cx.path.clone(),
bench: is_bench_fn(&self.cx, i),
ignore: is_ignored(&self.cx, i),
should_fail: should_fail(i)
};
self.cx.testfns.borrow_mut().push(test);
self.cx.testfns.push(test);
self.cx.reexports.push(self.cx.path.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to reexport even public test functions, right? Ideally it wouldn't reexport any public item at all, it would only reexport private items as necessary (either private test functions, or private modules that contain test functions).

Copy link
Member Author

Choose a reason for hiding this comment

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

We could avoid reexporting public test functions, but I don't think it's worth it since it would complicate the implementation a bit and I don't think I've even seen a public test function.

Copy link
Member

Choose a reason for hiding this comment

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

Seems far far saner to just handle everything in a uniform manner (i.e. always import from foo::__test_reexports, rather than some from that and other from just foo).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm actually more interested in avoiding reexporting public modules. If I have a public module chain foo::bar::baz with a private module tests at the end, it would ideally access any tests methods via foo::bar::baz::__test_reexports::tests instead of via __test_reexports::foo::__test_reexports::bar::__test_reexports::baz::__test__reexports::tests.

Copy link
Member

Choose a reason for hiding this comment

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

Why is that particularly valuable? It seems like it would be far more common for a human to interact with this implementation, rather than the output, so keeping the implementation simple seems like the best strategy.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a serious complication, really, and of course it forestalls the human asking the question of "why is this reexported when it's already public?"

And it makes the expanded output much simpler, which is a win for the reader, and also presumably a win for the compiler as well, because a smaller AST means less memory used and also less processing time to visit or fold over the AST. I don't know if the compiler win is anything more than negligible of course ;) but I do think that avoiding the human confusion of "why reexport a public item?" is a good thing.

Copy link
Member

Choose a reason for hiding this comment

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

avoiding the human confusion of "why reexport a public item?" is a good thing.

I think "reexport everything for simplicity" is a perfectly sensible answer. I personally assumed this was the strategy was taken when reading the pull request description.

(And, yes, the compilation time is negligible, e.g. see -Z time-passes for the passes like prelude injection and finding the main symbol: the expensive bits are LLVM and things like type checking and coherence. This affects none of them: all of those passes will 'automatically' do nothing, very cheaply, for modules without anything interesting in them.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I'm not going to push for this if you disagree. It seems an unnecessary complication of the AST, but I guess it doesn't really matter.

// debug!("have {} test/bench functions",
// cx.testfns.len());
}
}
}

let res = fold::noop_fold_item(&*i, self);
self.cx.path.borrow_mut().pop();
// We don't want to recurse into anything other than mods, since
// mods or tests inside of functions will break things
let res = match i.node {
ast::ItemMod(..) => fold::noop_fold_item(&*i, self),
_ => SmallVector::one(i),
};
self.cx.path.pop();
res
}

fn fold_mod(&mut self, m: &ast::Mod) -> ast::Mod {
let reexports = mem::replace(&mut self.cx.reexports, Vec::new());
let mut mod_folded = fold::noop_fold_mod(m, self);
let reexports = mem::replace(&mut self.cx.reexports, reexports);

// Remove any #[main] from the AST so it doesn't clash with
// the one we're going to add. Only if compiling an executable.

fn nomain(item: Gc<ast::Item>) -> Gc<ast::Item> {
box(GC) ast::Item {
attrs: item.attrs.iter().filter_map(|attr| {
if !attr.name().equiv(&("main")) {
if !attr.check_name("main") {
Some(*attr)
} else {
None
Expand All @@ -136,27 +149,50 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
}
}

let mod_nomain = ast::Mod {
inner: m.inner,
view_items: m.view_items.clone(),
items: m.items.iter().map(|i| nomain(*i)).collect(),
};
for i in mod_folded.items.mut_iter() {
*i = nomain(*i);
}
if !reexports.is_empty() {
mod_folded.items.push(mk_reexport_mod(&mut self.cx, reexports));
self.cx.reexports.push(self.cx.path.clone());
}

mod_folded
}
}

fold::noop_fold_mod(&mod_nomain, self)
fn mk_reexport_mod(cx: &mut TestCtxt, reexports: Vec<Vec<ast::Ident>>)
-> Gc<ast::Item> {
let view_items = reexports.move_iter().map(|r| {
cx.ext_cx.view_use_simple(DUMMY_SP, ast::Public, cx.ext_cx.path(DUMMY_SP, r))
}).collect();
let reexport_mod = ast::Mod {
inner: DUMMY_SP,
view_items: view_items,
items: Vec::new(),
};
box(GC) ast::Item {
ident: cx.reexport_mod_ident.clone(),
attrs: Vec::new(),
id: ast::DUMMY_NODE_ID,
node: ast::ItemMod(reexport_mod),
vis: ast::Public,
span: DUMMY_SP,
}
}

fn generate_test_harness(sess: &Session, krate: ast::Crate)
-> ast::Crate {
fn generate_test_harness(sess: &Session, krate: ast::Crate) -> ast::Crate {
let mut cx: TestCtxt = TestCtxt {
sess: sess,
ext_cx: ExtCtxt::new(&sess.parse_sess, sess.opts.cfg.clone(),
ExpansionConfig {
deriving_hash_type_parameter: false,
crate_name: "test".to_string(),
}),
path: RefCell::new(Vec::new()),
testfns: RefCell::new(Vec::new()),
path: Vec::new(),
reexports: Vec::new(),
testfns: Vec::new(),
reexport_mod_ident: token::str_to_ident("__test_reexports"),
is_test_crate: is_test_crate(&krate),
config: krate.config.clone(),
};
Expand All @@ -171,7 +207,7 @@ fn generate_test_harness(sess: &Session, krate: ast::Crate)
});

let mut fold = TestHarnessGenerator {
cx: cx
cx: cx,
};
let res = fold.fold_crate(krate);
fold.cx.ext_cx.bt_pop();
Expand Down Expand Up @@ -275,7 +311,6 @@ fn add_test_module(cx: &TestCtxt, m: &ast::Mod) -> ast::Mod {
We're going to be building a module that looks more or less like:

mod __test {
#![!resolve_unexported]
extern crate test (name = "test", vers = "...");
fn main() {
test::test_main_static(::os::args().as_slice(), tests)
Expand Down Expand Up @@ -332,15 +367,9 @@ fn mk_test_module(cx: &TestCtxt) -> Gc<ast::Item> {
};
let item_ = ast::ItemMod(testmod);

// This attribute tells resolve to let us call unexported functions
let resolve_unexported_str = InternedString::new("!resolve_unexported");
let resolve_unexported_attr =
attr::mk_attr_inner(attr::mk_attr_id(),
attr::mk_word_item(resolve_unexported_str));

let item = ast::Item {
ident: token::str_to_ident("__test"),
attrs: vec!(resolve_unexported_attr),
attrs: Vec::new(),
id: ast::DUMMY_NODE_ID,
node: item_,
vis: ast::Public,
Expand Down Expand Up @@ -368,18 +397,6 @@ fn path_node(ids: Vec<ast::Ident> ) -> ast::Path {
}
}

fn path_node_global(ids: Vec<ast::Ident> ) -> ast::Path {
ast::Path {
span: DUMMY_SP,
global: true,
segments: ids.move_iter().map(|identifier| ast::PathSegment {
identifier: identifier,
lifetimes: Vec::new(),
types: OwnedSlice::empty(),
}).collect()
}
}

fn mk_tests(cx: &TestCtxt) -> Gc<ast::Item> {
// The vector of test_descs for this crate
let test_descs = mk_test_descs(cx);
Expand All @@ -399,13 +416,13 @@ fn is_test_crate(krate: &ast::Crate) -> bool {
}

fn mk_test_descs(cx: &TestCtxt) -> Gc<ast::Expr> {
debug!("building test vector from {} tests", cx.testfns.borrow().len());
debug!("building test vector from {} tests", cx.testfns.len());

box(GC) ast::Expr {
id: ast::DUMMY_NODE_ID,
node: ast::ExprVstore(box(GC) ast::Expr {
id: ast::DUMMY_NODE_ID,
node: ast::ExprVec(cx.testfns.borrow().iter().map(|test| {
node: ast::ExprVec(cx.testfns.iter().map(|test| {
mk_test_desc_and_fn_rec(cx, test)
}).collect()),
span: DUMMY_SP,
Expand All @@ -431,7 +448,12 @@ fn mk_test_desc_and_fn_rec(cx: &TestCtxt, test: &Test) -> Gc<ast::Expr> {
span: span
};

let fn_path = path_node_global(path);
let mut visible_path = Vec::new();
for ident in path.move_iter() {
visible_path.push(cx.reexport_mod_ident.clone());
visible_path.push(ident);
}
let fn_path = cx.ext_cx.path_global(DUMMY_SP, visible_path);

let fn_expr = box(GC) ast::Expr {
id: ast::DUMMY_NODE_ID,
Expand Down
7 changes: 0 additions & 7 deletions src/librustc/middle/privacy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use util::nodemap::{NodeMap, NodeSet};
use syntax::ast;
use syntax::ast_map;
use syntax::ast_util::{is_local, local_def, PostExpansionMethod};
use syntax::attr;
use syntax::codemap::Span;
use syntax::parse::token;
use syntax::owned_slice::OwnedSlice;
Expand Down Expand Up @@ -786,12 +785,6 @@ impl<'a> PrivacyVisitor<'a> {

impl<'a> Visitor<()> for PrivacyVisitor<'a> {
fn visit_item(&mut self, item: &ast::Item, _: ()) {
// Do not check privacy inside items with the resolve_unexported
// attribute. This is used for the test runner.
if attr::contains_name(item.attrs.as_slice(), "!resolve_unexported") {
return;
}

let orig_curitem = replace(&mut self.curitem, item.id);
visit::walk_item(self, item, ());
self.curitem = orig_curitem;
Expand Down