Ban private final methods on classes

Summary:
`final private` methods don't make sense. `final` bans overriding that method, but `private` stops child classes seeing the method.

This leads to a typehole from code of the form:

```
class MyParent {
  private final function foo(): void {}
}
class MyChild extends MyParent {
  private function foo(): void {}
}
```

HHVM rejects this, because `MyChild` is overriding a final method. hh allowed this, because `MyParent::foo` is private to `MyParent` and shouldn't affect `MyChild`.

Ban methods on classes from being both private and final.

This is a backwards compatibility break, as it bans `MyParent` even if there are no child classes. This should be an easy fix though: remove `final` from any `private` methods.

Note that this still allows `final private` methods on traits. This prevent accidental clobbering of trait methods from the using class. We might want to revisit this in future, as the same type hole exists there (see updated typehole test).

Ignore linking failure due to D32058157 breaking LTO builds.

Reviewed By: jamesjwu

Differential Revision: D28342765

fbshipit-source-id: 53adaa8a0d8a291ca6fd4c62e1f203848201a547

hphp/hack/hhi/exceptions.hhi

@@ -52,7 +52,7 @@ class Error implements Throwable {
   final public function getTraceAsString()[]: string;
   public function __toString(): string;
   public function toString(): string;
-  final private function __clone(): void;
+  private function __clone(): void;
 }
 
 class ArithmeticError extends Error {}
@@ -102,7 +102,7 @@ class Exception implements Throwable {
   final public function getTraceAsString()[]: string;
   public function __toString(): string;
   public function toString(): string;
-  final private function __clone(): void;
+  private function __clone(): void;
 
   final public static function getTraceOptions()[read_globals];
   final public static function setTraceOptions($opts)[globals];

hphp/hack/hhi/reflection.hhi

@@ -148,7 +148,7 @@ abstract class ReflectionFunctionAbstract implements Reflector {
   public function isDeprecated()[]: bool;
   public function getExtension()[];
   public function getExtensionName()[];
-  final private function __clone();
+  private function __clone();
   public function hasReturnType()[]: bool;
   public function getReturnType()[]: ?ReflectionType;
   public function getReifiedTypeParamInfo()[]: varray<darray<string, bool>>;
@@ -206,7 +206,7 @@ class ReflectionMethod extends ReflectionFunctionAbstract implements Reflector {
 class ReflectionParameter implements Reflector {
   public $name = '';
 
-  final private function __clone();
+  private function __clone();
   public static function export($function, $parameter, $return = null);
   public function __construct($function, $parameter, $info = null)[];
   public function __toString()[];
@@ -248,7 +248,7 @@ class ReflectionProperty implements Reflector {
   public $name = '';
   public $class = '';
 
-  final private function __clone();
+  private function __clone();
   public static function export($class, $name, $return = null);
   public function __construct($class, string $name)[];
   public function __toString()[];
@@ -275,7 +275,7 @@ class ReflectionExtension implements Reflector {
 
   public $name = '';
 
-  final private function __clone();
+  private function __clone();
   public static function export($name, $return = false);
   public function __construct($name)[];
   public function __toString()[];
@@ -291,7 +291,7 @@ class ReflectionExtension implements Reflector {
 
 class ReflectionTypeConstant implements Reflector {
 
-  final private function __clone();
+  private function __clone();
   public static function export($class, string $name, $return = null);
   public function __construct($class, string $name)[];
   public function __toString()[]: string;
@@ -304,7 +304,7 @@ class ReflectionTypeConstant implements Reflector {
 }
 
 class ReflectionTypeAlias implements Reflector {
-  final private function __clone();
+  private function __clone();
   final public function __construct(string $name)[];
   public function __toString()[]: string;
   public function getTypeStructure()[]: darray;
@@ -320,7 +320,7 @@ class ReflectionTypeAlias implements Reflector {
 }
 
 class ReflectionType {
-  final private function __clone();
+  private function __clone();
   public function __construct(?Reflector $param_or_ret = null,
                               darray $type_hint_info = darray[]);
   public function allowsNull()[]: bool;
@@ -329,7 +329,7 @@ class ReflectionType {
 }
 
 class ReflectionFile implements Reflector {
-  final private function __clone();
+  private function __clone();
   final public function __construct(string $name)[];
   public function __toString()[]: string;
   public function getName()[]: string;

hphp/hack/hhi/stdlib/builtins_functioncredential.hhi

@@ -13,7 +13,7 @@
   * but you almost certainly should NOT be attempting to construct this object
   */
 final class FunctionCredential {
-  private final function __construct() {}
+  private function __construct() {}
 
   public final function getClassName()[]: ?string;
 

hphp/hack/hhi/stdlib/builtins_transliterator.hhi

@@ -8,8 +8,8 @@ class Transliterator {
 
   public string $id = '';
 
-  private final function __construct();
-  private final function __init(string $idOrRules,
+  private function __construct();
+  private function __init(string $idOrRules,
                                 int $direction,
                                 bool $rules): bool;
   public static function create(
@@ -20,7 +20,7 @@ class Transliterator {
     string $rules,
     int $direction = self::FORWARD,
   ): ?Transliterator;
-  private final function __createInverse(): ?Transliterator;
+  private function __createInverse(): ?Transliterator;
   public function createInverse(): ?Transliterator;
   public function getErrorCode(): int;
   public function getErrorMessage(): string;

hphp/hack/src/errors/error_codes.ml

@@ -235,6 +235,7 @@ module NastCheck = struct
     | EntryPointGenerics [@value 3094]
     | InternalProtectedOrPrivate [@value 3095]
     | InoutInTransformedPsuedofunction [@value 3096]
+    | PrivateAndFinal [@value 3097]
   [@@deriving enum, show { with_path = false }]
 
   let err_code = to_enum

hphp/hack/src/errors/errors.ml

@@ -2081,6 +2081,12 @@ let mk_multiple_xhp_category pos =
 
 let multiple_xhp_category pos = add_error (mk_multiple_xhp_category pos)
 
+let private_and_final p =
+  add
+    (NastCheck.err_code NastCheck.PrivateAndFinal)
+    p
+    "Class methods cannot be both `private` and `final`."
+
 let return_in_gen p =
   add
     (NastCheck.err_code NastCheck.ReturnInGen)

hphp/hack/src/errors/errors.mli

@@ -1021,6 +1021,8 @@ val invalid_req_extends : Pos.t -> unit
 
 val abstract_with_body : Pos.t * 'a -> unit
 
+val private_and_final : Pos.t -> unit
+
 val return_in_gen : Pos.t -> unit
 
 val return_in_finally : Pos.t -> unit

hphp/hack/src/oxidized/gen/error_codes.rs

@@ -3,7 +3,7 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the "hack" directory of this source tree.
 //
-// @generated SignedSource<<4068eecc5db1df8b2b16fdad6281f299>>
+// @generated SignedSource<<4449a899b2c6bd520370c8f09d716fe2>>
 //
 // To regenerate this file, run:
 //   hphp/hack/src/oxidized_regen.sh
@@ -215,6 +215,7 @@ pub enum NastCheck {
     EntryPointGenerics = 3094,
     InternalProtectedOrPrivate = 3095,
     InoutInTransformedPsuedofunction = 3096,
+    PrivateAndFinal = 3097,
 }
 impl TrivialDrop for NastCheck {}
 arena_deserializer::impl_deserialize_in_arena!(NastCheck);

hphp/hack/src/typing/nast_check/nast_check.ml

@@ -31,6 +31,7 @@ let visitor =
       Trait_reuse_check.handler;
       Enum_supertyping_check.handler;
       List_rvalue_check.handler;
+      Private_final_check.handler;
     ]
 
 let stateful_visitor ctx =

hphp/hack/src/typing/nast_check/private_final_check.ml

@@ -0,0 +1,30 @@
+(*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the "hack" directory of this source tree.
+ *
+ *)
+
+[@@@warning "-33"]
+
+open Hh_prelude
+
+[@@@warning "+33"]
+
+open Aast
+
+(* Ban `private final` on classes, but not traits. *)
+
+let handler =
+  object
+    inherit Nast_visitor.handler_base
+
+    method! at_method_ env m =
+      match (env.Nast_check_env.classish_kind, m.m_visibility, m.m_final) with
+      | (Some Ast_defs.Ctrait, _, _) -> ()
+      | (_, Private, true) ->
+        let (pos, _) = m.m_name in
+        Errors.private_and_final pos
+      | _ -> ()
+  end

hphp/hack/test/typecheck/case_sensitive_inheritance8.php

@@ -3,7 +3,7 @@
 
 trait MyTrait {
 
-  final private function createBuildID(): string {
+  private function createBuildID(): string {
     return "foo";
   }
 

hphp/hack/test/typecheck/case_sensitive_inheritance8.php.exp

@@ -1,4 +1,4 @@
 File "case_sensitive_inheritance8.php", line 18, characters 27-39:
 MyA inherits a method named `createBuildID` whose name differs from this one (`createBuildId`) only by case. (Typing[4393])
-  File "case_sensitive_inheritance8.php", line 6, characters 26-38:
+  File "case_sensitive_inheritance8.php", line 6, characters 20-32:
   It was inherited from MyTrait as `createBuildI~~D~~`. If you meant to override it, please use the same casing as the inherited method. Otherwise, please choose a different name for the new method

hphp/hack/test/typecheck/final_private_trait.php

@@ -0,0 +1,11 @@
+<?hh
+
+trait MyTrait {
+  public function foo(): void { $this->bar(); }
+  final private function bar(): void {}
+}
+
+class MyClass {
+  use MyTrait;
+  private function bar(): void {} // should error, clobbering trait internals
+}

hphp/hack/test/typecheck/final_private_trait.php.exp

@@ -0,0 +1,4 @@
+File "final_private_trait.php", line 10, characters 20-22:
+You cannot override this method (Typing[4070])
+  File "final_private_trait.php", line 5, characters 26-28:
+  It was declared as final

hphp/hack/test/typecheck/new_inference/oldtypehole/t21684610.php

@@ -15,7 +15,7 @@ final public static function do<T, Ti as I1<T>>(
 }
 
 final class A2 extends A1 {
-  final private static function query(): A3 {
+  private static function query(): A3 {
     invariant_violation('_');
   }
 

hphp/hack/test/typecheck/private_final_class_meth.php.exp

@@ -0,0 +1,2 @@
+File "private_final_class_meth.php", line 5, characters 26-37:
+Class methods cannot be both `private` and `final`. (NastCheck[3097])

hphp/hack/test/typecheck/public_private.php

@@ -10,6 +10,6 @@
  */
 
 class Foo {
-  final public static private function bar(): void {
+  public static private function bar(): void {
   }
 }

hphp/hack/test/typecheck/public_private.php.exp

@@ -1,2 +1,2 @@
-File "public_private.php", line 13, characters 23-29:
+File "public_private.php", line 13, characters 17-23:
 Methods cannot have multiple visibility modifiers (Parsing[1002])

hphp/hack/test/typecheck/refinements/invoke_on_intersection_with_tconst.php

@@ -3,7 +3,7 @@
 
 abstract class C {
   const type TID = int;
-  final private function __construct(
+  private function __construct(
     private this::TID $id,
   ) {}
   final public function getID(): this::TID {

hphp/hack/test/typecheck/refinements/this_refine_intersect_2.php

@@ -12,7 +12,7 @@ abstract class TypedJSModule
   protected ?this::TExports $rawExports = null;
   abstract const type TExports as shape(...);
 
-  final private function getFallbackExportsIfExist(): ?this::TExports {
+  private function getFallbackExportsIfExist(): ?this::TExports {
     if (
       $this is ITypedJSModuleWithFallback
     ) {

hphp/hack/test/typecheck/tconst/method_contravariant.php

@@ -3,7 +3,7 @@
 abstract class X {
   abstract const type T as arraykey = string;
 
-  final private function __construct(private this::T $val) {}
+  private function __construct(private this::T $val) {}
 
   public function get(): this::T {
     return $this->val;

hphp/hack/test/typecheck/typeconsts/tany_ambiguous.php

@@ -7,7 +7,7 @@
 abstract class Base {
   const type TData = TANY;
 
-  final private function __construct(protected this::TData $data) {}
+  private function __construct(protected this::TData $data) {}
 
   final protected function getData(): this::TData {
     return $this->data;

hphp/hack/test/typecheck/typehole/T105759639.php

@@ -0,0 +1,14 @@
+<?hh
+
+trait MyTrait {
+  public function foo(): void { $this->bar(); }
+  final private function bar(): void {}
+}
+
+class MyClass {
+  use MyTrait;
+}
+
+class MyChild extends MyClass {
+  private function bar(): void {}
+}

hphp/hack/test/typecheck/visibilities/refine_shadow_2.php

@@ -3,7 +3,7 @@
 
 final class EN {
   use TR;
-  final private function asEN(): EN {
+  private function asEN(): EN {
     return $this;
   }
 

hphp/runtime/ext/asio/ext_asio.php

@@ -7,7 +7,7 @@
 <<__Sealed(StaticWaitHandle::class, WaitableWaitHandle::class)>>
 abstract class Awaitable {
 
-  final private function __construct() {
+  private function __construct() {
     throw new \InvalidOperationException(
       \get_class($this) . "s cannot be constructed directly"
     );

hphp/runtime/ext/asio/ext_wait-handle.cpp

@@ -146,7 +146,6 @@ finish_class() {
   DEBUG_ONLY auto const ctor = cls->getCtor();
   assertx(ctor == wh->getCtor());
   assertx(ctor->attrs() & AttrPrivate);
-  assertx(ctor->attrs() & AttrFinal);
 
   cls->allocExtraData();
   assertx(!cls->m_extra->m_nativeDataInfo);

hphp/runtime/ext/icu/ext_icu_iterator.php

@@ -19,7 +19,7 @@ class IntlIterator implements Iterator {
   /**
    * Only for internal use
    */
-  private final function __construct(): void {}
+  private function __construct(): void {}
 
   /**
    * Get the current element

hphp/runtime/ext/icu/ext_icu_transliterator.php

@@ -10,13 +10,13 @@ class Transliterator {
   /**
    * This class may only be instantiated by ::create or ::createFromRules
    */
-  private final function __construct() {}
+  private function __construct() {}
 
   /**
    * Setup internal transliterator object
    */
   <<__Native>>
-  private final function __init(string $idOrRules,
+  private function __init(string $idOrRules,
                                 int $direction,
                                 bool $rules): bool;
 
@@ -64,7 +64,7 @@ public static function createFromRules(string $rules,
    * Helper for ->createInverse()
    */
   <<__Native>>
-  private final function __createInverse(): ?Transliterator;
+  private function __createInverse(): ?Transliterator;
 
   /**
    * Create an inverse transliterator

hphp/system/php/lang/BaseException.ns.php

@@ -183,7 +183,7 @@ public function toString() {
     return $res;
   }
 
-  final private function __clone() {
+  private function __clone() {
     \trigger_error("Trying to clone an uncloneable object of class " .
                    \get_class($this), \E_USER_ERROR);
   }