Buffer NLL errors

[pnkfelix]

Buffer the errors generated during MIR-borrowck (aka NLL).

This is the first big step towards resolving issue #46908.

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[pnkfelix]

r? @nikomatsakis

[estebank]

The code changes look reasonable. Has there been no change to the stderr output of any test? I would have expected some switches in the ordering of at least some existing tests.

meta note With this kind of buffering of errors, I worry that when encountering ICEs a lot of context might be lost if the ICE happens before the NLL errors vector is drained. That being said I'm not opposed to the idea and ICEs are rare enough that in practice it shouldn't matter, it's just something to keep in mind.

[nikomatsakis]

Looks good, but what do you think about storing Diagnostic instead of DiagnosticBuilder?

[nikomatsakis]

So, I hate to mention this after y'all have been wrestling with lifetimes for so long, but it occurs to me that this PR would be much simpler if DiagnosticBuilder did not carry a 'cx lifetime. Indeed, if we look at the definition of DiagnosticBuilder, we see that it is the combination of two things:

rust/src/librustc_errors/diagnostic_builder.rs

Lines 23 to 29 in bd455ef

	/// Used for emitting structured error messages and other diagnostic information.
	#[must_use]
	#[derive(Clone)]
	pub struct DiagnosticBuilder<'a> {
	pub handler: &'a Handler,
	diagnostic: Diagnostic,
	}

I believe that the diagnostic field contains everything related to display the error and so forth, and the Handler just has the links to the session, stdout, etc. I believe that @michaelwoerister introduced this precisely so that we can serialize the set of errors produced by a query then load them later and "replay" them (also cc @estebank, who can problem confirm this).

I was thinking that maybe we should add a method like

impl DiagnosticBuilder<'cx> {
    fn buffer(self, buffered: &mut Vec<Diagnostic>) -> Diagnostic {
        buffered.push(self.diagnostic);
    }
}

and then we can replace all those calls like .emit() with .buffer(). Moreover, the buffer vector doesn't carry any lifetime.

In order to emit later, you can use DiagnosticBuilder::new_diagnostic(tcx.sess.diagnostic(), d) .

[pnkfelix]

I'll look into this now

[bors]

☔ The latest upstream changes (presumably #52405) made this pull request unmergeable. Please resolve the merge conflicts.

[pnkfelix]

@estebank asked:

Has there been no change to the stderr output of any test? I would have expected some switches in the ordering of at least some existing tests.

I worked pretty hard to try to ensure that the ordering is unchanged on all tests.

[pnkfelix]

@estebank noted:

meta note With this kind of buffering of errors, I worry that when encountering ICEs a lot of context might be lost if the ICE happens before the NLL errors vector is drained.

Indeed, I actually wanted the buffering to be controlled (i.e. be disabled) via a debug flag, but I wasn't able to come up with enough of an argument for doing so.

I think that the case you note might serve as a motivation for adding a debug flag to force an "immediate mode" on these errors.

[pnkfelix]

I think that the case you note might serve as a motivation for adding a debug flag to force an "immediate mode" on these errors.

or ... hmm, I wonder if there's a reasonable way to drain the errors buffer immediately in response to an ICE...

I incorporated the Vec<Diagnostic> idea into the PR.

[nikomatsakis]

@bors r+ p=1

EP2 criticial.

[bors]

📌 Commit 1a0294b has been approved by nikomatsakis

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: rustc: Work around an upstream wasm ThinLTO bug #52506

[bors]

📌 Commit 1a0294b has been approved by nikomatsakis

[bors]

⌛ Testing commit 1a0294b with merge 8dbbd81...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8dbbd81 to master...