[ Typechecker ] __ConsistentConstruct attribute on a class with a final constructor hides error in the overridden constructor

[lexidor]

Describe the bug Overriding a final method is not allowed. This is also true for constructors. The typechecker flags overrides of final methods. Adding a __ConsistentConstruct attribute to the base class hides errors in the overridden constructor. The runtime rejects the code at parse time.

Standalone code, or other way to reproduce the problem

<<__ConsistentConstruct>>
class CCHidesFinal {
  final public function __construct() {}
}

class NotDetected extends CCHidesFinal {
  public function __construct() {
    parent::__construct();
  }
}

class WithoutCC {
  final public function __construct() {}
}

class Detected extends WithoutCC {
  public function __construct() {
    parent::__construct();
  }
}

Steps to reproduce the behavior:

1. Run hh_client, and observe one error (1a)
2. Two errors were expected here.
3. Invoke hhvm on this source file, observe a fatal error (1b)
4. See error

1a.

Typing[4070] Some members in class Detected are incompatible with those declared in type WithoutCC [1]
-> You cannot override this method [2]
-> It was declared as final [3]

src/file.hack:16:24
    11 | 
    12 | class WithoutCC {
[3] 13 |   final public function __construct() {}
    14 | }
    15 | 
[1] 16 | class Detected extends WithoutCC {
[2] 17 |   public function __construct() {
    18 |     parent::__construct();
    19 |   }

1 error found.

1b.

Fatal error: Cannot override final method CCHidesFinal::__construct() in /path/to/src/file.hack on line 6

Expected behavior

This typechecker should report an error on the constructor of NotDetected.

Actual behavior

Only the constructor of Detected has an error.

Environment

• Operating system

Ubuntu 18.04

• Installation method

apt-get with dl.hhvm.com repository

• HHVM Version

HipHop VM 4.161.0-dev (rel) (non-lowptr)
Compiler: 1652671571_425595268
Repo schema: 0b708ab954ff5a1cb58291a7ac8422d3b33a07d1
hackc-7718eddae9f1e8713daefbfb454902e5bcd7dc5d-4.161.0-dev

Additional context __ConsistentConstruct doesn't really make sense on classes with final constructors. Every final constructor is always consistent. Hack does not infer that and missing out the __ConsistentConstruct disallows new {%classname<WithoutCC>%}(), even though the constructor is final.

Typing[4060] Can't use new on classname<WithoutCC>; __construct arguments are not guaranteed to be consistent in child classes [1]
-> This declaration is neither final nor uses the <<__ConsistentConstruct>> attribute [2]

src/file.hack:23:14
    20 | }
    21 | 
[2] 22 | function _x(classname<WithoutCC> $cls): WithoutCC {
[1] 23 |   return new $cls();
    24 | }

1 error found.

I found this error because I tried overriding the constructor of Facebook\HackTest\HackTest. This class has a final constructor and a __ConsistentConstruct attribute.

[lexidor]

This comment leads me to believe that the intention is to allow new $cls() without consistent construct if the constructor is final.

https://github.com/facebook/hhvm/blob/c9f8eeb17f6dd59c55d7566b48ed3eb1316faa51/hphp/hack/src/typing/typing.ml#L7837-L7839

[Wilfred]

Thanks. Filed as T126834394 internally.