Leave all ambient type parameters in scope while instantiating 'as' constraints
commitcd087be1385834d2b0c5905f91409cbd4182b48a
authorJake Bailey (Hacklang) <jakebailey@fb.com>
Wed, 6 Feb 2019 05:15:19 +0000 (5 21:15 -0800)
committerHhvm Bot <hhvm-bot@users.noreply.github.com>
Wed, 6 Feb 2019 05:21:16 +0000 (5 21:21 -0800)
treec20500e5d1313bc1e56330b018afceae840ec023
parentc5662d93ffef276a8cca2d5107b8ec4207c2e720
Leave all ambient type parameters in scope while instantiating 'as' constraints

Summary:
Currently, the typechecker fails to detect the error in the added test case class_tparam_substitution_conflicts_with_method_tparam2, where we violate a where constraint by passing `C::class` when `C` is not a subtype of `B`:

```
abstract class BaseIterable<Tv> {
  public function filterInstanceOf<Tout>(
    classname<Tout> $subclass,
  ): BaseIterable<Tout> where Tout as Tv {
    throw new Exception('unimplemented');
  }
}

abstract class AbstractMappedIterable<Tin, Tout>
  extends BaseIterable<Tout> {}

class MappedIterable<Tv, Tnext>
  extends AbstractMappedIterable<Tv, Tnext> {}

class A {}
class B {}
class C {}

function test(MappedIterable<A, B> $iter): BaseIterable<C> {
  // Not allowed; this MappedIterable extends BaseIterable<B>,
  // and C is not a subtype of B
  return $iter->filterInstanceOf(C::class);
}
```

This occurs because when we instantiate the type of `filterInstanceOf` in the context of `AbstractMappedIterable`, we substitute `Tv` with `Tout` (because `AbstractMappedIterable` extends `BaseIterable<Tout>`), producing this:

```
  public function filterInstanceOf<Tout>(
    classname<Tout> $subclass,
  ): BaseIterable<Tout> where Tout as Tout
```

There is no distinction between the `Tout` introduced as a type parameter to `AbstractMappedIterable` and the `Tout` introduced as a type parameter to `filterInstanceOf`, so we end up with the trivial constraint `Tout as Tout`, which does not provide the guarantee we get in `BaseIterable` (where we must provide `filterInstanceOf` with a subtype of the element type).

The problem is propagated to `MappedIterable`, since we use `MappedIterable`'s substitution context to first instantiate the method's type in the context of `AbstractMappedIterable`, then instantiate it in the context of `MappedIterable`. The method is given the same type as in `AbstractMappedIterable`, with the trivial `Tout as Tout` constraint (and no reference to `MappedIterable`'s actual output type, `Tnext`).

I found that I was able to at least resolve this problem in `MappedIterable`  (but not `AbstractMappedIterable`) with a small change to Decl_instantiate. Instantiation currently doesn't seem to behave correctly for methods like `filterInstanceOf`, which use one of the class type parameters to constrain a method type parameter. When instantiating a generic function, we remove class type parameters which share a name with any of the method type parameters from our substitution, making it impossible to express that a method type parameter is constrained by a class type parameter with the same name. If we instead use the original substitution while instantiating the constraint, we behave a bit better, producing the correct type in subclasses of `AbstractMappedIterable` which don't define a class type parameter named `Tout` (even if we still produce `Tout as Tout` in `AbstractMappedIterable` itself). This results in the correct type for `MappedIterable::filterInstanceOf`:

```
  public function filterInstanceOf<Tout>(
    classname<Tout> $subclass,
  ): BaseIterable<Tout> where Tout as Tnext
```

We should address the fact that there is no distinction between the class type parameter `Tout` and the method type parameter `Tout` in `AbstractMappedIterable`, but that problem does not block this stack, so I have not tried to solve it.

Reviewed By: Wilfred, arxanas

Differential Revision: D13829887

fbshipit-source-id: 7b963ccb864080ddad1e1bf7cab8310cfe2bc816
hphp/hack/src/decl/decl_instantiate.ml
hphp/hack/test/typecheck/class_tparam_substitution_conflicts_with_method_tparam1.php [new file with mode: 0644]
hphp/hack/test/typecheck/class_tparam_substitution_conflicts_with_method_tparam1.php.exp [new file with mode: 0644]
hphp/hack/test/typecheck/class_tparam_substitution_conflicts_with_method_tparam2.php [new file with mode: 0644]
hphp/hack/test/typecheck/class_tparam_substitution_conflicts_with_method_tparam2.php.exp [new file with mode: 0644]