Forbid truthiness tests on Traversable interface types
commitb05a9459273239f99bbe5fa76577b591f0813f19
authorJake Bailey (Hacklang) <jakebailey@fb.com>
Fri, 2 Nov 2018 21:05:29 +0000 (2 14:05 -0700)
committerHhvm Bot <hhvm-bot@users.noreply.github.com>
Fri, 2 Nov 2018 21:08:22 +0000 (2 14:08 -0700)
tree9b319824aa1e4f237759d53bd7dfaab23a67ccd7
parentff402f4dd6e2c94390be3c20e9694cd6f0920dee
Forbid truthiness tests on Traversable interface types

Summary:
D8786329 bans truthiness tests on all class types (particularly, concrete or abstract classes, but not interfaces) which implement Traversable but not Container, since any instance of such a class will always be truthy, even when empty. It does not emit an error for any interface type, but testing the truthiness of an interface type which implements `Traversable` but not `Container` is also a potential logic error.

For example, some functions take a Traversable and then test its truthiness in order to determine whether it is empty:

```
function f(Traversable<int> $x): void {
  if (!$x) {
    print('x was empty');
  } else {
    print('x was non-empty');
  }
}
```

But this is not correct--types which implement `Container` will be falsy when empty, but any other type won't be. The behavior when the sequence is empty changes based on the runtime type of `$x`.

There is no way to check whether a `Traversable` is empty in general without causing side effects (since generators implement `Traversable`).

It seems to be the case that many situations where we take a `Traversable` interface type and then check its truthiness are ones where we could simply use the `Container` or `KeyedContainer` interface instead.

When the current behavior really is what a user wants, they should write this instead:

```
function f(Traversable<int> $x): void {
  if ($x instanceof Container && C\is_empty($x)) {
    print('x was empty');
  } else {
    print('x was either a non-empty Container or some other Traversable');
  }
}
```

This change forbids truthiness tests on any interface type which implements `Traversable` but not `Container`.

Reviewed By: DavidSnider

Differential Revision: D8900763

fbshipit-source-id: b97af95694b843dc80e96513afbbb518738d001e
hphp/hack/src/errors/errors.ml
hphp/hack/src/errors/errors_sig.ml
hphp/hack/src/typing/tast_check/truthiness_test.ml
hphp/hack/src/typing/tast_utils.ml
hphp/hack/test/typecheck/truthiness_test_03.php.exp
hphp/hack/test/typecheck/truthiness_test_11.php
hphp/hack/test/typecheck/truthiness_test_11.php.exp
hphp/hack/test/typecheck/truthiness_test_15.php.exp