Fix false positives in unreachable_code lint

[FabianWolff]

Fixes #89779 (both the false positives as well as the empty match) and fixes #88393.

r? @Mark-Simulacrum

[Mark-Simulacrum]

Can you give a high level description of the changes in the second commit? I can probably figure it out based on the diff, but it would be good to document that in the commit message and PR description to make it easier to identify what's going on.

[FabianWolff]

Can you give a high level description of the changes in the second commit? I can probably figure it out based on the diff, but it would be good to document that in the commit message and PR description to make it easier to identify what's going on.

I've added a comment in the code explaining what is happening.

Honestly, at this point, I think it would be better to have a separate pass that does unreachable code warnings. The warnings in the type checker are incomplete (#85071), and adding them to the liveness analysis was a bad idea in hindsight because it just isn't a good fit (and avoiding duplicate warnings if the warning can be issued in two different places is also cumbersome).

This PR should solve #89779 for now, but we should look elsewhere for a good long-term solution.

[tabokie]

This will also fix #88393.

[bors]

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

[FabianWolff]

This will also fix #88393.

Right, thanks for pointing this out! I have added a test case for it.

@rustbot ready

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

[Mark-Simulacrum]

Hm, can you say more about the specific case being addressed here? It's not quite clear to me why diverges would be yes and warn unreachable would be no, yet we shouldn't lint on that expression? Maybe some code examples for each case?

[FabianWolff]

enum Void {}
fn void() -> Void {todo!()}

fn bar(b: bool) {
    if b {
        void(); // A
    } else {
        // B
    }
    // C
}

In this example, A diverges because it has an uninhabited type. We can't issue a warning that "any code [in particular, C] following this expression is unreachable", though, because C might be reachable via the else branch. Hence A diverges and we don't want to warn about unreachable successors (C). If B also turns out to be diverging, we will warn about the entire if expression diverging and making C unreachable.

[Mark-Simulacrum]

Hm, so it seems like when we're exiting the if in the CFG, we should only consider C diverging if A and B have diverged -- so at point C we should have diverges = no, and as such not emit any warnings?

[FabianWolff]

Exactly.

[Mark-Simulacrum]

My comment was meant to suggest that I (still) don't understand the purpose/necessity for WarnUnreachable -- it seems to me that we should already not be warning when seeing C just based on Diverges = no at that point, so WarnUnreachable isn't actually necessary?

[FabianWolff]

Ah, sorry. Consider another example then:

if ... {
    diverges(); // X
    alsoDiverges(); // Y
}
// Z

Y has the successor Z and diverges, so Diverges=Yes, but WarnUnreachable=No because Z is also reachable via the (implicit) else branch. By contrast, X has the successor Y and diverges, so Diverges=Yes, and WarnUnreachable=Yes because X is Y's only predecessor. If there was a non-implicit else branch, the Diverges information would be used to check whether the entire if is diverging (in which case we would want to warn about Z being unreachable).

So you are right in that WarnUnreachable doesn't make a difference if Diverges=No, but it does for Diverges=Yes.

[Mark-Simulacrum]

I would expect the implicit else branch to have Diverges::No as its "result", making point Z have the same diverges as point A (presumably No, in your example):

// A
if ... {
   diverges(); // X
   diverges(); // Y
}
// Z

Y's successor is Z, but Z has two predecessors in the CFG: (roughly) A or Y, depending on if the if was taken. (Really, the predecessor of Z is the if condition and Y, I guess).

Getting this right is likely to be relatively hard, which is why I'm not sure we should be trying to roll this on our own -- a MIR rewrite would make it much easier.

[FabianWolff]

Getting this right is likely to be relatively hard, which is why I'm not sure we should be trying to roll this on our own -- a MIR rewrite would make it much easier.

I totally agree, but I don't have the MIR knowledge and time to do this myself at the moment.

[Mark-Simulacrum]

I left one question -- the code at a brief glance seems OK, but would like to review it more thoroughly with a better understanding of the intent behind the changes here (per the comment I left).

I tend to agree with you (#90636 (comment)) that we should be exploring other options. In particular, "dead code paths" seem like a pass that is well-suited for MIR, particularly since it may let us run const-prop and similar to detect more cases (though it's not clear whether that's actually desirable -- sometimes worse quality lints are actually better). On the other hand, MIR may be too low level to effectively map back. I think all the linting we do in the liveness pass should be possible to do on MIR, though -- we already have similar passes there, I think. If we feel that such a rewrite is feasible, I'd probably suggest pursuing that over patchwork fixes.

[JohnCSimon]

Ping from triage: Can you address the comment from the reviewer and fix the build?
@FabianWolff

FYI: when a PR is ready for review, post a message containing
@rustbot ready to switch to S-waiting-on-review so the PR appears in the reviewer's backlog.

[JohnCSimon]

@FabianWolff
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you force push to it, else you won't be able to reopen.
Thanks for your contribution.

@rustbot label: +S-inactive