`this` should not be allowed as an argument type in co- or contravariant classes

[acrylic-origami]

HHVM Version

$ hhvm --version
HipHop VM 3.13.1 (rel)

Standalone code, or other way to reproduce the problem

For a final class, the this type is always equivalent to that exact class. By this equivalence, using this in final classes with variant type parameters is always invalid, because the argument position is contravariant, reversing the variance of the parameters of the class type — covariant parameters become contravariantly valid, and vice versa.

this lets simple variance violations bypass typechecker scrutiny (pun...definitely intended). Here's the covariant case:

<?hh // strict
class Base {}
class Derived extends Base {
    public function __construct(public int $derived_prop) {}
}
final class ImplCov<+T> {
    public function __construct(private T $v) {}
    public function put(this $v): void {
        $this->v = $v->v;
    }
    public function pull(): T {
        return $this->v;
    }
}
class Violate {
    public static function foo(ImplCov<Derived> $v): void {
        self::bar($v);
        echo $v->pull()->derived_prop; // Wait... Base doesn't have $derived_prop!
    }
    public static function bar(ImplCov<Base> $v): void {
        $v->put(new ImplCov(new Base()));
    }
}

Expected result

Illegal usage of a {co, contra}variant type parameter

Actual result

No errors!, but Violate::foo(new ImplCov(new Derived(42))); results in:

Notice: Undefined property: Base::$derived_prop in this_violation.php on line 18

Without ImplCov being final, the typechecker complains at $v->put(new ImplCov(new Base())); in Violate::bar with Since ImplCov is not final, this might not be ImplCov. I wonder if it suffices to disallow variant final classes to inherit or define methods with this as an argument type, or if a violation can occur in non-final classes?

[jwatzman]

Interesting find -- I've poked the core Hack team internally (about this and #7216, which also didn't get a response). Hopefully I can get a better way to bring stuff to their attention that doesn't require being an FB employee :-P

[andrewjkennedy]

Good catch! I'm looking into a number of type soundness issues, especially around variance, so I'll add this one to my list :-) Andrew

[acrylic-origami]

I noticed the new rules in 7e1887c (catching this violation buried in my own code no less!) so I'm closing this for now.

[acrylic-origami]

A minor catch: type constants aren't checked for this violation. If ImplCov is rewritten:

final class ImplCov<+T> {
    const type TThis = this;
    public function __construct(private T $v) {}
    public function put(this::TThis $v): void {
        $this->v = $v->pull();
    }
    public function pull(): T {
        return $this->v;
    }
}

The code typechecks again with the same violation.

[lexidor]

I am going over old issues on this repository, to see which ones apply to the current versions of hhvm.

You now get typechecker error on both your repros. Thanks for reporting them.

Typing[4158] The "this" type cannot be used in this contravariant position because its enclosing class "ImplCov1" is final and has a variant type parameter "T"
   --> src/file.hack
 12 |     public function put(this $v): void {
    |                         ^^^^

Typing[4158] The "this" type cannot be used in this contravariant position because its enclosing class "ImplCov2" is final and has a variant type parameter "T"
   --> src/file.hack
 22 |     public function put(this::TThis $v): void {
    |                         ^^^^

2 errors found.