Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reexport tests without polluting namespaces #52890

Merged
merged 6 commits into from
Aug 2, 2018

Conversation

djrenren
Copy link
Contributor

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.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:14] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:14] tidy error: /checkout/src/test/run-pass/issue-52557.rs: missing trailing newline
[00:04:16] some tidy checks failed
[00:04:16] 
[00:04:16] 
[00:04:16] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:16] 
[00:04:16] 
[00:04:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:16] Build completed unsuccessfully in 0:00:59
[00:04:16] Build completed unsuccessfully in 0:00:59
[00:04:16] make: *** [tidy] Error 1
[00:04:16] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0cd75afa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0a896663:start=1533004435080686603,finish=1533004435088991190,duration=8304587
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1a039f92
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1682e290
travis_time:start:1682e290
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0bb3e7e4
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:42:36] 
[00:42:36] - error: cannot test inner function
[00:42:36] -   --> $DIR/test-inner-fn.rs:15:5
[00:42:36] -    |
[00:42:36] - LL |     #[test] //~ ERROR cannot test inner function [unnameable_test_functions]
[00:42:36] -    |
[00:42:36] -    = note: requested on the command line with `-D unnameable-test-functions`
[03625020 .
2274628 ./obj
---
145460 ./obj/build/bootstrap/debug/incremental
133976 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
133972 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
130592 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs
130588 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs/s-f3el8jlfcp-1th506i-3t5kexjst7huj
128816 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
122972 ./obj/build/x86_64-unknow

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov
Copy link
Contributor

Interesting trick.

Actually, can't this work in the opposite way?

#[test]
fn foo() {}

=>

// The function itself is not changed
#[test]
fn foo() {}

// ... but the import is added.
#[some_marker_for_test_harness]
pub use foo as foo_gensym;

This would make the logic cleaner on the expander side, but looks like it would make life harder for the test harness generator since it needs to check function signatures, process should_fail attributes, etc.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment showing the transformation in code, like in #52890 (comment)?

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();
Copy link
Contributor

@petrochenkov petrochenkov Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orig_ident = item.ident;
item.ident = item.ident.gensym();

let use_item = self.cx.item_use_simple_(
item.ident.span,
orig_vis,
Some(Ident::from_interned_str(item.ident.as_interned_str())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some(orig_ident)

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]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keywords::SelfValue.ident()

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2018
@djrenren
Copy link
Contributor Author

djrenren commented Jul 31, 2018

@petrochenkov unfortunately, that change wouldn't work because we can't pub use a non-pub function. This is E0364. There is a variant of your suggestion that turns:

#[test]
fn foo(){}

into:

#[was_public|was_priv]
pub fn foo_gensym(){}

and then insert the appropriate use in test.rs but it's not really worth it in my opinion. It would keep us from tracking nameability though.

The tricky thing is that we don't want to expand all #[test]'s with use statements because this will make inner-fn tests produce spurious errors because:

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

would become

fn foo() {
    #[test]
    fn bar(){}
    use self::bar as bar_gensym;
}

which will fail.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/ae/3a/b30eb9b2e05d0fa102ad6f1544e10129de857a570ee25549665021fa7450/awscli-1.15.68-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 26.7MB/s eta 0:00:01
    1% |▌                               | 20kB 2.0MB/s eta 0:00:01
    2% |▊                               | 30kB 2.3MB/s eta 0:00:01
    3% |█                               | 40kB 2.1MB/s eta 0:00:01
---
[00:54:38]     |
[00:54:38] 552 |     fn test_multiple_int_constants() {
[00:54:38]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:54:38] 
[00:54:38] error: unused import: `test_op_i`
[00:54:38]    --> libterm/terminfo/parm.rs:558:8
[00:54:38] 558 |     fn test_op_i() {
[00:54:38]     |        ^^^^^^^^^
[00:54:38] 
[00:54:38] error: unused import: `test_param_stack_failure_conditions`

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@djrenren
Copy link
Contributor Author

djrenren commented Aug 1, 2018

@petrochenkov Any idea why those changes broke the build?

@petrochenkov
Copy link
Contributor

@djrenren
Are you sure it passed previously?
The generated imports do seem unused except in "the semi-pathological case where you have test functions referencing each other".

@djrenren
Copy link
Contributor Author

djrenren commented Aug 1, 2018

Yeah, the errors make sense, I'm just confused why it passed before...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:57:27] travis_fold:start:test_stage1-term
travis_time:start:test_stage1-term
Testing term stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:27]    Compiling term v0.0.0 (file:///checkout/src/libterm)
[00:57:27] error: unused import: `test_get_dbpath_for_term`
[00:57:27]   --> libterm/terminfo/searcher.rs:80:4
[00:57:27] 80 | fn test_get_dbpath_for_term() {
[00:57:27]    |    ^^^^^^^^^^^^^^^^^^^^^^^^
[00:57:27]    |
[00:57:27]    = note: `-D unused-imports` implied by `-D warnings`
[00:57:27]    = note: `-D unused-imports` implied by `-D warnings`
[00:57:27] 
[00:57:27] error: unused import: `test_veclens`
[00:57:27]    --> libterm/terminfo/parser/compiled.rs:351:8
[00:57:27] 351 |     fn test_veclens() {
[00:57:27]     |        ^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_basic_setabf`
[00:57:27]    --> libterm/terminfo/parm.rs:545:8
[00:57:27] 545 |     fn test_basic_setabf() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_multiple_int_constants`
[00:57:27]    --> libterm/terminfo/parm.rs:552:8
[00:57:27] 552 |     fn test_multiple_int_constants() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_op_i`
[00:57:27]    --> libterm/terminfo/parm.rs:558:8
[00:57:27] 558 |     fn test_op_i() {
[00:57:27]     |        ^^^^^^^^^
[00:57:27] 
[00:57:27] error: unused import: `test_param_stack_failure_conditions`
---
[00:57:27]     |
[00:57:27] 618 |     fn test_push_bad_param() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] error: unused import: `test_comparison_ops`
[00:57:27]    --> libterm/terminfo/parm.rs:623:8
[00:57:27] 623 |     fn test_comparison_ops() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_conditionals`
[00:57:27]    --> libterm/terminfo/parm.rs:642:8
[00:57:27] 642 |     fn test_conditionals() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_format`
[00:57:27]    --> libterm/terminfo/parm.rs:657:8
[00:57:27] 657 |     fn test_format() {
[00:57:27]     |        ^^^^^^^^^^^
[00:57:27] 
[00:57:28] error: aborting due to 10 previous errors
[00:57:28] error: aborting due to 10 previous errors
[00:57:28] 
[00:57:28] error: Could not compile `term`.
[00:57:28] To learn more, run the command again with --verbose.
[00:57:28] 
[00:57:28] 
[00:57:28] 
[00:57:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "term" "--" "--quiet"
[00:57:28] 
[00:57:28] 
[00:57:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:57:28] Build completed unsuccessfully in 0:15:35
[00:57:28] Build completed unsuccessfully in 0:15:35
[00:57:28] Makefile:58: recipe for target 'check' failed
[00:57:28] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0770fe80
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@djrenren
Copy link
Contributor Author

djrenren commented Aug 1, 2018

@petrochenkov Should be all set now.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 77f9aca has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2018
@bors
Copy link
Contributor

bors commented Aug 2, 2018

⌛ Testing commit 77f9aca with merge 02a369a...

bors added a commit that referenced this pull request Aug 2, 2018
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.
@bors
Copy link
Contributor

bors commented Aug 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 02a369a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants