Page MenuHomePhabricator

Write a lint rule for detecting unreachable blocks
Open, NormalPublic

Description

In D18984, human review caught return true; debugging code. Linters should be able to detect this, at least in the simple cases.

We can do this roughly like this:

Find all the conditional sub-blocks. for, foreach, while, and catch are conditional. do is not conditional. return and throw are not conditional, and may make sense to evaluate as though they were sub-blocks. break may also get similar evaluation inside a loop.

try/catch, if and switch need some special treatment, since they have purely conditional cases (if, switch with no default) and more complex cases (if/else and switch with a default).

For each sub-block, evaluate whether it always returns or not by recursively applying these rules to it. For return or throw sub-blocks, the answer is "yes". For other sub-blocks, the answer is "yes" if any returning sub-block is always executed.

If a block returns unconditionally and has semantic symbols after the point at which it returns, raise a lint error.

One thing that may be tricky is try/catch:

try {
  sometimes_throw();
  return true;
} catch (Exception $ex) {
  return false;
}

return 'A';

If we look at this like it reads:

do {
  sometimes_throw();
  return true;
} while (false);

if (Exception $ex) {
  return false;
}

...then we will incorrectly detect that return false; is unreachable.

If we look at this like it reads:

if (sometimes) {
  sometimes_throw();
  return true;
} else {
  return false;
}

...then we'll get a better result, but fail to detect that return false is unreachable if sometimes_throw() is actually empty or some statement which can not possibly raise an exception. This is probably okay.

Another tricky thing is goto, but I think it's reasonable to write this off and say "if we find a goto anywhere in a block, just give up and don't try to determine reachability"; we don't use goto and probably no one else should either?

Other tricky things are multi-level breaks (tricky but doable?) and break $level; (impossible to evaluate in the general case). break $level; was removed in PHP 5.4 so these aren't major concerns.

99% of the things that are valuable to catch are trivial (debugging code) so this doesn't need to be totally flawless. We're just looking for dumb unreachable code, not this kind of thing:

if ($var % 2) {
  return 'A';
} else if (!($var % 2)) {
  return 'B';
}

return 'C';

"C" is unreachable and this could be determined statically, but this level of sophistication in analysis is unnecessary and a false negative is completely acceptable.