Don't hide ICEs from previous incremental compiles

[traxys]

I think this fixes #65401, the compiler does not fail to ICE after the first compilation, tested on the last snippet of this comment.
I am not very sure of the fix as I don't understand much of the structure of the compiler.

[rust-highfive]

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

[Centril]

r? @michaelwoerister cc @eddyb

[jackh726]

You should add the mcve in the comment (#63154 (comment)) as a test (as in #65395).

[traxys]

If I understood the issue correctly, the test should be that when rustc -Cincremental is run for the second time the compilation should ICE again, which is currently not the case. I don't know how to test that something should ICE when run twice

[jackh726]

Here's an example of a test specifically for incremental compilation: #60697

[traxys]

The problem is now on how can I make the compiler ICE reliablely ?

[jackh726]

Oh, I misread the issue being solved! I'm not exactly sure the correct test for this.

[pnkfelix]

@jackh726 I can help and will do so tonight or tomorrow

[traxys]

Apart from needing the test, is the fix correct ?

[michaelwoerister]

The fix looks correct to me. Thanks, @traxys!

[michaelwoerister]

Here is some documentation on using "revisions" for test cases: https://rust-lang.github.io/rustc-guide/tests/adding.html#revisions

[pnkfelix]

In addition to @michaelwoerister 's point about using "revisions" for test cases, we also need to check about whether there is a good way to cause the compiler to ICE in a reliable fashion (i.e., something that we could put in a test suite and rely on going forward).

#[rustc_error] might work as a way to get the compiler to ICE, perhaps if you combine it with -Z treat-err-as-bug; but I think in this case you specifically need something that calls delay_span_bug, right? I meant to look at this last week but then got distracted by linker bug!

[traxys]

I tried the #[rustc_error] with -Z treat-err-as-bug and indeed given that it does not trigger a delay_span_bug it does not exhibit the bug on current rustc

[pnkfelix]

@traxys okay. I think the way to go here would be to extend the rustc_error attribute to be able to generate a delayed_span_bug instead of always causing an immediate fatal error.

E.g. something where #[rustc_error] would behave the same as today, but #[rustc_error(delayed)] would call delayed_span_bug instead.

If you want to have a go at implementing this yourself, the main places to look are here:

rust/src/librustc_codegen_utils/lib.rs

Lines 30 to 40 in d3d28a4

	/// check for the #[rustc_error] annotation, which forces an
	/// error in codegen. This is used to write compile-fail tests
	/// that actually test that compilation succeeds without
	/// reporting an error.
	pub fn check_for_rustc_errors_attr(tcx: TyCtxt<'_>) {
	if let Some((def_id, _)) = tcx.entry_fn(LOCAL_CRATE) {
	if tcx.has_attr(def_id, sym::rustc_error) {
	tcx.sess.span_fatal(tcx.def_span(def_id), "compilation successful");
	}
	}
	}

(which implements the actual handling of the attribute)

and here:

rust/src/libsyntax/feature_gate/builtin_attrs.rs

Line 540 in d3d28a4

rustc_attr!(TEST, rustc_error, Whitelisted, template!(Word)),

which implements the initial check that #[rustc_error] always takes the form of a single Word (namely, rustc_error and not rustc_error(stuff)).

Based on my reading of the definition of template!, here:

rust/src/libsyntax/feature_gate/builtin_attrs.rs

Lines 113 to 116 in d3d28a4

	/// A convenience macro for constructing attribute templates.
	/// E.g., `template!(Word, List: "description")` means that the attribute
	/// supports forms `#[attr]` and `#[attr(description)]`.
	macro_rules! template {

it seems like you should be able to get this going initially by just replacing Word up above (in builtin_attrs.rs) with Word, List: "delayed" and work your way out from there.

[traxys]

Should I do this in this PR or in another PR ?

[pnkfelix]

Should I do this in this PR or in another PR ?

I'd say go ahead and do it in this PR, but as a separate commit.

(If this #[rustc_error(delayed)] thing were a more general feature, or if I expected it to require a significant amount of code, then I would argue for a separate PR. But I don't expect it to see too much usage outside of testing this PR.)

[traxys]

@pnkfelix I added something that seems to be correct, the only problem is that it does not trigger the issue with the rustc affected by the issue, and I don't know why, I need to look in more details

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, 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.

2019-10-31T13:47:23.9099729Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-31T13:47:23.9296791Z ##[command]git config gc.auto 0
2019-10-31T13:47:23.9381583Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-31T13:47:23.9444354Z ##[command]git config --get-all http.proxy
2019-10-31T13:47:24.5793443Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65470/merge:refs/remotes/pull/65470/merge
---
2019-10-31T13:53:40.1753075Z    Compiling serde_json v1.0.40
2019-10-31T13:53:41.7251196Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-31T13:53:53.2483739Z     Finished release [optimized] target(s) in 1m 29s
2019-10-31T13:53:53.2566967Z tidy check
2019-10-31T13:53:54.0626443Z tidy error: /checkout/src/librustc_codegen_utils/lib.rs:48: line longer than 100 chars
2019-10-31T13:53:54.0923293Z tidy error: /checkout/src/libsyntax/feature_gate/builtin_attrs.rs:540: line longer than 100 chars
2019-10-31T13:53:55.6399756Z some tidy checks failed
2019-10-31T13:53:55.6401033Z Found 485 error codes
2019-10-31T13:53:55.6401222Z Found 0 error codes with no tests
2019-10-31T13:53:55.6401553Z Done!
2019-10-31T13:53:55.6401553Z Done!
2019-10-31T13:53:55.6404913Z 
2019-10-31T13:53:55.6405409Z 
2019-10-31T13:53:55.6409793Z 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"
2019-10-31T13:53:55.6409975Z 
2019-10-31T13:53:55.6410002Z 
2019-10-31T13:53:55.6423776Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-31T13:53:55.6423861Z Build completed unsuccessfully in 0:01:33
2019-10-31T13:53:55.6423861Z Build completed unsuccessfully in 0:01:33
2019-10-31T13:53:55.6474236Z == clock drift check ==
2019-10-31T13:53:55.6481729Z   local time: Thu Oct 31 13:53:55 UTC 2019
2019-10-31T13:53:55.7322112Z   network time: Thu, 31 Oct 2019 13:53:55 GMT
2019-10-31T13:53:55.7325205Z == end clock drift check ==
2019-10-31T13:53:57.1070772Z 
2019-10-31T13:53:57.1177636Z ##[error]Bash exited with code '1'.
2019-10-31T13:53:57.1218203Z ##[section]Starting: Checkout
2019-10-31T13:53:57.1220077Z ==============================================================================
2019-10-31T13:53:57.1220138Z Task         : Get sources
2019-10-31T13:53:57.1220192Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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. (Feature Requests)

[traxys]

As @eddyb and @pnkfelix suggested, because writing a test is complex, I only left the fix and the test will be provided in another PR

[michaelwoerister]

That's OK too. The changes are straightforward. Thanks for your work, @traxys!

@bors r+

[bors]

📌 Commit 5930551 has been approved by michaelwoerister

[varkor]

we also need to check about whether there is a good way to cause the compiler to ICE in a reliable fashion

fn main() {
	break rust;
}

ICEs "reliably", as long as this joke isn't removed (though it doesn't use delay_span_bug, so it's still probably not appropriate without some adjustment).

[bors]

⌛ Testing commit 5930551 with merge 1d506eca253e003f26ecb3a6ca5e3f0c8314d488...

[tmandry]

@bors retry rolled up