Allowing #[test] on inner functions

Some background: issue #36629 "Inner function cannot be tested"

Currently Rust doesn't support #[test]s inside of functions. Example:

#[test]
fn outer_test() {
}

fn foo() {
    #[test]
    fn inner_test() {
    }
}

This expands (using rustc --test lib.rs -Z unpretty=hir) into:

#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
extern crate test;
#[cfg(test)]
#[rustc_test_marker]
pub const outer_test: test::TestDescAndFn = test::TestDescAndFn {
    desc: test::TestDesc {
        name: test::StaticTestName("outer_test"),
        ignore: false,
        allow_fail: false,
        should_panic: test::ShouldPanic::No,
        test_type: test::TestType::Unknown,
    },
    testfn: test::StaticTestFn(|| test::assert_test_result(outer_test())),
};
fn outer_test() {}

fn foo() {
    extern crate test;
    #[cfg(test)]
    #[rustc_test_marker]
    pub const inner_test: test::TestDescAndFn = test::TestDescAndFn {
        desc: test::TestDesc {
            name: test::StaticTestName("inner_test"),
            ignore: false,
            allow_fail: false,
            should_panic: test::ShouldPanic::No,
            test_type: test::TestType::Unknown,
        },
        testfn: test::StaticTestFn(|| test::assert_test_result(inner_test())),
    };
    fn inner_test() {}
}
#[main]
pub fn main() -> () {
    extern crate test;
    test::test_main_static(&[&outer_test])
}

Note that inner_test is generated but main can't access it so it's ignored. I would really like to fix this because I have attribute macros that would benefit from being able to inject tests but they can't because of this issue.

@petrochenkov suggested a workaround. I wish I understood it, but I'm not familiar enough with Rust's internals. So, in my naivety, I'd like to propose an alternative solution: could each TestDescAndFn be made a static instead of a const, and then assigned an #[export_name] so they can be referenced from main?

Here's an example of what I mean:

struct TestDescAndFn(&'static str, fn());

pub fn foo() {
    #[export_name = "_inner_test_export_name"]
    #[used]
    static INNER_TEST: TestDescAndFn = TestDescAndFn("This is inner_test", inner_test);
    fn inner_test() {}
}

fn main() {
    extern "Rust" {
        #[link_name = "_inner_test_export_name"]
        static inner_test_static: TestDescAndFn;
    }
    let inner_test = unsafe { &inner_test_static };
    inner_test.1();
}

Is this a feasible solution? Another idea I had was to use something like linkme's distributed slices, though that seems less portable than this.

2 Likes

#[test] and test harness already use "macro 2.0" hygiene instead of reexports starting from https://github.com/rust-lang/rust/pull/63919#discussion_r318318098, so in theory #[test] functions inside functions of blocks should already be supported or almost supported.

Looks like the pass that collects test cases into a single list simply ignores inner functions. If it's tweaked to not ignore them everything should "just work" in theory.

6 Likes

Oh neat, thanks for the update. I'll have to fiddle with the test harness and see if I can get it to work.

1 Like

Okay, I got a chance to test this out but I have no idea what I'm doing and need some help debugging this. I made the following patch (based on HEAD=8498c5f5b02dbb4ed58a1eb4901b0b733342c35f):

From 35bbd5d4d74a054c4251ee2dcb3fe0ead5170791 Mon Sep 17 00:00:00 2001
From: Michael Bradshaw <mjbshaw@google.com>
Date: Sat, 15 Feb 2020 16:30:23 -0800
Subject: [PATCH] Support #[test] on inner functions

---
 src/librustc_builtin_macros/test_harness.rs | 65 ++++++++++++---------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/src/librustc_builtin_macros/test_harness.rs b/src/librustc_builtin_macros/test_harness.rs
index 70f1c0e4e2d..479dcca99eb 100644
--- a/src/librustc_builtin_macros/test_harness.rs
+++ b/src/librustc_builtin_macros/test_harness.rs
@@ -89,6 +89,8 @@ pub fn inject(
 struct TestHarnessGenerator<'a> {
     cx: TestCtxt<'a>,
     tests: Vec<Test>,
+    module_id: ast::NodeId,
+    parent_span: Span,
 }

 impl<'a> MutVisitor for TestHarnessGenerator<'a> {
@@ -108,36 +110,41 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
             self.tests.push(test);
         }

-        // We don't want to recurse into anything other than mods, since
-        // mods or tests inside of functions will break things
-        if let ast::ItemKind::Mod(mut module) = item.kind {
-            let tests = mem::take(&mut self.tests);
-            noop_visit_mod(&mut module, self);
-            let mut tests = mem::replace(&mut self.tests, tests);
-
-            if !tests.is_empty() {
-                let parent =
-                    if item.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { item.id };
-                // Create an identifier that will hygienically resolve the test
-                // case name, even in another module.
-                let expn_id = self.cx.ext_cx.resolver.expansion_for_ast_pass(
-                    module.inner,
-                    AstPass::TestHarness,
-                    &[],
-                    Some(parent),
-                );
-                for test in &mut tests {
-                    // See the comment on `mk_main` for why we're using
-                    // `apply_mark` directly.
-                    test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
-                }
-                self.cx.test_cases.extend(tests);
+        if let ast::ItemKind::Mod(ref module) = item.kind {
+            self.module_id =
+                if item.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { item.id };
+            self.parent_span = module.inner;
+        }
+
+        let tests = mem::take(&mut self.tests);
+        noop_visit_item_kind(&mut item.kind, self);
+        let mut tests = mem::replace(&mut self.tests, tests);
+        if !tests.is_empty() {
+            // Create an identifier that will hygienically resolve the test
+            // case name, even in another module.
+            let expn_id = self.cx.ext_cx.resolver.expansion_for_ast_pass(
+                self.parent_span,
+                AstPass::TestHarness,
+                &[],
+                Some(self.module_id),
+            );
+
+            for test in &mut tests {
+                // See the comment on `mk_main` for why we're using
+                // `apply_mark` directly.
+                test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
             }
-            item.kind = ast::ItemKind::Mod(module);
+            self.cx.test_cases.extend(tests);
         }
+
         smallvec![P(item)]
     }

+    fn visit_block(&mut self, b: &mut P<ast::Block>) {
+        self.parent_span = b.span;
+        noop_visit_block(b, self);
+    }
+
     fn visit_mac(&mut self, _mac: &mut ast::Mac) {
         // Do nothing.
     }
@@ -234,7 +241,13 @@ fn generate_test_harness(
         test_runner,
     };

-    TestHarnessGenerator { cx, tests: Vec::new() }.visit_crate(krate);
+    TestHarnessGenerator {
+        cx,
+        tests: Vec::new(),
+        module_id: ast::CRATE_NODE_ID,
+        parent_span: krate.span,
+    }
+    .visit_crate(krate);
 }

 /// Creates a function item for use as the main function of a test build.
--
2.24.0.525.g8f36a354ae-goog

Running rustc --edition=2018 -O -Zunpretty=hir --test /tmp/t.rs on the following code:

fn foo() {
    #[test]
    fn bar() {
    }
}

produces the following error:

error[E0425]: cannot find value `bar` in this scope
 --> /tmp/t.rs:3:5
  |
3 | /     fn bar() {
4 | |     }
  | |_____^ not found in this scope

#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
fn foo() {
    extern crate test;
    #[cfg(test)]
    #[rustc_test_marker]
    pub const bar: test::TestDescAndFn = test::TestDescAndFn {
        desc: test::TestDesc {
            name: test::StaticTestName("bar"),
            ignore: false,
            allow_fail: false,
            should_panic: test::ShouldPanic::No,
            test_type: test::TestType::Unknown,
        },
        testfn: test::StaticTestFn(|| test::assert_test_result(bar())),
    };
    fn bar() {}
}
#[main]
pub fn main() -> () {
    extern crate test;
    test::test_main_static(&[&bar])
}
error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.

The generated code looks right, but it looks like I've messed up the spans since bar can't be resolved. If anyone can offer some guidance I'd greatly appreciate it.

My guess is that self.module_id need to be set for blocks as well if you want to resolve anything relatively to the block.

I tried that first, actually, but it results in a panic+abort. Ugly diff:

diff --git a/src/librustc_builtin_macros/test_harness.rs b/src/librustc_builtin_macros/test_harness.rs
index 70f1c0e4e2d..98cb574bb88 100644
--- a/src/librustc_builtin_macros/test_harness.rs
+++ b/src/librustc_builtin_macros/test_harness.rs
@@ -89,6 +89,8 @@ pub fn inject(
 struct TestHarnessGenerator<'a> {
     cx: TestCtxt<'a>,
     tests: Vec<Test>,
+    module_id: ast::NodeId,
+    parent_span: Span,
 }

 impl<'a> MutVisitor for TestHarnessGenerator<'a> {
@@ -108,36 +110,55 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
             self.tests.push(test);
         }

-        // We don't want to recurse into anything other than mods, since
-        // mods or tests inside of functions will break things
-        if let ast::ItemKind::Mod(mut module) = item.kind {
-            let tests = mem::take(&mut self.tests);
-            noop_visit_mod(&mut module, self);
-            let mut tests = mem::replace(&mut self.tests, tests);
-
-            if !tests.is_empty() {
-                let parent =
-                    if item.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { item.id };
-                // Create an identifier that will hygienically resolve the test
-                // case name, even in another module.
-                let expn_id = self.cx.ext_cx.resolver.expansion_for_ast_pass(
-                    module.inner,
-                    AstPass::TestHarness,
-                    &[],
-                    Some(parent),
-                );
-                for test in &mut tests {
-                    // See the comment on `mk_main` for why we're using
-                    // `apply_mark` directly.
-                    test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
-                }
-                self.cx.test_cases.extend(tests);
+        let old_span = self.parent_span;
+        let old_module_id = self.module_id;
+
+        if let ast::ItemKind::Mod(ref module) = item.kind {
+            self.module_id =
+                if item.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { item.id };
+            self.parent_span = module.inner;
+        }
+
+        let tests = mem::take(&mut self.tests);
+        noop_visit_item_kind(&mut item.kind, self);
+        let mut tests = mem::replace(&mut self.tests, tests);
+        if !tests.is_empty() {
+            // Create an identifier that will hygienically resolve the test
+            // case name, even in another module.
+            let expn_id = self.cx.ext_cx.resolver.expansion_for_ast_pass(
+                self.parent_span,
+                AstPass::TestHarness,
+                &[],
+                Some(self.module_id),
+            );
+
+            for test in &mut tests {
+                // See the comment on `mk_main` for why we're using
+                // `apply_mark` directly.
+                test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
             }
-            item.kind = ast::ItemKind::Mod(module);
+            self.cx.test_cases.extend(tests);
         }
+
+        self.parent_span = old_span;
+        self.module_id = old_module_id;
+
         smallvec![P(item)]
     }

+    fn visit_block(&mut self, b: &mut P<ast::Block>) {
+        let old_span = self.parent_span;
+        let old_module_id = self.module_id;
+
+        self.parent_span = b.span;
+        self.module_id =
+            if b.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { b.id };
+        noop_visit_block(b, self);
+
+        self.parent_span = old_span;
+        self.module_id = old_module_id;
+    }
+
     fn visit_mac(&mut self, _mac: &mut ast::Mac) {
         // Do nothing.
     }
@@ -234,7 +255,13 @@ fn generate_test_harness(
         test_runner,
     };

-    TestHarnessGenerator { cx, tests: Vec::new() }.visit_crate(krate);
+    TestHarnessGenerator {
+        cx,
Support #[test] on inner functions
+        tests: Vec::new(),
+        module_id: ast::CRATE_NODE_ID,
+        parent_span: krate.span,
+    }
+    .visit_crate(krate);
 }

 /// Creates a function item for use as the main function of a test build.

commit 8498c5f5b02dbb4ed58a1eb4901b0b733342c35f (origin/master, origin/HEAD, master)
Merge: a29424a2265 4b3c66d2c30
Author: bors <bors@rust-lang.org>
Date:   Fri Feb 7 23:08:52 2020 +0000

    Auto merge of #65232 - nikomatsakis:lazy-norm-anon-const-push-2, r=matthewjasper

    replace the leak check with universes, take 2

    This PR is an attempt to revive the "universe-based region check", which is an important step towards lazy normalization. Unlike before, we also modify the definition of

    ```
    static ----------+-----...------+       (greatest)
~/P/r/src ❯❯❯ git add -u                                                                                                                             ✘ 141 alt_inner_test ✭ ◼
~/P/r/src ❯❯❯ git commit --amend                                                                                                                         alt_inner_test ✭ ✚ ◼
[alt_inner_test 7e088a8d74f] Support #[test] on inner functions
 Date: Sat Feb 15 16:30:23 2020 -0800
 1 file changed, 49 insertions(+), 20 deletions(-)
~/P/r/src ❯❯❯ git log -p                                                                                                                                   alt_inner_test ✭ ◼
commit 7e088a8d74f768b69aff0f2d09281c376e67bc76 (HEAD -> alt_inner_test)
Author: Michael Bradshaw <mjbshaw@google.com>
Date:   Sat Feb 15 16:30:23 2020 -0800

    Support #[test] on inner functions

diff --git a/src/librustc_builtin_macros/test_harness.rs b/src/librustc_builtin_macros/test_harness.rs
index 70f1c0e4e2d..0021743b34a 100644
--- a/src/librustc_builtin_macros/test_harness.rs
+++ b/src/librustc_builtin_macros/test_harness.rs
@@ -22,6 +22,8 @@ use std::{iter, mem};
 struct Test {
     span: Span,
     ident: Ident,
+    parent_span: Span,
+    module_id: ast::NodeId,
 }

 struct TestCtxt<'a> {
@@ -89,6 +91,8 @@ pub fn inject(
 struct TestHarnessGenerator<'a> {
     cx: TestCtxt<'a>,
     tests: Vec<Test>,
+    module_id: ast::NodeId,
+    parent_span: Span,
 }

 impl<'a> MutVisitor for TestHarnessGenerator<'a> {
@@ -104,40 +108,59 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
         if is_test_case(&item) {
             debug!("this is a test item");

-            let test = Test { span: item.span, ident: item.ident };
+            let test = Test { span: item.span, ident: item.ident, parent_span: self.parent_span, module_id: self.module_id };
             self.tests.push(test);
         }

-        // We don't want to recurse into anything other than mods, since
-        // mods or tests inside of functions will break things
-        if let ast::ItemKind::Mod(mut module) = item.kind {
-            let tests = mem::take(&mut self.tests);
-            noop_visit_mod(&mut module, self);
-            let mut tests = mem::replace(&mut self.tests, tests);
+        let old_span = self.parent_span;
+        let old_module_id = self.module_id;

-            if !tests.is_empty() {
-                let parent =
-                    if item.id == ast::DUMMY_NODE_ID { ast::CRATE_NODE_ID } else { item.id };
+        if let ast::ItemKind::Mod(ref module) = item.kind {
+            self.module_id =
+                if item.id == ast::DUMMY_NODE_ID { self.module_id } else { item.id };
+            self.parent_span = module.inner;
+        }
+
+        let tests = mem::take(&mut self.tests);
+        noop_visit_item_kind(&mut item.kind, self);
+        let mut tests = mem::replace(&mut self.tests, tests);
+        if !tests.is_empty() {
+            for test in &mut tests {
                 // Create an identifier that will hygienically resolve the test
                 // case name, even in another module.
                 let expn_id = self.cx.ext_cx.resolver.expansion_for_ast_pass(
-                    module.inner,
+                    test.parent_span,
                     AstPass::TestHarness,
                     &[],
-                    Some(parent),
+                    Some(test.module_id),
                 );
-                for test in &mut tests {
-                    // See the comment on `mk_main` for why we're using
-                    // `apply_mark` directly.
-                    test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
-                }
-                self.cx.test_cases.extend(tests);
+
+                // See the comment on `mk_main` for why we're using
+                // `apply_mark` directly.
+                test.ident.span = test.ident.span.apply_mark(expn_id, Transparency::Opaque);
             }
-            item.kind = ast::ItemKind::Mod(module);
+            self.cx.test_cases.extend(tests);
         }
+
+        self.parent_span = old_span;
+        self.module_id = old_module_id;
+
         smallvec![P(item)]
     }

+    fn visit_block(&mut self, b: &mut P<ast::Block>) {
+        let old_span = self.parent_span;
+        let old_module_id = self.module_id;
+
+        self.parent_span = b.span;
+        self.module_id =
+            if b.id == ast::DUMMY_NODE_ID { self.module_id } else { b.id };
+        noop_visit_block(b, self);
+
+        self.parent_span = old_span;
+        self.module_id = old_module_id;
+    }
+
     fn visit_mac(&mut self, _mac: &mut ast::Mac) {
         // Do nothing.
     }
@@ -234,7 +257,13 @@ fn generate_test_harness(
         test_runner,
     };

-    TestHarnessGenerator { cx, tests: Vec::new() }.visit_crate(krate);
+    TestHarnessGenerator {
+        cx,
+        tests: Vec::new(),
+        module_id: ast::CRATE_NODE_ID,
+        parent_span: krate.span,
+    }
+    .visit_crate(krate);
 }

 /// Creates a function item for use as the main function of a test build.

Panic+abort:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', /usr/local/google/home/mjbshaw/Projects/rust/src/librustc/hir/map/definitions.rs:347:9
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: rustc_driver::report_ice
   6: std::panicking::rust_panic_with_hook
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic
  10: rustc_resolve::macros::<impl rustc_expand::base::Resolver for rustc_resolve::Resolver>::expansion_for_ast_pass
  11: <rustc_builtin_macros::test_harness::TestHarnessGenerator as syntax::mut_visit::MutVisitor>::flat_map_item
  12: <alloc::vec::Vec<T> as syntax::util::map_in_place::MapInPlace<T>>::flat_map_in_place
  13: syntax::mut_visit::noop_visit_item_kind
  14: <rustc_builtin_macros::test_harness::TestHarnessGenerator as syntax::mut_visit::MutVisitor>::flat_map_item
  15: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  16: std::panicking::try::do_call
  17: __rust_maybe_catch_panic
  18: syntax::mut_visit::noop_visit_crate
  19: <rustc_builtin_macros::test_harness::TestHarnessGenerator as syntax::mut_visit::MutVisitor>::visit_crate
  20: rustc_builtin_macros::test_harness::inject
  21: rustc_session::utils::<impl rustc_session::session::Session>::time
  22: rustc_interface::passes::configure_and_expand_inner
  23: rustc_interface::passes::configure_and_expand::{{closure}}
  24: rustc_data_structures::box_region::PinnedGenerator<I,A,R>::new
  25: rustc_interface::passes::configure_and_expand
  26: rustc_interface::queries::Queries::expansion
  27: rustc_interface::queries::Queries::prepare_outputs
  28: rustc_interface::queries::Queries::global_ctxt
  29: rustc_interface::interface::run_compiler_in_existing_thread_pool
  30: scoped_tls::ScopedKey<T>::set
  31: syntax::attr::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.43.0-dev running on x86_64-unknown-linux-gnu

note: compiler flags: -Z unpretty=hir

query stack during panic:
end of query stack
[1]    144436 abort      RUST_BACKTRACE=1 ./build/x86_64-unknown-linux-gnu/stage2/bin/rustc  -O  --tes