Reduce confusing unreachable_code lints

[clubby789]

Closes #149042

Attempts to be smarter about identifying 'real'/user-written unreachable code to raise the lint

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[cjgillot]

Iirc some parts of mir building uses end_point for drop spans. Should this do the same?

[Zalathar]

You'll probably need to bless the coverage tests with:

./x test coverage coverage-run-rustdoc --set=build.profiler=true --bless

Coverage instrumentation unfortunately relies on a bunch of fragile heuristics that get disturbed by changes like this one, but luckily the new output seems reasonable.

[clubby789]

Iirc some parts of mir building uses end_point for drop spans. Should this do the same?

Some examples of diagnostic changes with this

error: unreachable expression
  --> $DIR/void-branch.rs:10:9
   |
LL |             std::mem::uninitialized::<Void>();
   |             --------------------------------- any code following this expression is unreachable
LL |         }
   |         ^ unreachable expression

LL | #[define_opaque(WithoutLt)]
LL | fn without_lt() -> impl for<'a> Trait<'a, Assoc = WithoutLt> {}
   |                             --                                ^
   |                             |
   |                             hidden type `&'a str` captures the lifetime `'a` as defined here

I'm not sure if there's any performance implications with using sourcemap parsing on a pretty hot path, I'll perf it

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[clubby789]

@bors try @rust-timer queue

[nnethercote]

The first commit seems like a small improvement. The second commit touches a lot of test and coverage output for seemingly little gain, and doesn't seem worthwhile.

Supposedly this fixes #149042 but no test is added to cover that case. Seems like a test should be added?

View changes since this review

[clubby789]

The second commit touches a lot of test and coverage output for seemingly little gain, and doesn't seem worthwhile.

Agree, I think I'll back it out (though the coverage changes were originally just not blessed with the first commit, so there's still going to be some sweeping changes there).

Seems like a test should be added?

Imo, tests/ui/uninhabited/void-branch.rs (which was added with the PR the lint change) demonstrates the span issue and fix well, do you think a dedicated test is needed?

[nnethercote]

do you think a dedicated test is needed?

#149042's description said a lint was issued in a case where it shouldn't be issued. void-branch.rs just sees some minor changes in output, which seems much less compelling. If you think it's good enough I will defer, it just seemed surprising.

[clubby789]

Ah, this change is about turning

  |
7 |   fn check() -> () {
  |  __________________^
8 | |     make_never();
  | |     ------------ any code following this expression is unreachable
9 | | }
  | |_^ unreachable expression

into

8 |     make_never();
  |     ------------^ unreachable expression
  |     |
  |     any code following this expression is unreachable

I think there's a good argument to be made that the implicit void return should not be counted as 'code' that's unreachable. I'll see if that's feasible to reliably detect

[rust-bors]

☀️ Try build successful (CI)
Build commit: b2b48b3 (b2b48b3941a98a5ab0c2d3f77d25a989ed59d04f, parent: 217cb73577ed6f30a2005dd75bab01d23ec4cd60)

[clubby789]

@rustbot ready

[cjgillot]

Where does the error point on the unchanged test? Do you mind adding another if false block with the println an keep the original one?

[clubby789]

No error is raised within the block without the additional println (similarly to fn block in the other test I added).

Do you mind adding another if false block with the println an keep the original one?

Sorry, can you add an example? I'm not quite sure what this should look like

[nnethercote]

This seems ok to me once @cjgillot's questions are addressed.

[nnethercote]

At this point I think we should just r? @cjgillot