From f76049cd6abd1964999764e5cf596748d29792b4 Mon Sep 17 00:00:00 2001 From: John Renner Date: Tue, 24 Jul 2018 17:51:37 -0700 Subject: [PATCH 1/6] Reexport tests without polluting namespaces --- src/libsyntax/ext/expand.rs | 20 ++++++++++++++++++- src/test/run-pass/issue-52557.rs | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 src/test/run-pass/issue-52557.rs diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index b84046d105051..4b17ca87d8775 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}; @@ -1354,12 +1355,29 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { // Ensure that test functions are accessible from the test harness. ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => { if item.attrs.iter().any(|attr| is_test_or_bench(attr)) { + 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 = Ident::from_interned_str( + item.ident.as_interned_str()).gensym(); item }); + + // Use the gensymed name under the item's original visibility + let use_item = self.cx.item_use_simple_( + item.ident.span, + orig_vis, + Some(Ident::from_interned_str(item.ident.as_interned_str())), + self.cx.path(item.ident.span, vec![item.ident])); + + SmallVector::many( + noop_fold_item(item, self).into_iter() + .chain(noop_fold_item(use_item, self))) + } else { + noop_fold_item(item, self) } - noop_fold_item(item, self) } _ => noop_fold_item(item, self), } diff --git a/src/test/run-pass/issue-52557.rs b/src/test/run-pass/issue-52557.rs new file mode 100644 index 0000000000000..d6d272b5fadcc --- /dev/null +++ b/src/test/run-pass/issue-52557.rs @@ -0,0 +1,33 @@ +// 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 or the MIT license +// , 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() {} +} + +use a::*; +use b::*; + +fn conflict() { + let _: bool = foo(); +} \ No newline at end of file From caab47d3e95377c1fb9ba80f866f459060e103af Mon Sep 17 00:00:00 2001 From: John Renner Date: Mon, 30 Jul 2018 19:55:23 -0700 Subject: [PATCH 2/6] Just tidying up --- src/test/run-pass/issue-52557.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/test/run-pass/issue-52557.rs b/src/test/run-pass/issue-52557.rs index d6d272b5fadcc..2b8dfe162cc3d 100644 --- a/src/test/run-pass/issue-52557.rs +++ b/src/test/run-pass/issue-52557.rs @@ -22,12 +22,17 @@ mod a { mod b { #[test] - fn foo() {} + fn foo() { + local_name(); // ensure the local name still works + } + + #[test] + fn local_name() {} } use a::*; use b::*; -fn conflict() { +pub fn conflict() { let _: bool = foo(); -} \ No newline at end of file +} From 7947c58d2dafefed7bfa7e8a24d5f90d11b43517 Mon Sep 17 00:00:00 2001 From: John Renner Date: Tue, 31 Jul 2018 13:17:44 -0700 Subject: [PATCH 3/6] Allow unnameable tests --- src/libsyntax/ext/expand.rs | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 4b17ca87d8775..975b91a1088a0 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -475,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) }; @@ -1050,6 +1051,11 @@ struct InvocationCollector<'a, 'b: 'a> { cfg: StripUnconfigured<'a>, invocations: Vec, 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> { @@ -1067,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) -> SmallVector> { + 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) -> SmallVector> { + 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 }) } @@ -1307,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; @@ -1347,14 +1367,14 @@ 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. 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_vis = item.vis.clone(); // Publicize the item under gensymed name to avoid pollution @@ -1370,16 +1390,16 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { item.ident.span, orig_vis, Some(Ident::from_interned_str(item.ident.as_interned_str())), - self.cx.path(item.ident.span, vec![item.ident])); + self.cx.path(item.ident.span, vec![Ident::from_str("self"), item.ident])); SmallVector::many( - noop_fold_item(item, self).into_iter() - .chain(noop_fold_item(use_item, self))) + self.fold_unnameable(item).into_iter() + .chain(self.fold_unnameable(use_item))) } else { - noop_fold_item(item, self) + self.fold_unnameable(item) } } - _ => noop_fold_item(item, self), + _ => self.fold_unnameable(item), } } From 549f0fd9f7740954cb14367d735db724669c8f79 Mon Sep 17 00:00:00 2001 From: John Renner Date: Tue, 31 Jul 2018 15:00:45 -0700 Subject: [PATCH 4/6] Address code review --- src/libsyntax/ext/expand.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 975b91a1088a0..96003be31522c 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1373,15 +1373,19 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { result } // Ensure that test functions are accessible from the test harness. + // #[test] fn foo() {} + // becomes: + // #[test] pub fn foo_gensym(){} + // use foo_gensym as foo; ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => { if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) { - let orig_vis = item.vis.clone(); + 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 = Ident::from_interned_str( - item.ident.as_interned_str()).gensym(); + item.ident = item.ident.gensym(); item }); @@ -1389,8 +1393,9 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { let use_item = self.cx.item_use_simple_( item.ident.span, orig_vis, - Some(Ident::from_interned_str(item.ident.as_interned_str())), - self.cx.path(item.ident.span, vec![Ident::from_str("self"), item.ident])); + Some(orig_ident), + self.cx.path(item.ident.span, + vec![keywords::SelfValue.ident(), item.ident])); SmallVector::many( self.fold_unnameable(item).into_iter() From af7ae2f278cb5f5b2d054d15dc36a39178a48b69 Mon Sep 17 00:00:00 2001 From: John Renner Date: Wed, 1 Aug 2018 11:28:08 -0700 Subject: [PATCH 5/6] Allow test imports to go unused --- src/libsyntax/ext/expand.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 96003be31522c..590d8cd6c99f4 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1376,6 +1376,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { // #[test] fn foo() {} // becomes: // #[test] pub fn foo_gensym(){} + // #[allow(dead_code)] // use foo_gensym as foo; ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => { if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) { @@ -1390,13 +1391,26 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { }); // Use the gensymed name under the item's original visibility - let use_item = self.cx.item_use_simple_( + 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(dead_code)] 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("dead_code")) + ] + )) + ); + + ui + }); + SmallVector::many( self.fold_unnameable(item).into_iter() .chain(self.fold_unnameable(use_item))) From 77f9aca2a3e30eb430ba5a506261ff126c2d3077 Mon Sep 17 00:00:00 2001 From: John Renner Date: Wed, 1 Aug 2018 12:33:10 -0700 Subject: [PATCH 6/6] Use the correct allow --- src/libsyntax/ext/expand.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 590d8cd6c99f4..7a9422ef53d58 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1376,7 +1376,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { // #[test] fn foo() {} // becomes: // #[test] pub fn foo_gensym(){} - // #[allow(dead_code)] + // #[allow(unused)] // use foo_gensym as foo; ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => { if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) { @@ -1398,12 +1398,12 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> { self.cx.path(item.ident.span, vec![keywords::SelfValue.ident(), item.ident])); - // #[allow(dead_code)] because the test function probably isn't being referenced + // #[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("dead_code")) + attr::mk_nested_word_item(Ident::from_str("unused")) ] )) );