must-compile-successfully ignored by ui tests

[nikomatsakis]

From what I can tell, ui tests that don't have any //~ ERROR annotations and have no stderr file are implicitly assumed to require successful compilation. We ought to reject such tests, and instead require an explicit // must-compile-succesfully comment.

Here are some mentoring instructions. The file that controls the test suite is runtest.rs. UI tests in particular are controlled by run_ui_test():

rust/src/tools/compiletest/src/runtest.rs

Lines 2381 to 2448 in ab79caa

	fn run_ui_test(&self) {
	// if the user specified a format in the ui test
	// print the output to the stderr file, otherwise extract
	// the rendered error messages from json and print them
	let explicit = self.props
	.compile_flags
	.iter()
	.any(|s| s.contains("--error-format"));
	let proc_res = self.compile_test();
	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);
	let normalized_stdout =
	self.normalize_output(&proc_res.stdout, &self.props.normalize_stdout);
	let stderr = if explicit {
	proc_res.stderr.clone()
	} else {
	json::extract_rendered(&proc_res.stderr, &proc_res)
	};
	let normalized_stderr = self.normalize_output(&stderr, &self.props.normalize_stderr);
	let mut errors = 0;
	errors += self.compare_output("stdout", &normalized_stdout, &expected_stdout);
	errors += self.compare_output("stderr", &normalized_stderr, &expected_stderr);
	if errors > 0 {
	println!("To update references, run this command from build directory:");
	let relative_path_to_file = self.testpaths
	.relative_dir
	.join(self.testpaths.file.file_name().unwrap());
	println!(
	"{}/update-references.sh '{}' '{}'",
	self.config.src_base.display(),
	self.config.build_base.display(),
	relative_path_to_file.display()
	);
	self.fatal_proc_rec(
	&format!("{} errors occurred comparing output.", errors),
	&proc_res,
	);
	}
	let expected_errors = errors::load_errors(&self.testpaths.file, self.revision);
	if self.props.run_pass {
	let proc_res = self.exec_compiled_test();
	if !proc_res.status.success() {
	self.fatal_proc_rec("test run failed!", &proc_res);
	}
	}
	if !explicit {
	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);
	}
	}
	}

You can see that it begins with a call to compile_test, but it does not inspect the return value from that function. In contrast, compile-fail tests check and -- if compilation is successful -- they require that a must_compile_successfully comment is present:

rust/src/tools/compiletest/src/runtest.rs

Lines 153 to 163 in ab79caa

	if self.props.must_compile_successfully {
	if !proc_res.status.success() {
	self.fatal_proc_rec("test compilation failed although it shouldn't!", &proc_res);
	}
	} else {
	if proc_res.status.success() {
	self.fatal_proc_rec(
	&format!("{} test compiled successfully!", self.config.mode)[..],
	&proc_res,
	);
	}

We want to do something similar, but in the run_ui_test function. This will likely reveal existing tests that need a // must-compile-successfully comment.

[nikomatsakis]

cc @oli-obk, who did the awesome work towards respecting //~ ERROR annotations

[tommyip]

I can work on this!

[nikomatsakis]

@tommyip great =)

[tommyip]

@nikomatsakis Ok so these are the tests that implicitly compiles (42 out of 617 ui tests), some of which contains the flag run-pass.

ui/check_match/issue-43253.rs
ui/codemap_tests/unicode_3.rs
ui/deprecated-macro_escape-inner.rs  // run-pass
ui/deprecated-macro_escape.rs  // run-pass
ui/deriving-meta-empty-trait-list.rs  // run-pass
ui/enum-size-variance.rs  // run-pass
ui/explain.rs
ui/hello_world/main.rs
ui/issue-19100.rs  // run-pass
ui/issue-43806.rs  // run-pass
ui/lint/command-line-lint-group-allow.rs
ui/lint/command-line-lint-group-warn.rs
ui/lint/not_found.rs
ui/lint/unreachable_pub-pub_crate.rs
ui/lint/unreachable_pub.rs
ui/lint/unused_parens_json_suggestion.rs
ui/macros/trace-macro.rs
ui/nll/closure-requirements/propagate-approximated-ref.rs
ui/nll/closure-requirements/propagate-approximated-shorter-to-static-no-bound.rs
ui/nll/closure-requirements/propagate-approximated-shorter-to-static-wrong-bound.rs
ui/nll/closure-requirements/propagate-approximated-val.rs
ui/nll/closure-requirements/propagate-despite-same-free-region.rs
ui/nll/maybe-initialized-drop-uninitialized.rs
ui/path-lookahead.rs  // run-pass
ui/print-fuel/print-fuel.rs
ui/print_type_sizes/anonymous.rs
ui/print_type_sizes/generics.rs
ui/print_type_sizes/multiple_types.rs
ui/print_type_sizes/niche-filling.rs
ui/print_type_sizes/no_duplicates.rs
ui/print_type_sizes/packed.rs
ui/print_type_sizes/padding.rs
ui/print_type_sizes/repr-align.rs
ui/print_type_sizes/uninhabited.rs
ui/print_type_sizes/variants.rs
ui/reachable/expr_andand.rs
ui/reachable/expr_oror.rs
ui/rfc_1940-must_use_on_functions/fn_must_use.rs
ui/span/macro-span-replacement.rs
ui/span/multispan-import-lint.rs
ui/span/unused-warning-point-at-signature.rs  // run-pass
ui/test-should-panic-attr.rs  // run-pass

Should we enforce // must-compile-successfully in the ones with the run-pass flag?

[oli-obk]

I think run-pass should imply the must-compile-successfully flag