Add "modes" to compiletest, for running all tests with NLL enabled and comparing with master

[pnkfelix]

The NLL team has been trying to find a good workflow for evaluating the discrepancies between AST borrowck and NLL-based borrowck.

• Past workflows have developed and used things like -Z borrowck=compare and 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:

• "how many test cases does NLL deviate from AST borrowck",
• "what do the deviations look like", or
• "does this deviation represent a bug that needs to be fix? Or is it an improvement on the AST borrowck?"

Here's a new proposal for a (hopefully) relatively small change to compiletest that should yield an easier workflow to answer questions like those above, at least for the ui/ subset of our tests.

1. Context: The 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.)
2. Add a "mode" argument to 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 tells compiletest to pass -Z nll in addition to any other flags when invoking rustc, at least for the ui/ tests).
3. When running under a particular mode, if there is $file.$mode.stderr file, then this file will be used as the source of "acceptable output". If there is no such file, then compiletest will fallback to the regular filename $file.stderr.
4. One complication: UI tests also include "inline" comments of the form //~ ERROR that 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 //~ ERROR annotations when running in a particular mode. We will still see the errors that occur from the stderr output, it's just less convenient.
5. To ensure that we are tracking discrepancies somewhere, whenever there is a $file.$mode.stderr, then some tool (probably compiletest, but maybe tidy?) will be responsible to checking that $file.rs somewhere 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 num

or 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:

• To find the cases that have discrepancies for nll, one can use ls *.nll.stderr
• To find what a given discrepancy looks like, one can use diff onetest.stderr onetest.nll.stderr
• To see if a discrepancy is a bug or not, grep for FIXME or YAYME in the .rs files.
  • This workflow is perhaps not ideal; @nikomatsakis has pointed out that it might be nicer if these comments somehow lived in the *.$mode.stderr files.

Open Questions (to be resolved by implementor)

• What should compiletest do about occurrences of //~ ERROR in the source text? In particular, should it check that the error output still has those cases, even when running under a given mode?
  • The current inclination of @pnkfelix and @nikomatsakis is that it is actually okay for compiletest to ignore the //~ ERROR annotations when running under a given mode. The reasoning here is this: the //~ ERROR annotations 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/YAYME issue, and we want to allow them to disappear or differ.

[nikomatsakis]

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.

[nikomatsakis]

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 nll or whatever. The compile test harness as a configuration that is loaded here:

rust/src/tools/compiletest/src/main.rs

Line 58 in fedce67

let config = parse_config(env::args().collect());

So we have to extend parse_config with some new option --compare-mode or whatever that takes a string.

rust/src/tools/compiletest/src/main.rs

Line 68 in fedce67

pub fn parse_config(args: Vec<String>) -> Config {

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

fn make_compile_args(&self, input_file: &Path, output_file: TargetLocation) -> Command {

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

	let expected_stderr_path = self.expected_output_path(UI_STDERR);
	let expected_stderr = self.load_expected_output(&expected_stderr_path);
	let expected_stdout_path = self.expected_output_path(UI_STDOUT);
	let expected_stdout = self.load_expected_output(&expected_stdout_path);

Right now, if the revision is Some, then we look for $test.$rev.stderr, I think we want to look for $test.$rev.$mode.stderr in that case, so that we can always do ls *.nll.stderr to get all things with a given mode. (Or maybe we should make it .mode_$mode.stderr or 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 //~ ERROR comments, 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

	if !expected_errors.is_empty() || !proc_res.status.success() {
	// "// error-pattern" comments
	self.check_expected_errors(expected_errors, &proc_res);
	} else if !self.props.error_patterns.is_empty() || !proc_res.status.success() {
	// "//~ERROR comments"
	self.check_error_patterns(&proc_res.stderr, &proc_res);
	}

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.

[kennytm]

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.

[pnkfelix]

@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 compiletest between the support for revisions and that for modes, but in terms of UI, the interface is different here.)

[nikomatsakis]

Also, @kennytm, I opened this issue regarding extending revisions to UI tests (#48878). That said, I think they already work, and we're just not using them. I've got to do some local tests.

[nikomatsakis]

I 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 lexical and nll modes, for example -- in those cases, if we have a revision named nll and we are in comparison mode nll, it'd be sort of nice to just ignore the test, or do something smart. But I guess we can leave that for later.

[memoryleak47]

I'd like to try this one. :)

[nikomatsakis]

@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.)

[pnkfelix]

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.stderr file 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=nll against the ui tests on some dedicated target like linux.

[pnkfelix]

Tidy fix is #49844

[pnkfelix]

Once #49900 lands, this issue will be almost completely resolved; I think the only task I could imagine beyond that would be to expand #49900 (and also compare-mode?) to work on all the tests, not just the ones in src/test/ui.

[nikomatsakis]

I think we can call this done for now until we have a concrete extension in mind.

[pnkfelix]

namely @nikomatsakis and @pnkfelix agreed that any remaining work here is best spent on resolving #44844 (plus porting the run-pass/ tests to ui/, which should probably be filed as another issue).

[nikomatsakis]

In particular closed in favor of #44844