Add compiletest --compare-mode nll option

[memoryleak47]

Before implementing the tidy stuff, I'd appreciate if someone reviews the changes so far.
This is my first non-trivial pull request, so I could really use some feedback. :)
closes #48879.

r? @nikomatsakis

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

[nikomatsakis]

@memoryleak47

This is my first non-trivial pull request, so I could really use some feedback. :)

Welcome =)

[nikomatsakis]

This looks really good! It's hard to test compiletest, so I guess I'll try to do a local build.

[memoryleak47]

I'm currently working on the tidy change. Should I open another pull request for that?

[pnkfelix]

One problem from my initial attempt to try out the PR: it seems like it will reuse previous cached results that were not run with --compare-mode nll.

More specifically, if I run compiletest on src/test/run-pass, and then immedately run it a second time, the second will finish much faster and every test will just say " ... ignored" after it, because it is reusing the cached success result.

But the problem is that if I run compiletest on src/test/run-pass, and then immedately run it a second time but this time pass --compare-mode nll, then we should not reuse the cached results (because the input arguments to compiletest differed).

As I write this, I am wondering whether this is just considered part of the compiletest design, in the sense that its users need to know well enough to clear the cache when making changes to the compiletest invocation. So maybe the problem here is "works as designed."

But its possible that we might be better off including the mode somewhere in the path we generate for the files we generate.

• Or again, maybe the user should just know enough to pass a --stage-id parameter to compiletest that includes an encoding of the mode...

[pnkfelix]

After discussion with @nikomatsakis, I think I like another option that he suggested, which I will outline here:

1. For right now, in this PR: make compiletest ignore the stamp files when you run it with --compare-mode
2. Longer term (not in this PR), assuming that we add support for this to x.py, then as part of adding support for this to x.py, we should make compiletest resume respecting the stamp files, and at the same time x.py --compare-mode nll would be responsible for passing an appropriate --stage-id parameter so that the stamp files have the compare mode encoded in their filenames.

[memoryleak47]

Is there a good reason, to not just change the stamp-function to include the compare-mode in the output path? (eg. <path>-<stage-id>-<compare-mode>.stamp)
Also, I'm having trouble running compiletest with --compare-mode nll for debugging, as this doesn't seem to be possible using x.py, and I'm not familiar with using compiletest directly.

[nikomatsakis]

@memoryleak47 sorry for not getting back to you! Last week was Rust All Hands and it was a pretty overwhelming week.

[nikomatsakis]

@memoryleak47

Is there a good reason, to not just change the stamp-function to include the compare-mode in the output path?

There is no good reason =) that probably makes sense. I think the reason @pnkfelix did not suggest that at first is that the stage-id already contains a certain amount of "context" -- i.e., the caller is responsible for keeping it unique -- but it seems fine to have the compile-mode in there as a separate field.

[pnkfelix]

@memoryleak47 regarding your question about running compiletest directly, my usual technique is to run x.py test with --verbose mode (or sometimes -vv for "very verbose" mode) and then copy-pasting the appropriate command line from that output into my own terminal and then editting that command line to add the flags I'm interested in.

[pnkfelix]

@memoryleak47 also I'd argue that one reason to not change the "stamp-function" is just to minimize the impact of this PR... I don't want to risk breaking other people's work-flows for this somewhat isolated use case...

[TimNN]

Your PR failed on Travis. 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.

Resolving deltas: 100% (607705/607705), completed with 4799 local objects.
---
[00:00:44] configure: rust.quiet-tests     := True
---
[00:04:09] tidy error: /checkout/src/tools/compiletest/src/main.rs:626: line longer than 100 chars
[00:04:10] some tidy checks failed
[00:04:10]
[00:04:10]
[00:04:10] 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:10] expected success, got: exit code: 1
[00:04:10]
[00:04:10]
[00:04:10] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:10] Build completed unsuccessfully in 0:01:43
[00:04:10] Makefile:79: recipe for target 'tidy' failed
[00:04:10] make: *** [tidy] Error 1
---
$ cat obj/tmp/sccache.log
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:00c3a60e:start=1522787404361827543,finish=1522787404369620476,duration=7792933
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:257549e8
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:257549e8:start=1522787404376848300,finish=1522787404383568298,duration=6719998
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:04641c62
$ dmesg | grep -i kill
[   11.067144] init: failsafe main process (1095) killed by TERM signal

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.

[memoryleak47]

Alright, running compiletest with --compare-mode should now ignore timestamps.
Already tested that locally using @pnkfelix's trick. Thanks. :)
Testing the ui suite with --compare-mode nll causes a lot of errors, but I guess this is expected behaviour, caused by differences between nll and ast borrowck.

[nikomatsakis]

As a meta comment, this is the kind of thing where I wish we had a testing framework for compiletest (#47606). It always makes me nervous, since a wrong move could mean we are not testing what we think we are testing =)

That said, these changes seem good to me.

So r=me but I'd like @pnkfelix to "sign off" as well, for added security.

[nikomatsakis]

r? @pnkfelix

[pnkfelix]

hmm. right now, even when you are using --compare-mode=nll, we still attempt to be helpful and print the following message when you have a mismatch:

The actual stderr differed from the expected stderr.
Actual stderr saved to /home/pnkfelix/Dev/Mozilla/issue48879/rust-48879/objdir-dbgopt/build/x86_64-unknown-linux-gnu/test/ui/augmented-assignments.stderr
To update references, run this command from build directory:
/home/pnkfelix/Dev/Mozilla/issue48879/rust-48879/src/test/ui/update-references.sh '/home/pnkfelix/Dev/Mozilla/issue48879/rust-48879/objdir-dbgopt/build/x86_64-unknown-linux-gnu/test/ui' 'augmented-assignments.rs'

but of course that script isn't going to the right thing in this case...

[pnkfelix]

also, I don't know if this is exactly relevant to this particular PR, but during my experimentation: When I created an empty augmented-assignments.nll.stderr file, I expected to see a diff that just showed all plusses. (I.e., a diff against an empty file)

But instead it seems like if the revision file is empty, then it falls back on the base file augmented-assignments.stderr, in this particular case...

• (From skimming the PR, I'm not even clear on how we are observing such a semantics... but nonetheless, it is what I am seeing.)

(I don't know whether other people think that fall back behavior for empty files is more intutive... it wasn't intuitive for me, since I'm the kind of person who will just do touch augmented-assignments.nll.stderr and then run the tool to see what the content is that I need to add.)

[pnkfelix]

Note: the prior two comments are more nits about things that we can do in follow-up PR's; they need not block landing this as-is.

[pnkfelix]

@bors r+

[bors]

📋 Looks like this PR is still in progress, ignoring approval

[pnkfelix]

@bors r+

[bors]

📌 Commit 03e1509 has been approved by pnkfelix

[bors]

⌛ Testing commit 03e1509 with merge db4235c...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing db4235c to master...