Auto merge of #52890 - djrenren:test-visibility, r=petrochenkov

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.

2 files changed, 100 insertions, 5 deletions

diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
index 9f8909e1626..383813d73b1 100644
--- a/src/libsyntax/ext/expand.rs
+++ b/src/libsyntax/ext/expand.rs
@@ -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};
@@ -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)
         };
@@ -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> {
@@ -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 })
     }
@@ -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;
@@ -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),
         }
     }
 
diff --git a/src/test/run-pass/issue-52557.rs b/src/test/run-pass/issue-52557.rs
new file mode 100644
index 00000000000..2b8dfe162cc
--- /dev/null
+++ b/src/test/run-pass/issue-52557.rs
@@ -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();
+}