Repository navigation
Add "modes" to compiletest, for running all tests with NLL enabled and comparing with master #48879
Description
Activity
I'm marking this as belonging to both @rust-lang/wg-compiler-nll and @rust-lang/wg-traits -- this is because we are both going to need this tool! In general, having a mode like this will be useful anytime that we plan to make major alterations to some core component of the system.
- changed the title
[-]Add rustc NLL mode to compiletest, for driving comparisons between borrowck modes[/-][+]Add "modes" to compiletest, for running all tests with NLL enabled and comparing with master[/+]on Mar 9, 2018 Mentoring instructions
I'm going to assume we will use a command line option to drive this new mode. Let's call "compare-mode". Then we can run the tests with
./x.py test foo --test-args --compare-mode nllor whatever. The compile test harness as a configuration that is loaded here:rust/src/tools/compiletest/src/main.rs
Line 58 in fedce67
So we have to extend
parse_configwith some new option--compare-modeor whatever that takes a string.rust/src/tools/compiletest/src/main.rs
Line 68 in fedce67
That comparison mode will be translated to some arguments to rustc. Those arguments are arranged in this function:
rust/src/tools/compiletest/src/runtest.rs
Line 1584 in fedce67
Probably it's best we allow a mapping between the comparison mode and the arguments (that mapping can just be hardcoded into this function though). For example, NLL at least currently really wants three flags added:
match self.config.comparison_mode { Some(ComparisonMode::NLL) => { rustc.args(&["-Znll", "-Zborrowck=mir", "-Ztwo-phase-borrows"]); } None => { } }This is where the code currently finds the expected files. We'll want to alter these functions to take the comparison mode into account. That's probably a new option to be added, along with revision.
rust/src/tools/compiletest/src/runtest.rs
Lines 2504 to 2508 in fedce67
Right now, if the revision is
Some, then we look for$test.$rev.stderr, I think we want to look for$test.$rev.$mode.stderrin that case, so that we can always dols *.nll.stderrto get all things with a given mode. (Or maybe we should make it.mode_$mode.stderror something more distinctive?)Anyway, we'll have to alter this to search for the expected output with the given mode and -- if not found -- to then try with a mode of
None.Finally, to skip checking for
//~ ERRORcomments, if a comparison mode is set, we just don't do any of this stuff:rust/src/tools/compiletest/src/runtest.rs
Lines 2548 to 2554 in fedce67
Uuuuh at that point we are basically done with the compiletest changes, right?
Then the last step is to alter tidy to enforce the FIXME convention, but I don't know much about that. We'll figure that out when we get there.
In compiletest there is already the concept of "revisions" which y'all are already using in compile-fail tests. I think it's better we extend this to support UI tests than layering an NLL-specific "modes" on top of it.Edit: Disregard, that was #48878.
@kennytm this is different interface from revisions. Revisions are encoded with flags on a test by test basis. A mode is given more privileged status within
compiletest. We explicitly do not want to add a revision for every test to represent running with and without NLL.(There's some chance that code might be shared within
compiletestbetween the support for revisions and that for modes, but in terms of UI, the interface is different here.)Reacted by kennytmI think though we will eventually want to specify the intersection of the "comparison mode" with a revision. There are a lot of tests that are already encoded with
lexicalandnllmodes, for example -- in those cases, if we have a revision namednlland we are in comparison modenll, it'd be sort of nice to just ignore the test, or do something smart. But I guess we can leave that for later.1 remaining item
@memoryleak47 that would be super duper. @pnkfelix and I were just saying that this remains pretty high priority. Let me know if I can help in any way. (Of course there are already mentoring instructions.)
- added a commit that references this issue
on Apr 6, 2018 I'm going to actually reopen this issue as a way to track when we've gotten this compare-mode into a state where we actually have files checked in that we are referencing against.
(I just discovered that tidy complains about a
augemented-assignments.nll.stderrfile that I had lying around, so we clearly cannot yet reach that point without some more work.)It might be good to even keep this issue open until we have bors set up to be running
compare-more=nllagainst the ui tests on some dedicated target like linux.- added a commit that references this issue
on Apr 11, 2018 - added a commit that references this issue
on Apr 19, 2018 I think we can call this done for now until we have a concrete extension in mind.
namely @nikomatsakis and @pnkfelix agreed that any remaining work here is best spent on resolving #44844 (plus porting the
run-pass/tests toui/, which should probably be filed as another issue).In particular closed in favor of #44844
The NLL team has been trying to find a good workflow for evaluating the discrepancies between AST borrowck and NLL-based borrowck.
-Z borrowck=compareand the https://github.com/pnkfelix/nll-probe tool, but here I'm going to try to avoid a deep dive into how those have failed to satisfy our needs.Our goal here: We want a way to track the current state of NLL, including the cases where there are known deviations, in a manner that allows immediate analysis of that state to determine things like:
Here's a new proposal for a (hopefully) relatively small change to
compiletestthat should yield an easier workflow to answer questions like those above, at least for theui/subset of our tests.ui/tests are set up so that each test consists of an input$file.rs, and a set of expected outputs in$file.stderr(compilation error messages) and$file.stdout(non erroneous compiler output; rarely used). (For more info, see this chapter in the rustc-guide.)compiletest, encoded either as a command line parameter or as an environment variable, or both. (The first mode we'll support will be "nll", which tellscompiletestto pass-Z nllin addition to any other flags when invokingrustc, at least for theui/tests).$file.$mode.stderrfile, then this file will be used as the source of "acceptable output". If there is no such file, thencompiletestwill fallback to the regular filename$file.stderr.//~ ERRORthat indicate what error is expected on which line (these are mildly redundant with the stderr files above). Because messages may differ in the mode M, but we don't want to edit the sources too much, compiletest should just ignore the//~ ERRORannotations when running in a particular mode. We will still see the errors that occur from the stderr output, it's just less convenient.$file.$mode.stderr, then some tool (probablycompiletest, but maybetidy?) will be responsible to checking that$file.rssomewhere contains a comment that explains the source of the discrepancy. This could be a specially formatted FIXME, if the new behavior seems worse than before:// $mode FIXME(#123) -- summary of discrepancy caused by NLL bug with corresponding issue numor a "YAYME" comment for when the new behavior is an improvement:
// $mode YAYME(#123) -- summary of a beneficial discrepancy(presumably the ticket linked by YAYME would just be the tracking issue for NLL or whatever other mode is being tested).
Benefits of the proposed system:
ls *.nll.stderrdiff onetest.stderr onetest.nll.stderr*.$mode.stderrfiles.Open Questions (to be resolved by implementor)
compiletestdo about occurrences of//~ ERRORin the source text? In particular, should it check that the error output still has those cases, even when running under a given mode?compiletestto ignore the//~ ERRORannotations when running under a given mode. The reasoning here is this: the//~ ERRORannotations will already get checked by compiletest runs that don't have a mode. We probably don't want to force an error when there's a discrepancy when running under a given mode; any discrepancies, including any of those errors disappearing, should be accounted for in the linked// $mode FIXME/YAYMEissue, and we want to allow them to disappear or differ.