[ 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.

hhvm/hphp/hack/src/typing/typing.ml

Lines 7837 to 7839 in c9f8eeb

	(* When computing the classes for a new T() where T is a generic,
	* the class must be consistent (final, final constructor, or
	* <<__ConsistentConstruct>>) for its constructor to be considered *)

[Wilfred]

Thanks. Filed as T126834394 internally.