Allow classname<T> in instanceof test

[jesseschalken]

As far as I can tell, the commit e5e54bb is in error.

If $y is classname<T> (name of a class that is compatible with T) and $x instanceof $y is true, $x is necessarily compatible with T. I don't see the relevance to type erasure mentioned in the error message.

Fixes #7617

[hhvm-bot]

@aorenste has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[andrewjkennedy]

The problem with this construct is that it leads to unsoundness. Consider, for example:

function mycast<T>(mixed $x, classname<T> $t) : T {
  if ($x instanceof $t) { return $x; }
  else throw new Exception("Type didn't match");
}
function expectListOfString(List<string> $x) { ... }

Now if I write expectListOfString(mycast($y, List::class)) but with $y having type List<int> then no exception will get thrown at runtime (because of generics erasure).

[jesseschalken]

And yet this code passes but it has the exact same problem:

class List<T> {
  public function add(T $x): void {}
}

function main(): void {
  $y = new List();
  $y->add(1);
  foo($y);
}

function foo(mixed $x): void {
  if ($x instanceof List) {
    expectListOfString($x);
  }
}

function expectListOfString(List<string> $x): void {
  // ..
}

[andrewjkennedy]

With

enable_experimental_tc_features = instanceof

set in the .hhconfig file, or by running hh_single_type_check, that code does get rejected (in //strict mode). With the following message:

File "jesse.php", line 16, characters 24-25:
Invalid argument (Typing[4110])
File "jesse.php", line 20, characters 34-39:
This is a string
File "jesse.php", line 15, characters 7-8:
It is incompatible with a value of generic type T#1 from this instanceof test matching \List<T#1>
File "jesse.php", line 20, characters 34-39:
Considering that this type argument is invariant with respect to List

But I had forgotten that this was still marked as an "experimental" feature, whereas the classname restriction is turned on regardless. Thanks for spotting this!

It's probably time to turn on the "safe instanceof" feature permanently. (See 4c2a609 and related commits.) In general, we're trying to close all known strict-mode type soundness issues.

[jesseschalken]

@andrewjkennedy Awesome, I didn't know about enable_experimental_tc_features = instanceof. According to the commit, it looks like it already does what I suggested in my comment here.

I agree enable_experimental_tc_features = instanceof should be turned on by default, or at least move the banning of instanceof (classname<T>) underneath that option so it is consistent.

Also I think the message

'instanceof' cannot be used on a generic classname because types are erased at runtime

should be more explanatory, like

'instanceof' cannot be used on a classname of a type parameter because that type parameter may itself be generic but 'instanceof' cannot check generics because they are erased at runtime

or more simply

'instanceof' is only allowed on non-generic classes, T may be a generic class

If you agree, I'll open a new PR to update the message.

[andrewjkennedy]

Thanks @jesseschalken that's a great suggestion for improving the error message. Perhaps "may itself be an instantiated generic type". The word "generic" is overloaded - I often see people talk about "the generic" when they mean a type parameter such as T, and other times "generic" means a constructed type such as C<int>.

[fredemmott]

@jesseschalken if I'm understanding 603cbd7 correctly, this can be closed: while the safe behavior is still experimental, if the experimental flag isn't set, the old unsafe behavior is back - the latest build of the typechecker is now allowing https://github.com/facebook/fbshipit/blob/master/src/shipit/repo/ShipItRepo.php#L96-L98 to be removed without the experimental option

[fredemmott]

whoops, that should be on the issue, not here