cargo test fails with `rustc 1.24.0-nightly (0a3761e63 2018-01-03)`

[topecongiro]

idempotence_tests is failing:

Mismatch at tests/target/macros.rs:213:
 ⏎
 // #1577⏎
 fn issue1577() {⏎
-    let json = json!({⏎
-        "foo": "bar",⏎                                                                                                       
-    });⏎                                                                                                                     
+    let json =⏎
+        json!({});⏎                                                                                                          
 }⏎
 ⏎
 gfx_pipeline!(pipe {⏎

cargo test passed without any error with rustc 1.24.0-nightly (b65f0bedd 2018-01-01).

[nrc]

I've been looking at this just now - I think it is caused by the error recovery mechanism in the parser somehow - we're calling parse_macro_arg twice with identical arguments and getting back an error the first time around and {} the second time around. The input is identical, so I assume there is some state in the ParseSess, but I don't see anything in that data structure which looks suspicious (could be in TLM too, I guess).

[topecongiro]

rust-lang/rust#47146 seems to have caused this?

[nrc]

Yeah, looks like a strong candidate. We probably need to completly reset the SpanHandler, rather than just calling reset_err_count

[ereslibre]

I'm curious about what could be breaking rustfmt on that patch. From what I can see on rust-lang-nursery/rust-toolstate@1b316cc we have f0e5c9..0f4ebf possibilities, right? (rust-lang/rust@f0e5c95...0f4ebf9)

[ereslibre]

Or maybe I'm just wrong and rust-toolstate only updates references when their status change (pass -> fail, or fail -> pass) and then rust-lang/rust#47146 would be the problem.

Sorry, it was my first contribution and didn't know I also had to check rustfmt :(

[ereslibre]

So, yes, you are right @nrc about your suspicions about the state.

Calling to reset_err_count will effectively set the error count of the handler to 0. However, when diagnostics are emitted, we reach https://github.com/rust-lang/rust/blob/0f4ebf9f0a3196420e25cf1558b49ea3f38643c4/src/librustc_errors/lib.rs#L573.

Here, we only emit the diagnostic if it wasn't emitted before (the exact same diagnostic), here we discard repeated diagnostics.

And thus, with rust-lang/rust#47146 the error count will never get increased again for this very same error, as the state is kept despite the count was reset.

[nrc]

Sorry, it was my first contribution and didn't know I also had to check rustfmt :(

Don't worry! We're still experimenting with how to manage tools in the Rust repo and at the moment tools get silently broken with no warning, so there is no reason you should have known.

Yes, I assumed it was the hashing that was causing the problem, I should have been more explicit, but I was hoping for a quick fix and it was late :-)

[ereslibre]

Just wanted to comment that this patch on rust fixes the issue:

diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs
index c4db39fae8..5001ce7a41 100644
--- a/src/librustc_errors/lib.rs
+++ b/src/librustc_errors/lib.rs
@@ -313,6 +313,7 @@ impl Handler {
     // NOTE: DO NOT call this function from rustc, as it relies on `err_count` being non-zero
     // if an error happened to avoid ICEs. This function should only be called from tools.
     pub fn reset_err_count(&self) {
+        self.emitted_diagnostics.replace(FxHashSet());
         self.err_count.store(0, SeqCst);
     }

However I'm not sure if the semantics of reset_err_count really match with also replacing the emitted_diagnostics with an empty hash set. Maybe this method can be renamed (I don't know how rust policy is here, or if it can break other third parties using this API) to a more generic reset_err_session or something similar.

What do you think? I'd be willing to help fixing the problem since somehow I was the one that caused it.

[topecongiro]

@ereslibre Thank you for the PR!