Delay specific span_bug() call until abort_if_errors()

[ebfull]

An actual typeck error is the cause of many failed compilations but an
unrelated bug is being reported instead. It is triggered because a typeck
error is presumably not yet identified during compiler execution, which
would normally bypass an invariant in the presence of other errors. In
this particular situation, we delay the reporting of the bug until
abort_if_errors().

Closes#23827, closes#24356, closes#23041, closes#22897, closes#23966,
closes#24013, and closes#23729

There is at least one situation where this bug may still be genuinely
triggered (#23437).

[rust-highfive]

r? @alexcrichton

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

[ebfull]

r? @pnkfelix

[pnkfelix]

don't change the message here; it is still a call to cat_expr_unadjusted up above, which is info I wanted to capture in the ICE message.

[pnkfelix]

My reasoning behind calling span_bug in the first place was that I wanted to be notified about cases where the call to cat_expr is err'ing.

I guess as long as such cases continue to emit an ICE, then its not wrong to make this change.

But I wonder if it might be simpler (and more correct) to try to identify an earlier spot in the control flow to add an additional call to abort_if_errors.

Update: Of course there is also the option of doing both; i.e. land this PR now, but also try to fix the ICE itself by finding a new place (or more) to call abort_if_errors.

Update 2: Adding earlier calls to abort_if_errors won't fix things, both for the reason outlined down below (that I transcribed from @arielb1) but also because the code path in question is already checking if the tcx.sess has reported any errors anyway.

[pnkfelix]

@arielb1 points out that inserting a new call to abort_if_errors would not be ideal, since it will continue to cause us to abort, when we really want to allow compilation to keep going and find more (independent) errors

The real problem here, @arielb1 points out, is that the guard on the span_bug here is only checking the tcx.sess to see if any errors have been emitted, but the fcx itself may also have errors registered that are not reflected in the tcx.sess.

Update: On further investigation, I am not 100% sure the analysis in this comment is quite right. E.g. I do not yet see how one can have errors registered in the fcx that are not reflected in the tcx.sess; stay tuned...

[pnkfelix]

Actually, another potential way to go here would be to use the same strategy employed in this PR, except with the refinement that the Some arm for the match *delayed_bug should only ICE if !self.diagnostic().handler().has_errors().

That is, if we did end up reporting at least one error, then we could just say "okay, lets pretend that was the error that caused cat_expr to Err.

(Though I do worry about the potential for circular reasoning here, where a cat_expr Err that has gone undetected could then yield a spurious error downstream, and then that's reported, but is useless to the user and at the same time we get no ICE bug report...)

[ebfull]

Actually, another potential way to go here would be to use the same strategy employed in this PR, except with the refinement that the Some arm for the match *delayed_bug should only ICE if !self.diagnostic().handler().has_errors().
That is, if we did end up reporting at least one error, then we could just say "okay, lets pretend that was the error that caused cat_expr to Err.

If I'm reading your comment right, that's what the code currently does. It should abort before the match *delayed_bug if there are any errors.

[arielb1]

I say remove the tcx.sess.has_errors() check in that case. The worst case here is that we get crazy lifetime errors.

[ebfull]

Done with both your recommendations, but it'd be nicer if these ICEs can be avoided by changing mem_categorization somehow.

[pnkfelix]

@ebfull after discussion with @arielb1 on IRC, I think we can indeed avoid many of the ICE's, not by changing mem_categorization, but rather by restructuring the type checker to do high-level signature-matching checks (e.g. that trait impls define all necessary items from the trait definitions) before checking any bodies. I have a prototype that does that in development right now.

We may indeed still want to put in this PR, but i am not certain of that yet; I, like you, would prefer to get rid of this ICE entirely. (And indeed, the path to get there may still involve changes to mem_categorization; I am not sure yet.)

[pnkfelix]

@ebfull btw you were correct when you wrote:

If I'm reading your comment right, that's what the code currently does. It should abort before the match *delayed_bug if there are any errors.

that is, my comment was based on a misreading of your code.

Update: fixed typo where I mistyped @ebfull 's username.

[pnkfelix]

I have opened #24422 , which adopts the strategy outlined in an earlier comment.

However, it does not address all of the ICE's listed in this ticket. So we may still indeed want to adopt this change. I will look into it.

[ebull]

@pnkfelix ebfull != ebull :)

---

From: Felix S Klock II notifications@github.com
Sent: Tuesday, April 14, 2015 12:57 PM
To: rust-lang/rust
Cc: ebull
Subject: Re: [rust] Delay specific span_bug() call until abort_if_errors() (#24367)

I have opened #24422#24422 , which adopts the strategy outlined in an earlier comment.

[https://avatars3.githubusercontent.com/u/173127?v=3&s=400]#24422

Typeck highlevel before bodies by pnkfelix · Pull Request #24422 · rust-lang/rust · GitHub
typeck: Do high-level structural/signature checks before function body checks. This avoids various ICEs, e.g. premature calls to cat_expr that yield the dreaded "cat_expr Errd" ICE. However, it also means that some early error feedback...
Read more...#24422

However, it does not address all of the ICE's listed in this ticket. So we may still indeed want to adopt this change. I will look into it.

Reply to this email directly or view it on GitHubhttps://github.com//pull/24367#issuecomment-92979142.

[huonw]

r? @pnkfelix (fell through the cracks?)

[pnkfelix]

@huonw well, i was half-hoping to find a way to fix all of the known ICE's without adopting the strategy outlined in this PR. But its probably a better plan to adopt it at this point.

[pnkfelix]

@ebfull you'll need to update the test; the compiler has changed now with #24422 so that this error message is no longer emitted (since the problem in the impl below is signaled first).

[ebfull]

Done, I stopped testing against what you fixed in #24422 as well.

[pnkfelix]

@bors r+ be11171 rollup

[bors]

⌛ Testing commit be11171 with merge 7765aed...

[bors]

💔 Test failed - auto-mac-32-opt

[alexcrichton]

@bors: retry

On Sat, Apr 25, 2015 at 6:10 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/4695

—
Reply to this email directly or view it on GitHub
#24367 (comment).

[bors]

⌛ Testing commit be11171 with merge b01ed84...

[bors]

💔 Test failed - auto-linux-64-nopt-t

[pnkfelix]

@bors retry

[bors]

⌛ Testing commit be11171 with merge b0043db...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[ebfull]

Should this be backported to beta like #24422?

[pnkfelix]

triage: beta-nominated

(I am personally on fence, but it is worth discussing)

[pnkfelix]

not accepted for beta backport.