Typechecker does not identify errors in unreachable code

[ryangreenberg]

Describe the bug
Errors in code after a return statement are not identified.

Standalone code, or other way to reproduce the problem

function unreachable_1(): int {
  return 5;
  return $bar;
}

function unreachable_2(): int {
  $bar = 6;
  return 5;
  return $bar->getApple();
}

$ cat unreachable.hack
function unreachable_1(): int {
  return 5;
  return $bar;
}

function unreachable_2(): int {
  $bar = 6;
  return 5;
  return $bar->getApple();
}

$ hh_client
No errors!

Expected behavior

Typechecker error about the undefined variable or error about unreachable code.

Actual behavior

No errors.

Environment

• Operating system: MacOS Bug Sur
• Installation method: homebrew
• HHVM Version

hhvm --version
HipHop VM 4.150.0-dev (rel) (non-lowptr)
Compiler: 1645593988_N
Repo schema: 3a9d8190fa75970b0339c54801281ff070f42b81

hh_client --version
hackc-3d05e8829ec5c101725f1ce265af9f3038bc288f-4.150.0-dev

Additional context
This is related to #8660, where @Wilfred writes:

Dead code is really tricky. Users like to have some type checking there, but it's legitimate for the type checker to ignore it entirely. Dead code checks are a best effort.

The typechecker is working here in the goal of preventing runtime errors—the dead code can't produce an error. But this is a failure in the related goal of avoiding bugs; the existence of unreachable code itself is often a bug. I found this issue because of a refactor that introduced an early return and missed some important behavior.

[lexidor]

Variables being typed as nothing in unreachable code is an appropriate application of the type refinement rules. Given a nullable int, an if ($x is null) return; would refine to int. An unconditional return is equivalent to if ($x is mixed) return;. After this refinement, only nothing remains.

Treating dead code as an error would result in a lot of undesirable behavior. Given a function fun() with a return type annotation of nullable int and the following code.

$x = fun();
if ($x is int) return $x;
// Create a default value and return it.

If I know that fun() only every returns ints and I make the return type int instead of nullable int, Hack would report errors on this code. This would discourage tightening return types, since every time you do, the typechecker makes it look like you broke everything. In fact, you didn't break anything. Your code has not changed its behavior.

[ryangreenberg]

I don't really know enough about type systems to discuss intelligently what should be inferred for unreachable code—nothing seems reasonable—but I do think it would make the typechecker more useful for finding bugs if it identified and emitted errors for unreachable code.

Running UnreachableCodeLinter—which is limited to much less sophisticated analysis than the typechecker—on Slack's codebase, found ~20 unreachable blocks, several of which represented logic bugs.

In your example I disagree that identifying the errors would be undesirable. It's already pretty common for changing or constraining type annotations to produce new typechecker errors. I think it would be useful to be alerted that code you thought might execute at some point will never run. Nothing is broken in the sense that the runtime will error, but not all bugs manifest as runtime errors.