Introduce compile-pass

[ishitatsuyuki]

r? @alexcrichton

The plan is to move things that cannot fail (no assert, unwrap, etc) out so we don't have to run them, and in the long term we can also stop running LLVM for them.

Out of 3215 tests...

Language            Files        Lines         Code     Comments       Blanks
Rust                 3215       119254        64688        35135        19431

16% of them has an empty main (which is already moved in this PR).

grep -rnPzl 'fn main\(\)\s*{\s*}' | xargs rg --files-without-match cfg | wc -l
547

And only 50% of the tests contains assertions:

rg -e assert -e unwrap -e expect -e panic -l | wc -l
1600

The remainder is likely able to get moved, but they need check by a human so I didn't touch them in PR.

cc @rust-lang/compiler

• Update documentation

[Mark-Simulacrum]

The grep for main doesn't quite work because of cfg statements, e.g. https://github.com/rust-lang/rust/blob/master/src/test/run-pass/abi-sysv64-arg-passing.rs shouldn't have been moved but was.

[ishitatsuyuki]

@Mark-Simulacrum Thanks for noticing this. I've updated the numbers and will move those back with the next commit.

[alexcrichton]

Do we have timing information showing that there is a benefit to gain here? Can we be more conservative about moving tests over? I spot checked a few and tests like https://github.com/ishitatsuyuki/rust/blob/a9cc2c4b429d2f002212fbabd217eed3bb48917e/src/test/compile-pass/issue-15487.rs should not move.

[nagisa]

@alexcrichton why should it not move? There’s no information whatsoever to be gained from running that test.

[Zoxc]

@nagisa We want to skip trans/linking for compile-pass. Perhaps we should call it something to indicate that?

It's probably a good idea to add a whitelist of attributes and test flags which should be allowed for compile-pass and only move over tests which passes these. If a test uses link_args or exec_env it's probably wants to stay in run-pass. Should we have check-pass, link-pass and run-pass folders?

[nagisa]

Well, I’d much rather skip linking by specifying specific flags in the test file instead, for now, anyway.

Something like // compile-flags: -Zno-trans ;)

With run-pass, it makes no sense if some of the tests are not run (defeating the idea of a comment that specifies such behaviour), but with compile-pass, running or not running the backend still constitutes a "compile".

[bors]

☔ The latest upstream changes (presumably #49337) made this pull request unmergeable. Please resolve the merge conflicts.

[ishitatsuyuki]

To clarify, --emit=metadata currently miss a few lints we may hit right before LLVM passes, so I'm not going to do that in this PR.

Regarding linkage tests, I'm planning to add a new comment config emit or something like that. It defaults to metadata so we can save time, but we can configure it for tests that require linkage.

[ishitatsuyuki]

Status: waiting on decision. This is going to have a lot of conflicts so I'm not going to rebase until a concluding review.

[oli-obk]

At this point, is there any use in specifically having compile-pass tests instead of moving them to ui? One can also specify --emit-metadata on ui single tests once we want to that

[ishitatsuyuki]

@oli-obk The thing is that compile-pass tests are not UI tests.

From IRC:

ishitatsuyuki But it can be ui with just one line of comment
nagisa_w but then the purpose of the ui test suite gets dilated
ishitatsuyuki We're moving away from compile-fail too so I'm not sure if I should introduce a new one
nagisa_w we're moving from compile fail mostly because majority of the compile fail tests are ui tests.
ishitatsuyuki I see the point. I'll be introducing compile-pass then

[nagisa]

Yeah, I’m fairly in favour of compile-pass and would r+ this, but looking over 512 tests to ensure that none of them got moved by mistake is… a chore. I guess I’ll do that.

[nagisa]

From the short overview of the tests it seems to me that there are about three stages that are important:

• Everything that happens until codegen (i.e. --emit=metadata);
• Everything that happens during codegen;
• Checking the linking step.

None of these 512 tests, really care about being run and most of them, as suspected, fall into the first category. Notable exceptions would be the test pointed out by @alexcrichton above, which falls into the third category (checking the linking). There are also some tests that should’ve been a codegen test in a first place (issue-33992).

So, if we expect compile-pass to be a --emit=metadata by default, these tests should be moved back to either run-pass or a more appropriate location before we land this. We can achieve this either by moving tests to compile-pass piece-meal or rigorously reviewing all the tests.

@alexcrichton what sort of performance numbers are you looking for? Is a local build before/after sufficient? Should we also produce expected gains for when we start using --emit=metadata?

[ishitatsuyuki]

@nagisa Thanks for checking all the tests! However, I don't think that linkage tests belong to run-pass, and instead I'm planning to add a comment flag to control (and override) --emit. Also, I don't plan to move immediately due to potential misses by #49292.

Assuming that all of the moved tests can be classified into the three categories you said, we can probably grep link (for link_args and potentially any related comments) for No. 3, right?

Checking for codegen tests looks a bit harder. For those tests, it may be better to make them into proper codegen tests instead.

[alexcrichton]

@nagisa

er sorry for the delay!

why should it not move?

ah as you discovered in #49321 (comment) this test is intended for linking which --emit metadata will not hit.

what sort of performance numbers are you looking for?

This is sort of yet-another-test-suite to maintain and I'd just want to make sure it's pulling its weight. It looks like the motivation here is to have a faster test suite by avoiding linking/running/codegen of binaries, which makes sense but I'd like to get a sense of scale. For example how long does this new test suite take to finish? How long did it take to finish in run-pass?

[ishitatsuyuki]

@alexcrichton What should I do to get this to progress? Swap out linking tests? Taking some timing data?

[alexcrichton]

Could something like @nagisa's suggestion be used for this instead of a new test suite? With that I think selective tests could opt-in?

I don't think we should apply the flag in a blanket fashion, but we can start reviewing tests to be added to

[ishitatsuyuki]

@alexcrichton How does that compare to a blacklist based approach? Given that most tests are about trait resolution, how about specifying those that really need codegen instead?

[alexcrichton]

@ishitatsuyuki I would not be confident in the conclusion that most tests are about trait resolution, I think 100% of the tests I've ever written require the actual test to run. As a result I'm very wary to start one day ceasing execution of most of our tests.

[nikomatsakis]

I would prefer not to have more "test modes". I have rather been expecting us to consolidate things into the ui directory, and use comments in tests to direct what should happen. For example, you can currently have a compile-pass test by making a file in the ui directory and doing:

// must-compile-sucessfully

you can also add

// run-pass

to actually do the execution, if desired.

[ishitatsuyuki]

@alexcrichton All of the moved tests has an empty main. There's absolutely no meaning in running it, unless we're testing some special linkage.
Omitting codegen should be fine for the same reason, although it should be done in a later PR.

@nikomatsakis The point is to keep the name intuitive. See previous discussion at #49321 (comment).

[alexcrichton]

@ishitatsuyuki yes while empty main may most of the time imply testing the trait system it could also just as equally mean that linkage is being tested or that codegen doesn't panic/abort. I think it needs to be evaluated on a case-by-case basis whether it's appropriate to stop executing tests.

[ishitatsuyuki]

@alexcrichton:

Could something like @nagisa's suggestion be used for this instead of a new test suite? With that I think selective tests could opt-in?

The suggestion you mentioned assumes that compile-pass exists. I think compile-pass and run-pass are two different kind of things. Are you suggesting something like // compile-pass flag? That also works except it makes the run-pass name more confusing.

yes while empty main may most of the time imply testing the trait system it could also just as equally mean that linkage is being tested or that codegen doesn't panic/abort.

With the exception of some auxiliary tests, we only feed dead code into LLVM, or they may be eliminated before even getting into LLVM. Linkage tests are exceptions, and the rest doesn't really test any codegen because there's no real code to transform.

[alexcrichton]

Yeah as @nikomatsakis mentioned we probably want to consolidate everything anyway at some point and move purely towards annotations in the test file to dictate what sort of test it is.

My point about not blanket changing things is that we have tests which do rely on hitting llvm/linking. It's difficult to automatically classify which test is which.

[TimNN]

@alexcrichton / @ishitatsuyuki: How do you want to move forward with this? The open questions I see are

1. Do we want this mode?
2. If yes, where / how do we want it? What kind of options should be available?
3. Based on (2), move existing tests.

Im not sure from the comments, is there a clear answer for (1) & (2)? If so, I would propose to mainly implement (1) & (2) here and then move the individual tests in follow-up PRs with manual review.

[alexcrichton]

I'm personally ok with a mode such as this, and I think we'd just want it as a header like // dont-link-and-run or something like that. We'd then selectively start applying that attribute to tests where possible.

[rust-highfive]

The job x86_64-gnu-llvm-3.9 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:46] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:46] tidy error: /checkout/src/test/run-pass/compiletest-skip-trans.rs: missing trailing newline
[00:04:48] some tidy checks failed
[00:04:48] 
[00:04:48] 
[00:04:48] 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:48] 
[00:04:48] 
[00:04:48] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:48] Build completed unsuccessfully in 0:01:54
[00:04:48] Build completed unsuccessfully in 0:01:54
[00:04:48] Makefile:79: recipe for target 'tidy' failed
[00:04:48] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a979f27
$ 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)

[alexcrichton]

@bors: r+

[bors]

📌 Commit 00bc634 has been approved by alexcrichton

[bors]

⌛ Testing commit 00bc634 with merge 25749ad...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 25749ad to master...

[pnkfelix]

What is/was the intended difference between // skip-trans (now called // skip-codegen thanks to PR #50615) and // compile-pass?

Because I see several instances of // compile-pass, but only one instance of // skip-trans, and the latter appears to just be a supposed test of the feature within compiletest itself.

[pnkfelix]

(Is the idea that // compile-pass would actually run trans, while // skip-trans wouldn't even get that far? Because if that's the case, I don't see how the existing test is actually checking that...)

[ishitatsuyuki]

compile-pass is a hack to compile things that cannot be executed (libs etc). You still want to check linkage in this case. skip-codegen is for tests that does no runtime assertion, and tests things like type inference only.

[eddyb]

IMO we should use compile-pass instead of skip-codegen wherever possible.

[ishitatsuyuki]

@eddyb They are two different concepts; compile-pass should be used only in compile-fail tests (or ui, but strongly against the concept). skip-codegen is solely for run-pass tests that has an empty main or whatever. There's no point in feeding dead IR into LLVM.

[pnkfelix]

There's no point in feeding dead IR into LLVM.

Well, if the dead IR would cause LLVM to signal an error, then that's a good thing to know...

[ishitatsuyuki]

@pnkfelix Yes that theoretically may happen, but:

• Compiling stage2 rustc is a much complete test than just feeding those junk IRs which often contains nothing.
• If testing of certain features is desired they should be written as codegen tests.
• Someday MIR DCE will kick in and that will no longer make sense.

[eddyb]

I strongly prefer run-pass over compile-pass over skip-codegen over #[rustc_error] on fn main and it's mainly to do with the fact that I don't trust anything, having seen some weird issues in the past - the compiler is not as good of a "complete" test as you might assume.

[petrochenkov]

I usually write pass tests for the frontend, and they don't even generate any code except for empty main (+ necessary runtime stuff).
So I'd normally want skip-codegen to avoid growing time to run test suite.

However, if overhead from run-pass compared to skip-codegen is neglible for such "empty" tests (seems unlikely, especially on Windows where running an executable is expensive, but someone needs to measure), then run-pass would of course be more reliable.