Eager unreachable blocks for ! type

[varkor]

This change optimises the generation of MIR in the presence of !. Blocks are now immediately marked as unreachable if:

• A function argument is derived from the value of type !.
• A match arm binds a value whose type is derived from !.
• An rvalue is created whose type is derived from !.

This avoids unnecessary MIR generation and also fixes #43061 and fixes #41446.

r? @nikomatsakis

[nagisa]

There should be a MIR codegen test too. You can find some examples in src/test/mir-opt.

[varkor]

I've added one — I wasn't 100% sure which MIR pass to check after, so I've gone for SimplifyCfg-initial for now. I just tried to check that the right basic blocks were being marked as unreachable — let me know if this isn't quite stringent enough.

[nagisa]

SimplifyCfg-initial is okay.

[nikomatsakis]

This is actually not as conservative as might be needed. =) For example, it is certainly wrong that *mut ! requires a never value -- it could legally be null. Similarly, there is some dispute about whether &! is truly uninhabited, since in unsafe code such types can come to be (but never get dereferenced).

I would be happier if we just used TyNever here -- but perhaps the real question is why we don't delegate to one of the existing "is uninhabitable" functions. Was this a deliberate decision, or simply a matter of not knowing about said functions? :)

[varkor]

Ah, I shouldn't have forgotten about the weaker unsafe guarantees.
Not using an existing function was just an oversight! Would it be more appropriate to use conservative_is_uninhabited or is_ty_uninhabited_from_all_modules?

Edit: I've used conservative_is_uninhabited (and moved it into a more convenient location) for now, but if that's not the most appropriate one, let me know!

[nikomatsakis]

Similarly here, there is existing code that tries to make a similar judgement, I have to go digging around to find it, but I suspect we should be re-using it.

[varkor]

I had a look around, and couldn't find anything that looked suitable, but I might have been looking in the wrong places. If you can give me a pointer, I'll replace this one!

[nikomatsakis]

Let me look around.

[nikomatsakis]

Hmm, I'm not sure if this is quite right either. Consider a case where you have an arm with two patterns, and one of them is unreachable but the other isn't. e.g.

let x: Option<!> = None;
match x {
    Some(_) | None => ...
}

woudn't this (single) arm be filtered out here? Or am I confused. Maybe we want .filter(|arm| arm.patterns.iter().any(|pat| !pat.is_unreachable()))? i.e., keep the arm if any of its patterns are reachable? Or, probably, we want to filter out the patterns (and then filter out arms that have no patterns left)?

[varkor]

Whoops, you're absolutely right — I put the ! in the wrong place.
Edit: Although filtering the patterns themselves is a better idea.

[scottmcm]

Looks like the change basically removed all the code from https://github.com/rust-lang/rust/blob/master/src/test/compile-fail/uninhabited-matches-feature-gated.rs, since it's using the I-thought-it-was-instant-UB line

let x: Void = unsafe { std::mem::uninitialized() };

Failure from Travis:

[00:59:51] ---- [compile-fail] compile-fail/uninhabited-matches-feature-gated.rs stdout ----
[00:59:51] 	
[00:59:51] error: /checkout/src/test/compile-fail/uninhabited-matches-feature-gated.rs:25: expected error not found: non-exhaustive
[00:59:51] 
[00:59:51] error: 0 unexpected errors found, 1 expected errors not found
[00:59:51] status: exit code: 101
[00:59:51] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/uninhabited-matches-feature-gated.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/uninhabited-matches-feature-gated.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zmiri" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/uninhabited-matches-feature-gated.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
[00:59:51] not found errors (from test file): [
[00:59:51]     Error {
[00:59:51]         line_num: 25,
[00:59:51]         kind: Some(
[00:59:51]             Error
[00:59:51]         ),
[00:59:51]         msg: "non-exhaustive"
[00:59:51]     }
[00:59:51] ]
[00:59:51] 
[00:59:51] thread '[compile-fail] compile-fail/uninhabited-matches-feature-gated.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1170:13

[nagisa]

The loss of diagnostics due to UB in code seems unfortunate, but it is true that the match cannot be reached in the first place, so it being exhaustive or not doesn’t really matter here.

Should we try to also adjust the unreachable code lint to flag down cases like these?

[varkor]

I'll remove the offending statements; the other ones look okay. I think ensuring the unreachable code lint flags all of the cases introduced here would definitely be helpful (though maybe in a follow-up), as the main problem this introduces is that this eliminates a lot of MIR that would otherwise be analysed for issues.

Edit: #46164 covers extending the dead_code lint to handle these sorts of cases.

[nikomatsakis]

given that we didn't notice that the condition here was wrong, can we make a test targeting that particular case?

[varkor]

Yep, will do!

[varkor]

I've added a MIR test that differentiates between these two cases.

[nikomatsakis]

Hi @varkor -- so I was digging into this code the other day (just so you know I'm not ignoring it). Unfortunately, I don't seem to have kept many notes of my progress. :P

In answer to your questions:

Would it be more appropriate to use conservative_is_uninhabited or is_ty_uninhabited_from_all_modules?

It seems like we need a helper for this. That is, I see that several other parts of the code make a decision based on the feature-gates that are enabled (in particular, never_type enables more aggressive reasoning).

That said, one thing that I was puzzling over was what role privacy ought to play here. The "uninhabited from all modules" helper will for example fail to find some types are uninhabited even if they are because the source code cannot observe the uninhabitedness (e.g., it's in a private field). It seems like we probably do want to respect privacy initially, though maybe once we start optimizing, we should ignore it (e.g., otherwise, the borrowck behavior might be affected by private fields, which seems bad).

But for that matter we might want to use a more specific variant that just checks whether a type is uninhabited from the point of view of the code we are currently building.

I had a look around, and couldn't find anything that looked suitable, but I might have been looking in the wrong places. If you can give me a pointer, I'll replace this one!

I might have been wrong. There is definitely code that performs the same check -- notably the is_useful function and friends -- but I'm not sure if it's easily packaged for your use.

[nikomatsakis]

Still, I have to say that one other thing is bothering me. It feels like the original ICE, which occurred with this test:

fn blah(x: !) {
    x;
    // () // Uncommenting this explicit unit return avoids the ICE
}

is really arising because we are failing to do the "implicit coercion". That is, if you have the explicit (), I think you will find that there is a CoerceToAny being inserted, right? Do we not generate this CoerceToAny for blocks with an implicit return?

I guess I feel a bit nervous about "doubling down" on the strategy of not generating code when we see e.g. local variables with type !, vs just generating coercions in the right places.

This is particularly true since I fear that any checks and code we write here will be conservative, and thus it may be that there are cases where indeed there are coercions-to-! occurring but we are still generating code (I don't have a good example of this, but imagine that e.g. our check for whether an arm is unreachable misses some cases). This is probably not a theoretical limit but more a "I suspect future ICEs may be coming" sort of thing.

[varkor]

The main reason I didn't make use of the privacy-respecting uninhabited checks was because I was unsure of which situations one would not want the compiler to use all the information available to it for optimisation — intuitively, privacy is for the benefit of the user, and inhibiting optimisations because of it seems odd — though there could be some context there I'm missing. I suppose there could be transitive issues with dead code warnings for ! in private code, but even then it seems like a useful detail to surface to a user, even if they weren't directly responsible for it.

Yeah, if I remember correctly, the issue with the crash is failing to generate a NeverToAny — I didn't quite narrow down exactly what the specific case was, but it was potentially not being coerced because it was never actually "used", so there was no reason to coerce it.

To me the real issue seems to be "occurrences of values of !-type". Though it's possibly not the smallest fix to address this specific ICE, I think by eliminating code involving !-rvalues, this entire class of ICEs are eliminated — ICEs involving missing coercisions between ! and () (apparently not an isolated issue) should as far as I can tell necessitate a !-typed rvalue. And the check for ! rvalues isn't conservative in a way that affects this behaviour — it would be sufficient simply to check for a plain !-type — the more comprehensive composite checks can just terminate paths earlier.

If a fix solely for the missing NeverToAny was produced, I feel that there are more opportunities for future ICEs than with this change, which I think is more comprehensive.

That said, I do feel similarly in that it would be nice to find the missing NeverToAny in addition to this change, just as an extra layer of protection. I'll be a bit busy over the next few days, but if I can get some time, I'll try looking back into it.

[nikomatsakis]

The main reason I didn't make use of the privacy-respecting uninhabited checks was because I was unsure of which situations one would not want the compiler to use all the information available to it for optimisation — intuitively, privacy is for the benefit of the user, and inhibiting optimisations because of it seems odd — though there could be some context there I'm missing.

Well, the point is that the MIR is used for more than optimization. It is also used for safety checks as well as (soon...) borrow checking. And borrow checking in particular takes flow information into account. This means that if you "observe" a private uninhabited field, then you may have code that borrow checks successfully, but if that private field later changes its type to be inhabited, then downstream consumers may be affected. I don't know how likely this scenario is, but it's the kind of thing that we are trying to avoid by making "inhabitedness checks" privacy respecting in the first place.

We may well want another pass that runs after safety checks are done which ignores privacy.

In any case, using the "from all modules" check is kind of strictly more conservative in any case.

If a fix solely for the missing NeverToAny was produced, I feel that there are more opportunities for future ICEs than with this change, which I think is more comprehensive.

Hmm. Yes, I see your argument. I am trying to figure out what I am worried about here, maybe it's ill-founded.

[nikomatsakis]

OK, I've given this some more thought. I remain somewhat uncomfortable with this PR as is, although I think what it's doing makes sense. What makes me uncomfortable is that it feels just a bit ad-hoc -- that is, it's trying to do a kind of uniform "detect unreachable things" check, but somehow I don't feel "sure" that we're doing it in the right place or have covered all the cases.

I am wondering if we can dig a bit more into the original ICE first -- I at least would like to better understand what goes wrong. Maybe I have to schedule a bit of time to dig into it myself, @vorner not sure if you can kind of explain what detective work you did?

I guess it's also a good idea for me to offer an alternative: I think that I would feel mildly more comfortable if we implemented this via sort of "simple post pass" that detected "stores" to variables of type ! and killed the related code. That seems like something that will work uniformly. But maybe I'm missing the point.

[varkor]

The main motivation for this change was reducing the amount of unnecessary work around unreachable code, rather than addressing #43061, which happened to be a happy side-effect, but I can see that by claiming to fix it, it somewhat seems like sweeping it under the rug. Considering this PR was intended to be behaviour-preserving, the fact that it isn't seems to draw more attention to the possibility it doesn't cover every case where unreachable could be emitted earlier. I do think separately fixing the ICE would make the PR seem like a more secure decision.

Regarding locating the source of the original ICE: I think the most illustrative example so far is this one:

fn foo(x: !) {
  x; // ICEs
}
fn bar(x: !) {
  {x}; // No problem!
}

The type coercion seems relatively similar for the two:
Both of them get ... !, expected is NoExpectation for x, but bar coerces ! => _ (for the inner block — ... _, expected is NoExpectation), and following that the coercion ! => () is attempted for both. If it's an issue with the type coercion (and missing NeverToAny) this seems like a plausible spot? I still don't really understand the coercion system yet, though, so it's hard to make any hypotheses yet.

[nikomatsakis]

@varkor OK, so, digging a bit and leaving some notes in real time:

When we type-check x;, that is a StmtSemi (statement with semicolon), so we wind up at this code:

rust/src/librustc_typeck/check/mod.rs

Lines 4238 to 4240 in 3a39b2a

	hir::StmtSemi(ref expr, _) => {
	self.check_expr(&expr);
	}

As you can see, it calls check_expr with no hints or particular expectations on the return type:

rust/src/librustc_typeck/check/mod.rs

Lines 2759 to 2761 in 3a39b2a

	fn check_expr(&self, expr: &'gcx hir::Expr) -> Ty<'tcx> {
	self.check_expr_with_expectation(expr, NoExpectation)
	}

This call in turn invokes:

rust/src/librustc_typeck/check/mod.rs

Lines 2753 to 2757 in 3a39b2a

	fn check_expr_with_expectation(&self,
	expr: &'gcx hir::Expr,
	expected: Expectation<'tcx>) -> Ty<'tcx> {
	self.check_expr_with_expectation_and_lvalue_pref(expr, expected, NoPreference)
	}

which bottoms out here:

rust/src/librustc_typeck/check/mod.rs

Lines 3471 to 3474 in 3a39b2a

	fn check_expr_with_expectation_and_lvalue_pref(&self,
	expr: &'gcx hir::Expr,
	expected: Expectation<'tcx>,
	lvalue_pref: LvaluePreference) -> Ty<'tcx> {

Here, this an ExprPath and so check_expr_kind will do this (which is basically just computing the type):

rust/src/librustc_typeck/check/mod.rs

Lines 3645 to 3661 in 3a39b2a

	hir::ExprPath(ref qpath) => {
	let (def, opt_ty, segments) = self.resolve_ty_and_def_ufcs(qpath,
	expr.id, expr.span);
	let ty = if def != Def::Err {
	self.instantiate_value_path(segments, opt_ty, def, expr.span, id)
	} else {
	self.set_tainted_by_errors();
	tcx.types.err
	};
	// We always require that the type provided as the value for
	// a type parameter outlives the moment of instantiation.
	let substs = self.tables.borrow().node_substs(expr.hir_id);
	self.add_wf_bounds(substs, expr);
	ty
	}

and, on the way back up, we will notice that it is ! and set some flags, but we don't insert any sort of coercion:

rust/src/librustc_typeck/check/mod.rs

Lines 3498 to 3501 in 3a39b2a

	// Any expression that produces a value of type `!` must have diverged
	if ty.is_never() {
	self.diverges.set(self.diverges.get() | Diverges::Always);
	}

In contrast, for {x};, the code begins the same, but check_expr_kind will find an ExprBlock:

rust/src/librustc_typeck/check/mod.rs

Lines 3860 to 3862 in 3a39b2a

	hir::ExprBlock(ref body) => {
	self.check_block_with_expected(&body, expected)
	}

and so we invoke:

rust/src/librustc_typeck/check/mod.rs

Lines 4259 to 4261 in 3a39b2a

	fn check_block_with_expected(&self,
	blk: &'gcx hir::Block,
	expected: Expectation<'tcx>) -> Ty<'tcx> {

this will create a coercion. Since there are no break statements targeting this block, it'll go down this path:

rust/src/librustc_typeck/check/mod.rs

Lines 4289 to 4293 in 3a39b2a

	let tail_expr: &[P<hir::Expr>] = match tail_expr {
	Some(e) => slice::from_ref(e),
	None => &[],
	};
	CoerceMany::with_coercion_sites(coerce_to_ty, tail_expr)

So, I guess this is the key difference. We insert a coercion always for blocks (because there is the potential of many paths), but we don't insert any coercion in the case of just x;.

Now one thing I'm a bit unclear on is just why it ICEs later -- @varkor did you leave notes as to precisely where the ICE occurs?

[varkor]

I thought that the coercion in check_block_with_expected was probably the main difference there (compared to the path variable), but I wasn't clear on what a coercion to _ meant, or how it would affect the MIR (the coercion from ! => () seems to happen in coercion.rs either way).

The ICE itself is from:

rust/src/librustc_mir/borrow_check/nll/type_check/mod.rs

Lines 720 to 730 in 4e3901d

	match stmt.kind {
	StatementKind::Assign(ref place, ref rv) => {
	let place_ty = place.ty(mir, tcx).to_ty(tcx);
	let rv_ty = rv.ty(mir, tcx);
	if let Err(terr) =
	self.sub_types(rv_ty, place_ty, location.at_successor_within_block())
	{
	span_mirbug!(
	self,
	stmt,
	"bad assignment ({:?} = {:?}): {:?}",

where an assignment is generated in the MIR from a place with type ! to a place with type (). I haven't really dug into it from that direction, though, because that seemed just to be a consequence of an earlier issue, probably some distance from the source.

[nikomatsakis]

@varkor thanks, that's interesting.

I am trying to decide now what I think is the 'proper' fix. In some sense, I don't think that there is a "missing coercion". After all, the statement x; doesn't really require that the type of x be anything in particular. In a way, it's the {x}; -- with its kind of useless coercion to _ -- that is "wrong" (but not in a way that needs fixing).

This implies that your initial instincts were right, and that MIR building ought to be handling ! values a bit better. I note that calls to functions that have return type ! have somewhat special handling:

rust/src/librustc_mir/build/expr/into.rs

Lines 253 to 264 in 4e3901d

	let success = this.cfg.start_new_block();
	let cleanup = this.diverge_cleanup();
	this.cfg.terminate(block, source_info, TerminatorKind::Call {
	func: fun,
	args,
	cleanup: Some(cleanup),
	destination: if diverges {
	None
	} else {
	Some ((destination.clone(), success))
	}
	});

It falls out somewhat naturally there, since a Call always ends the basic block -- so we simply don't connect diverging functions to their successor blocks, in effect.

Anyway, I think I would be happier with a more minimal version of this PR that introduces the "disconnect" whenever an expression of type ! is evaluated -- so maybe more or less the changes you made to into(), although that is not obviously the right spot for it.

I would have expected it to maybe be as_rvalue or something? I'd like to dig a bit further into that failure in the MIR type-checker. In particular, that assignment that it is getting upset about, where does it come from? I guess we are creating some sort of "unit" with the result of the assignment or something?

I think maybe it's the changes to matches and pattern that make me uncomfortable. That is, I'd rather cut off execution when we evaluate an expression to a value of type ! (or perhaps any unreachable type), and not when we declare an lvalue of such type.

Does that make sense?

[varkor]

I've investigated a little more, under the assumption the coercion wasn't the problem, and discovered some interesting points:

• {x;} does actually the same assignment of () to ! as x; — but a separate basic block is generated for the block containing that assignment, which is then simply eliminated as dead code, without ever triggering the type check.

• rust/src/librustc_mir/build/block.rs

  Lines 141 to 147 in bb42071

  	// Then, the block may have an optional trailing expression which is a “return” value
  	// of the block.
  	if let Some(expr) = expr {
  	unpack!(block = this.into(destination, block, expr));
  	} else {
  	this.cfg.push_assign_unit(block, source_info, destination);
  	}

  
  This is the code that's generating the bad assignment — the implicit unit return at the end of a block. This explains why placing a manual () at the end of the function avoids the issue — it doesn't trigger this push_assign_unit.
• If the argument is x: u32 instead, then the destination place is _0, the return place, which is correct, so it's likely this is what it should be for x: ! as well (rather than _2, as it is in this example).
• I suspect the issue is coming from somewhere inside this as_local_rvalue:

  rust/src/librustc_mir/build/expr/into.rs

  Lines 52 to 59 in bb42071

  	ExprKind::NeverToAny { source } => {
  	let source = this.hir.mirror(source);
  	let is_call = match source.kind {
  	ExprKind::Call { .. } => true,
  	_ => false,
  	};
  	unpack!(block = this.as_local_rvalue(block, source));

  
  At some point the wrong destination is being used for the return value. However, it's late now, so I'll continue (hopefully conclude?) the search tomorrow.

I'll also reply about this PR specifically then!

[nikomatsakis]

@varkor nice, thanks for digging further in

[varkor]

I've tracked down the ICE issue and have a fix here: #47746.

To clarify my thoughts regarding this PR, which I think remains relevant (and in fact, is more so now that the ICE has been fixed separately). These optimisations are intended to decrease wasted time spent generating MIR for unreachable code, which is done to some extent, but to no significant degree currently.

1. I don't think there's any concern that further issues akin to Rust 1.20.0-nightly ICE with never_type: Broken MIR #43061 will be a problem: problems in the MIR generation (like Rust 1.20.0-nightly ICE with never_type: Broken MIR #43061) are no longer problems, because they will never be on paths taken in the presence of ! types (which could simplify some of the logic for handling those corner cases in the MIR generation). Problems before the MIR generation are highly likely to surface at an earlier stage, but if one didn't: see the next point.
2. The one downside this does have is that MIR that is no longer generated will not go through typeck, which means it might be easier for some more obscure errors to fall through the cracks. I think in practice, this will be a minor problem, as code that is unreachable anyway is dead, so at worst you permit a bug _only when it appears in dead code. I.e. this isn't something that will cause practical problems.
3. I think this is a desirable change. There's very little point wasting time generating code that will never be hit (and will be eliminated in a later pass anyway), and there's already precedent.

Regarding the cases handled here: I agree that an ad-hoc approach to unreachability is a little unsatisfying. I'd like to be comprehensive, though. I think this is perhaps a case of "you've missed [this case], that could be handled similarly", rather than "these are just random constructs to optimise". These were the cases I thought of, but if there are more, I think they should be added too. Any time you guarantee a ! rvalue or lvalue, I think it's sensible to handle (these seem like the main ones, but perhaps there are others). (I'm not sure why rvalues specifically should be treated specially.)

A post-pass (at least on the MIR) would be ineffective, because by that stage you've already spent all the time generating, etc. the code. And LLVM is going to do a decent-enough job of dead-code elimination afterwards. A post-pass at a higher level would be more effective, but I'm not sure if this is a plausible approach; this opportunistic "handle as we encounter it" approach seems to be somewhat sensible.

[nikomatsakis]

OK, so now that #47746 has landed, let's come back to this PR. I agree that there is probably more to be done here, though I'm still a bit torn on where and how it should be done (e.g., during MIR construction or as a later pass, etc).

[nikomatsakis]

A post-pass (at least on the MIR) would be ineffective, because by that stage you've already spent all the time generating, etc. the code. And LLVM is going to do a decent-enough job of dead-code elimination afterwards.

I am not sure if this time is really significant -- I don't think we expect a significant fraction of code to be stripped here.

What worries me is more that we ensure we are consistent in terms of where we report borrowck errors or other sorts of errors, and things like that.

I think the part of this PR that has made me the most nervous -- and I'm trying to put my finger on just why -- was the filtering of arms in a match. I'll have to re-read it. I think I would be happier if we could connect it to something more fundamental in MIR, e.g., if each time you assigned to a "place" of type ! (which ought to be impossible), that generated dead code.

I think this is perhaps a case of "you've missed [this case], that could be handled similarly", rather than "these are just random constructs to optimise".

I agree that this is a danger. I am trying to decide if I'm making a reasonable objection, or if the right approach is to just keep fixing the edge cases until there are no more.

[pietroalbini]

@nikomatsakis and @rust-lang/compiler, what's the status on this?

[nikomatsakis]

The status is that I haven't had time to look more closely at it yet. Sorry @varkor =)

[varkor]

No problem at all — I've found other things to distract me for the time being!

[nikomatsakis]

OK, I caught up on the conversation. I think I agree with basically everything that @arielb1 wrote. I'm trying to decide what that means precisely for this PR -- but I guess we can turn our attention temporarily to #50262 (and I've left a review there). I'm going to mark this as S-blocked for now until #50262 lands.

[TimNN]

Ping from triage. This PR has been blocked for over three months now. In line with the current triage guidelines I'm going to close it. Please reopen this or a new PR once the blocking PR has been merged.

I forgot to say: Thanks a lot for your contribution to the Rust project!