HHI: previous exception in exception's constructor needs to allow \Throwable, not just \Exception

[fredemmott]

HHVM Version

3.27

Standalone code, or other way to reproduce the problem

<?hh

class Foo extends \Exception {
  public function __construct(
    Throwable $previous,
  ) {
    parent::__construct('hello', 0, $previous);
  }
}

Actual result

src/__Private/LinterException.php:23:57,65: Invalid argument (Typing[4110])
  /private/tmp/hh_server/hhi_3b2ecd0f/exceptions.hhi:76:16,24: This is an object of type Exception
  src/__Private/LinterException.php:21:6,15: It is incompatible with an object of type Throwable

Expected result

No errors

[VitorFalcao]

Hello @fredemmott I am looking to contribute to HHVM, since I program in Hack in a daily basis and I thought this issue could be a good start. Is the solution just a quick change from ?Exception to ?Throwable in the following line? https://github.com/facebook/hhvm/blob/25162c745586cd5682db5825ffff0db75c62d6e7/hphp/hack/hhi/exceptions.hhi#L77

[fredemmott]

• Add tests to runtime (hphp/test/slow) and typechecker (hphp/hack/test/)
• Make that change to the HHI
• If runtime tests fail (unlikely), make a similar change to the runtime (probably somewhere in hphp/system/php or somewhere in hphp/runtime/ext/)

[VitorFalcao]

Hi @fredemmott sorry for the n00b question, but I am having trouble running the typechecker tests. How do I run them, please?

[xixixao]

@fredemmott Couple questions:

1. https://docs.hhvm.com/hack/statements/throw says throw only supports \Exception subclasses, but it supports \Throwable implementations.
2. What's the difference between \Error and \Exception? Should we have just one of them?

[fredemmott]

1. can you file an issue against docs.hhvm.com?
2. https://www.php.net/manual/en/language.errors.php7.php is a good overview; in short, Error is used instead of the PHP5 error reporting mechanism for builtin errors. HHVM does not consistently do this though, and several HHVM-specific errors extend Exception instead of Error. Some unification should happen at some point, but it's not as straightforward as replacing one of them with the other due to different expectaitons around what catch(Exception) will deal with, what will hit the PHP error handler, and what will hit the PHP exception handler.

[xixixao]

I had to read https://trowski.com/2015/06/24/throwable-exceptions-and-errors-in-php7/ (linked from comments) to really understand what's going on, and it's pretty unfortunate :( It might help to add a comment to the Hack declaration file for \Throwable, something along the lines of:

There used to be only Exception class, and PHP would just kill a script for certain kinds of errors (perhaps link or enumerate the errors here). Later those errors were converted to use the standard exception mechanism, but to avoid breaking old code, a new Error class was introduced. If you want to catch both Errors and Exceptions, catch Throwable. Read more here:...

[lexidor]

I am looking into __SystemLib to fix an issue with __SystemLib\BaseException::$previous. (This trait is used in \Error and \Exception.) It is typed as ?\Exception instead of ?\Throwable.

Fixing a <large codebase> is... horrendously impractical. We’d probably want to add a new property instead. The big problem is that it’s a protected property, so the type is invariant. So lots of subclasses would need to be modified at the same time as the type checker change.

What kind of behavior do we want for the old methods?

• The constructor of \Exception takes ?\Exception as its third argument (ought to be ?\Throwable).
• The constructor of \Error takes ?\Throwable, needs to be safe. (Depending on the runtime setting for property type enforcement, this throws.)
• \Exception has ->setPrevious(\Exception) in its hhi, but \Error and \Throwable do not.

We need to add ->getPreviousThrowable() and ->previousThrowable to __SystemLib\BaseException. Do we want to keep protected visibility for the property? The previous behavior of \Exception->setPrevious(\Exception) when a \Throwable was passed was unsound and sets ->previous anyway. What should ->getPrevious() do when ->previous and ->previousThrowable differ? \Error->getPrevious() already returns ?\Throwable, should \Error get the new methods for consistency?

[lexidor]

Not quite resolved. 4.162.0 fixes the property, but the third argument of \Exception is still ?\Exception. The property change was the one with the highest friction, since properties are invariant. In order to truly resolve this issue, we must change the hhi too.

Runtime ?\Throwable 😃 https://github.com/facebook/hhvm/blob/master/hphp/system/php/lang/Exception.php#L50 Hhi ?\Exception 😢 https://github.com/facebook/hhvm/blob/master/hphp/hack/hhi/exceptions.hhi#L90

[lexidor]

See https://github.com/facebook/hhvm/pull/9104

[lexidor]

This issue is resolved at runtime, but not in the hhi. https://github.com/facebook/hhvm/blob/a27b1661b3cdcb38eb5f1ee1bbe53f55f8f368a7/hphp/hack/hhi/exceptions.hhi#L90 This should be Throwable instead.