Remove assertion on reachable blocks w/o results

[flaper87]

Closes #11709

[flaper87]

@huonw r?

[flaper87]

@alexcrichton r?

[alexcrichton]

I don't consider myself familiar enough with trans to review this, although I know many others have been running around in trans lately.

There's a whole bunch of test cases in #11709 and they all look pretty small, so perhaps you could expand the test case a bit?

[huonw]

Why turn it into an if with .unwrap when you can just keep it "nice" with a match?

match b.expr {
    Some(e) => {
        bcx = expr::trans_into(bcx, e, dst);
    }
    None => {}
}

[flaper87]

Fewer lines of code. Would there be a benefit from using match instead of if here?

[alexcrichton]

When you have a functional language, it's good to make use of it :)

You're checking for Some twice (is_some and unwrap), and while LLVM will do its magic, it's generally a code smell to structure code this way.

[flaper87]

I'll change it back then, thanks!

[nikomatsakis]

I'm no fan of dest::Ignore (I'd like to rip it out) -- but I think what is SUPPOSED to happen for a block with a unit result is that we switch over to "Ignore" output mode. I wonder why that's not happening here. Still, I imagine that this change is harmless.

[flaper87]

I'll investigate this a bit further

[flaper87]

@nikomatsakis @alexcrichton instead of removing the asserition, I added a check where trans_block is called. The checks I added are the same used for clousures. I believe this could be improved a bit more later. For instance, whether dest should be Ignore or something else could be done in trans_block directly. In order to do that, we'd need to pass other arguments to the method.

[huonw]

@flaper87 as @alexcrichton said, there are several more test cases on #11709, e.g. adding the line let _r = {}; to the test you already have.

Also, #11865 hits the same assertion; does this fix it?

[flaper87]

@huonw @alexcrichton ops sorry about that, I didn't mean to ignore that comment. Let me add those tests there.

[flaper87]

@huonw This fix works for #11865 too.

[nikomatsakis]

@flaper87 what additional information would trans_block need?

[nikomatsakis]

@flaper87 in particular it seems like trans_block could easily check what its own output type is and so on. Might be more robust. (I would like to remove Ignore mode though...feels like it's a complex invariant we're maintaining and I'm not clear on exactly why)

[flaper87]

@nikomatsakis Agreed, callers would always pass a Dest and trans_block would choose whether to use it or not.

W.r.t removing expr::Ignore, I also agree that we don't actually need it. Dest could be just a Option<ValueRef>. I can make this change part of this PR too if we agree on it.

[nikomatsakis]

I'd leave that for a separate patch -- also, what I had in mind was
just having Dest always be a ValueRef

[nikomatsakis]

(r+ with the assertion I mentioned)

[flaper87]

@nikomatsakis done, Thanks!

[alexcrichton]

Feel free to // xfail-pretty the test, we sadly don't have any active maintainer of the pretty printer.