Add src/test/ui regression testing for NLL #49900
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(Hmm it seems like the output from NLL may have changed even in the short time period between now and when I created #49861 ! Good thing we're putting this in now!) |
|
Whenever this gets r+'ed, it probably needs to have p=10 or something because clearly these .nll.stderr files can go stale very quickly |
Closed
1 task
|
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.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 |
|
(hmm, interesting; I had assumed that when I pushed the new commit, that would override the build associated with its parent commit and thus we wouldn't see any log for the build failure associated with that. Clearly that assumption was incorrect...) |
|
@pnkfelix I am a bit nervous about the effect on bors cycle time. NLL is not known to be especially fast right now, and we'll also be running the tests more often. @rust-lang/infra, can we test that somehow? |
|
We could selectively enable this only on one fast image such as |
|
If we're fine with the implementation let's just land it and we can go from there; I don't think there's a good way to analyze potential impact on cycle time before landing. |
|
Hmm, I just realized that
(I had gotten warnings in my prototyping saying that it was running for over 60 seconds, but I was silly and ignored them, thinking that it might be some issue with my local setup.) I don't think I can land this until at least that is resolved. |
kennytm
added
S-blocked
|
Blocked by #49998. |
|
☔ The latest upstream changes (presumably #49972) made this pull request unmergeable. Please resolve the merge conflicts. |
E.g. when running with `--compare-mode=nll`, then each test line will look like e.g.: ``` test [ui (nll)] ui/issue-10969.rs ... ok ```
…ect current reality.
…l/nll/ in messages.
…`.stderr` files.
…ating of cause map. This seems to avoid poor scaling on src/test/ui/span/dropck_vec_cycle_checked.rs
…imal-causes` flag
This commit only applies the flag to the one test case,
ui/span/dropck_vec_cycle_checked.rs, that absolutely needs it. Without
the flag, that test takes an unknown amount of time (greater than 1
minute) to compile. But its possible that other tests would also
benefit from the flag, and we may want to make it the default (after
evaluating its impact on other tests).
In terms of its known impact on other tests, I have only evaluated the
ui tests, and the *only* ui test I have found that the flag impacts
(running under NLL mode, of course), is src/test/ui/nll/issue-31567.rs
In particular:
```
% ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/nll/issue-31567.rs
error[E0597]: `*v.0` does not live long enough
--> ../src/test/ui/nll/issue-31567.rs:22:26
|
22 | let s_inner: &'a S = &*v.0; //~ ERROR `*v.0` does not live long enough
| ^^^^^ borrowed value does not live long enough
23 | &s_inner.0
24 | }
| - borrowed value only lives until here
|
note: borrowed value must be valid for the lifetime 'a as defined on the function body at 21:1...
--> ../src/test/ui/nll/issue-31567.rs:21:1
|
21 | fn get_dangling<'a>(v: VecWrapper<'a>) -> &'a u32 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
For more information about this error, try `rustc --explain E0597`.
% ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc ../src/test/ui/nll/issue-31567.rs -Z nll-subminimal-causes
error[E0597]: `*v.0` does not live long enough
--> ../src/test/ui/nll/issue-31567.rs:22:26
|
22 | let s_inner: &'a S = &*v.0; //~ ERROR `*v.0` does not live long enough
| ^^^^^ borrowed value does not live long enough
23 | &s_inner.0
24 | }
| -
| |
| borrowed value only lives until here
| borrow later used here, when `v` is dropped
error: aborting due to previous error
For more information about this error, try `rustc --explain E0597`.
%
```
….nll.stderr`
This allows easy revision of the update-references.sh script (included
here) so that it can update the expected output for nll rather than
stderr. It also reminds the rustc developer via the filename that they
are looking at output generated under comapre-mode=nll.
One could argue that there is still a problem with the strategy encoded here:
if we reach a scenario where a change to the compiler brings the output
under AST and NLL modes back into sync, this code will continue to still
generate output to distinct `foo.stderr` and `foo.nll.stderr` files, and
will continue to copy those two files back to corresponding distinct
files in the source tree, even if the *content* of the two files is now the
same.
* Arguably the "right thing" to do in that case is to remove the
`foo.nll.stderr` file entirely.
* However, I think the real answer is that we will probably want to
double-check such cases by hand anyway. We should be regularly
double-checking the diffs between `foo.stderr` and
`foo.nll.stderr`, and if we see a zero-diff case, then we should
evaluate whether that is correct, and if so, remove the file by
hand.)
* In any case, I think the default behavior encoded here (or at
least *intended* to be encoded here) is superior to the
alternative of *only* generating a `foo.nll.stderr` file if one
already existed in the source tree at the time that `compiletest`
was invoked (and otherwise unconditionally generating a
`foo.stderr` file, as was the behavior prior to this commit),
because that alternative is more likely to cause rustc developers
to overwrite a `foo.stderr` file with the stderr output from a
compare-mode=nll run, which will then break the *normal*
`compiletest` run and probably be much more confusing for the
average rustc developer.
pnkfelix
force-pushed
the
compare-mode-nll-followup-3
branch
from
cf84091 to
33bcb4e
Compare
|
@bors r=nikomatsakis p=10 |
|
📌 Commit 33bcb4e has been approved by |
bors
added
S-waiting-on-bors
|
Also, to be clear: I am landing this based on earlier discussions with @nikomatsakis where he essentially said "if the infrastructure team is okay with us landing this, then we should do so, with high priority to avoid test rot", and my understanding from the infrastructure team is that we should go with a "attempt to land it, because there's no reasonable way for us to evaluate the performance impact without landing it." |
bors
added a commit
that referenced
this pull request
Apr 19, 2018
…tsakis Add src/test/ui regression testing for NLL This PR changes `x.py test` so that when you are running the `ui` test suite, it will also always run `compiletest` in the new `--compare-mode=nll`, which just double-checks that when running under the experimental NLL mode, the output matches the `<source-name>.nll.stderr` file, if present. In order to reduce the chance of a developer revolt in response to this change, this PR also includes some changes to make the `--compare-mode=nll` more user-friendly: 1. It now generates nll-specific .stamp files, and uses them (so that repeated runs can reuse previously cached results). 2. Each line of terminal output distinguishes whether we are running under `--compare-mode=nll` by printing with the prefix `[ui (nll)]` instead of just the prefix `[ui]`. Subtask of #48879
|
☀️ Test successful - status-appveyor, status-travis |
|
@pnkfelix I'm a little confused by something. This change made it so that rust/src/tools/compiletest/src/runtest.rs Lines 2883 to 2885 in 8830a03 Both If not, I can update it in #49812 if you'd like. EDIT: Also, it doesn't seem to be working as expected for me. If I start with a test that has a difference with NLL (like rust/src/tools/compiletest/src/runtest.rs Lines 2820 to 2822 in 8830a03 |
|
(also, I recognize that I made matters much worse here because I didn't request a review of the newer changes I had made, which included the bug I injected here. I'm sorry and I will try to do better; regression testing a feature like NLL, while important, was certainly not worth breaking a critical piece of infrastructure.) On the plus side, hopefully the problem injected by my bug won't cause too much trouble in the short term, since people should hopefully notice that their stderrs are getting overwritten. |
|
@ehuss As for this other note you wrote in your comment:
I did not attempt to address this in PR #50117 (my fix for #50113). I know there were trade-offs I was considering when I wrote this (see discussion attached to commit 33bcb4e ), but I didn't realize I had gotten things quite that bad. (But obviously I didn't realize a lot of problems in what I had done.) (That's my long-winded way of saying: "There probably is more work to do here." I won't be able to address it for at least a week. I won't be hurt if someone comes in and either fixes the code, or rips out my attempt to generate output to a special file under |
|
I think that #49812 fixes both issues. It essentially always uses the fully-qualified filename (includes nll mode when running nll) when writing out the output (which is only written on failure). Assuming the regular mode runs before nll, it seems to do the right thing for me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes
x.py testso that when you are running theuitest suite, it will also always runcompiletestin the new--compare-mode=nll, which just double-checks that when running under the experimental NLL mode, the output matches the<source-name>.nll.stderrfile, if present.In order to reduce the chance of a developer revolt in response to this change, this PR also includes some changes to make the
--compare-mode=nllmore user-friendly:--compare-mode=nllby printing with the prefix[ui (nll)]instead of just the prefix[ui].Subtask of #48879