Purge #[!resolve_unexported] from the compiler #15847

sfackler · 2014-07-21T05:14:17Z

This rewrites the test harness generation to create modules that reexport things as needed to make sure all tests are visible.

We now build up a set of modules that reexport everything the test framework needs, instead of turning off privacy.

sfackler · 2014-07-21T05:21:53Z

One note: the __test_reexports and __test module names should really be gensym'd to avoid clashes, but I ended up with resolve errors after giving it a try.

lilyball · 2014-07-21T05:26:22Z

👍

And yes, I was just thinking that __test and __test_reexports needs to be gensymmed as well. But we've been living with __test for long enough that it's not a big issue, we can solve that later.

lilyball · 2014-07-21T05:27:58Z

src/librustc/front/test.rs

+            *i = nomain(*i);
+        }
+        mod_folded.items.push(mk_reexport_mod(&mut self.cx, reexports));
+        self.cx.reexports.push(self.cx.path.clone());


This is creating a reexport mod for every single existing mod, right? Seems like it can avoid creating the reexport mods for mods that don't need any reexports.

huonw · 2014-07-21T05:28:27Z

Could you show the output of --test --pretty expanded (and/or what you're expecting it to be) for something simple like

mod foo {
    mod bar {
        #[test] fn bar_test() {}
    }
    #[test] fn foo_test() {}
}
#[test] fn global_test() {}

?

(It's just easier to review AST generation if you know approximately what the goal is.)

lilyball · 2014-07-21T05:29:23Z

src/librustc/front/test.rs

                        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());


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).

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.

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).

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.

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.

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.

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.)

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.

sfackler · 2014-07-21T05:30:13Z

@huonw sure:

#![feature(phase)]
#![no_std]
#![feature(globs)]
#[phase(plugin, link)]
extern crate std;
extern crate native;
use std::prelude::*;
mod foo {
    use std::prelude::*;
    mod bar {
        use std::prelude::*;
        #[test]
        fn bar_test() { }
        pub mod __test_reexports {
            use std::prelude::*;
            pub use foo::bar::bar_test;
        }
    }
    #[test]
    fn foo_test() { }
    pub mod __test_reexports {
        use std::prelude::*;
        pub use foo::bar;
        pub use foo::foo_test;
    }
}
#[test]
fn global_test() { }
pub mod __test_reexports {
    use std::prelude::*;
    pub use foo;
    pub use global_test;
}
pub mod __test {
    extern crate test;
    use std::prelude::*;
    pub fn main() {
        #![main]
        use std::slice::Vector;
        test::test_main_static(::std::os::args().as_slice(), TESTS);
    }
    pub static TESTS: &'static [self::test::TestDescAndFn] =
        &[self::test::TestDescAndFn{desc:
                                        self::test::TestDesc{name:
                                                                 self::test::StaticTestName("foo::bar::bar_test"),
                                                             ignore: false,
                                                             should_fail:
                                                                 false,},
                                    testfn:
                                        self::test::StaticTestFn(::__test_reexports::foo::__test_reexports::bar::__test_reexports::bar_test),},
          self::test::TestDescAndFn{desc:
                                        self::test::TestDesc{name:
                                                                 self::test::StaticTestName("foo::foo_test"),
                                                             ignore: false,
                                                             should_fail:
                                                                 false,},
                                    testfn:
                                        self::test::StaticTestFn(::__test_reexports::foo::__test_reexports::foo_test),},
          self::test::TestDescAndFn{desc:
                                        self::test::TestDesc{name:
                                                                 self::test::StaticTestName("global_test"),
                                                             ignore: false,
                                                             should_fail:
                                                                 false,},
                                    testfn:
                                        self::test::StaticTestFn(::__test_reexports::global_test),}];
}

lilyball · 2014-07-21T05:31:46Z

@sfackler The reexport fold happens before the std-inject fold? I don't suppose it could be moved after, could it? As those __test_rexport mods have no need for the prelude glob.

sfackler · 2014-07-21T05:35:14Z

I could reorder the phases, or just tag the reexport modules with #[no_prelude].

lilyball · 2014-07-21T05:35:56Z

If reordering the phases is simple, that would be nice because it means the std-inject fold would simply have fewer modules to fold over (which means less work). Otherwise, #[no_prelude] would be better than nothing.

lilyball · 2014-07-21T05:40:34Z

src/librustc/front/test.rs

+        if !reexports.is_empty() {
+            mod_folded.items.push(mk_reexport_mod(&mut self.cx, reexports));
+        }
+        self.cx.reexports.push(self.cx.path.clone());


You're still always pushing the current module, even if there are no test functions in it. The current module should be pushed if there are reexports or if it contains test functions. Otherwise you're pushing every single module.

I'd still like to see public modules not get pushed, as described before, if you're willing to accept the necessary complication in test path calculation (which really would just be testing if the specified item is public or private before inserting the __test_reexports interstitial mod).

Unlike the other thread on reexporting public items, I think it is worth not reexporting modules that don't have any reexports/tests. That doesn't add any complication to the test description calculation.

lilyball · 2014-07-21T06:04:46Z

The top-level __test_reexports mod needs to not be pub. The test runner certainly doesn't need it to be public, and making it public will incorrectly squelch valid dead_code warnings (as it will cause every item reachable from it to be considered crate-exported and therefore no longer dead).

lilyball · 2014-07-21T06:46:04Z

At the moment, the following are my remaining concerns:

Top-level __test_reexports should not be pub. (comment)
Test folder should be reordered after std-inject, or alternatively __test_reexports mods should be marked #[no_prelude] (comment)

Once those are done, then LGTM.

sfackler · 2014-07-21T07:02:16Z

The dead code issue seems like a bug in that lint. If rustc's building an executable, then pub items shouldn't be treated as exported since you can't link to an executable.

lilyball · 2014-07-21T07:03:04Z

@sfackler Perhaps, although the ability to squelch dead_code warnings in an executable by marking them pub is extremely useful for writing compilation tests.

lilyball · 2014-07-21T07:04:23Z

@sfackler Actually, it would be a huge mistake to stop considering those exported. The test runner for every single library would start emitting large numbers of dead_code warnings on exported items that are never called from any tests.

lilyball · 2014-07-21T07:27:27Z

This PR obsoletes #15115.

alexcrichton · 2014-07-21T15:17:15Z

This looks awesome, thanks @sfackler! I wonder if this is at the point where we could move it out of the compiler entirely and make testing a syntax extension...

sfackler · 2014-07-21T20:37:19Z

@alexcrichton An implementation may be blocked on #15778. It would also need to run after all other expansion, which is the real killer. I would like to move in this direction, since it would ensure that it'd be possible to create third party testing frameworks.

feat: preview adt field when hover Closes rust-lang#13977 ![20231108194345_rec_](https://github.com/rust-lang/rust-analyzer/assets/14040068/95894c4b-de6e-4ca4-98b3-6ab4559d0950)

sfackler added 2 commits July 20, 2014 18:05

Remove useless RefCells

e5cc221

Restructure test harness

bca53e9

We now build up a set of modules that reexport everything the test framework needs, instead of turning off privacy.

lilyball reviewed Jul 21, 2014
View reviewed changes

sfackler added 2 commits July 20, 2014 23:35

Purge !resolve_unexported

1d522a6

Don't create reexport module if there are none

173dcec

lilyball mentioned this pull request Jul 21, 2014

#[!resolve_unexported] => #[resolve_unexported] to fix pretty printing #15115

Closed

alexcrichton closed this in 414862d Jul 21, 2014

mvdnes mentioned this pull request Jul 23, 2014

visible_private_types warning when compiling with with test #15912

Closed

sfackler deleted the de-hack-test branch November 26, 2016 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purge #[!resolve_unexported] from the compiler #15847

Purge #[!resolve_unexported] from the compiler #15847

sfackler commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball Jul 21, 2014

sfackler Jul 21, 2014

huonw commented Jul 21, 2014

lilyball Jul 21, 2014

sfackler Jul 21, 2014

huonw Jul 21, 2014

lilyball Jul 21, 2014

huonw Jul 21, 2014

lilyball Jul 21, 2014

huonw Jul 21, 2014

lilyball Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball Jul 21, 2014

lilyball Jul 21, 2014

sfackler Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

sfackler commented Jul 21, 2014

Purge #[!resolve_unexported] from the compiler #15847

Purge #[!resolve_unexported] from the compiler #15847

Conversation

sfackler commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Jul 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

sfackler commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

lilyball commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

sfackler commented Jul 21, 2014