Skip to content

Commit

Permalink
Auto merge of #52890 - djrenren:test-visibility, r=petrochenkov
Browse files Browse the repository at this point in the history
Reexport tests without polluting namespaces

This should fix issue #52557.

Basically now we gensym a new name for the test function and reexport that.
That way the test function's reexport name can't conflict because it was impossible for the test author to write it down.
We then use a `use` statement to expose the original name using the original visibility.
  • Loading branch information
bors committed Aug 2, 2018
2 parents db54765 + 77f9aca commit 02a369a
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 5 deletions.
67 changes: 62 additions & 5 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use codemap::{ExpnInfo, MacroBang, MacroAttribute, dummy_spanned, respan};
use config::{is_test_or_bench, StripUnconfigured};
use errors::{Applicability, FatalError};
use ext::base::*;
use ext::build::AstBuilder;
use ext::derive::{add_derived_markers, collect_derives};
use ext::hygiene::{self, Mark, SyntaxContext};
use ext::placeholders::{placeholder, PlaceholderExpander};
Expand Down Expand Up @@ -474,6 +475,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
cx: self.cx,
invocations: Vec::new(),
monotonic: self.monotonic,
tests_nameable: true,
};
(fragment.fold_with(&mut collector), collector.invocations)
};
Expand Down Expand Up @@ -1049,6 +1051,11 @@ struct InvocationCollector<'a, 'b: 'a> {
cfg: StripUnconfigured<'a>,
invocations: Vec<Invocation>,
monotonic: bool,

/// Test functions need to be nameable. Tests inside functions or in other
/// unnameable locations need to be ignored. `tests_nameable` tracks whether
/// any test functions found in the current context would be nameable.
tests_nameable: bool,
}

impl<'a, 'b> InvocationCollector<'a, 'b> {
Expand All @@ -1066,6 +1073,20 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
placeholder(fragment_kind, NodeId::placeholder_from_mark(mark))
}

/// Folds the item allowing tests to be expanded because they are still nameable.
/// This should probably only be called with module items
fn fold_nameable(&mut self, item: P<ast::Item>) -> SmallVector<P<ast::Item>> {
fold::noop_fold_item(item, self)
}

/// Folds the item but doesn't allow tests to occur within it
fn fold_unnameable(&mut self, item: P<ast::Item>) -> SmallVector<P<ast::Item>> {
let was_nameable = mem::replace(&mut self.tests_nameable, false);
let items = fold::noop_fold_item(item, self);
self.tests_nameable = was_nameable;
items
}

fn collect_bang(&mut self, mac: ast::Mac, span: Span, kind: AstFragmentKind) -> AstFragment {
self.collect(kind, InvocationKind::Bang { mac: mac, ident: None, span: span })
}
Expand Down Expand Up @@ -1306,7 +1327,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
}
ast::ItemKind::Mod(ast::Mod { inner, .. }) => {
if item.ident == keywords::Invalid.ident() {
return noop_fold_item(item, self);
return self.fold_nameable(item);
}

let orig_directory_ownership = self.cx.current_expansion.directory_ownership;
Expand Down Expand Up @@ -1346,22 +1367,58 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

let orig_module =
mem::replace(&mut self.cx.current_expansion.module, Rc::new(module));
let result = noop_fold_item(item, self);
let result = self.fold_nameable(item);
self.cx.current_expansion.module = orig_module;
self.cx.current_expansion.directory_ownership = orig_directory_ownership;
result
}
// Ensure that test functions are accessible from the test harness.
// #[test] fn foo() {}
// becomes:
// #[test] pub fn foo_gensym(){}
// #[allow(unused)]
// use foo_gensym as foo;
ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => {
if item.attrs.iter().any(|attr| is_test_or_bench(attr)) {
if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) {
let orig_ident = item.ident;
let orig_vis = item.vis.clone();

// Publicize the item under gensymed name to avoid pollution
item = item.map(|mut item| {
item.vis = respan(item.vis.span, ast::VisibilityKind::Public);
item.ident = item.ident.gensym();
item
});

// Use the gensymed name under the item's original visibility
let mut use_item = self.cx.item_use_simple_(
item.ident.span,
orig_vis,
Some(orig_ident),
self.cx.path(item.ident.span,
vec![keywords::SelfValue.ident(), item.ident]));

// #[allow(unused)] because the test function probably isn't being referenced
use_item = use_item.map(|mut ui| {
ui.attrs.push(
self.cx.attribute(DUMMY_SP, attr::mk_list_item(DUMMY_SP,
Ident::from_str("allow"), vec![
attr::mk_nested_word_item(Ident::from_str("unused"))
]
))
);

ui
});

SmallVector::many(
self.fold_unnameable(item).into_iter()
.chain(self.fold_unnameable(use_item)))
} else {
self.fold_unnameable(item)
}
noop_fold_item(item, self)
}
_ => noop_fold_item(item, self),
_ => self.fold_unnameable(item),
}
}

Expand Down
38 changes: 38 additions & 0 deletions src/test/run-pass/issue-52557.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2015 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.

// This test checks for namespace pollution by private tests.
// Tests used to marked as public causing name conflicts with normal
// functions only in test builds.

// compile-flags: --test

mod a {
pub fn foo() -> bool {
true
}
}

mod b {
#[test]
fn foo() {
local_name(); // ensure the local name still works
}

#[test]
fn local_name() {}
}

use a::*;
use b::*;

pub fn conflict() {
let _: bool = foo();
}

0 comments on commit 02a369a

Please sign in to comment.