[ TypeChecker ] Ban final private on classes

[lexidor]

Describe the bug
When a base class declares a final private method and a child class declares a method with the same name, only the runtime catches this error, the typechecker doesn't catch it.

Standalone code, or other way to reproduce the problem

abstract class Base {
    final private function privateFinal(): string {
        return 'Base';
    }
}

final class Child extends Base {
    private function privateFinal(): string {
        return 'Child';
    }
}

Steps to reproduce the behavior:

1. hh_client
2. hhvm %SCRIPT_NAME%

Expected behavior

The typechecker should emit an error that you are overriding a final private method of Base.

Actual behavior

The typechecker reports No errors!
HHVM reports Fatal error: Cannot override final method Base::privateFinal().

Environment

• Operating system

Ubuntu 20.04

• Installation method

apt-get with dl.hhvm.com repository

• HHVM Version

HipHop VM 4.98.1 (rel)
Compiler: 1614289828_486548987
Repo schema: d1ae8e21bf3419a65f12a010527485564e719d07
hackc-68e6c090a26fc254dc69c342e54a6469e51330b3-4.98.1

Additional context
Either private final is an error in and of itself, or Child is doing something that it should not be allowed to do.

[jthemphill]

Nice find!

private final feels like an oxymoron to me. A type shouldn't care about its parents' private methods. I'm in favor of banning private final altogether.

[jthemphill]

Filed T88186581 internally to ban final private from the language.

[lexidor]

Would this remain supported in traits? The meaning of final private in traits is different from within classes. It means that the using class can't override the method.

[jthemphill]

Yes, we’ll continue to support final private in traits.

On T88186581, @periodic1236 wrote:

I agree with the goal here, but I think we need a solution for traits first. It is bad for framework designers if a class that uses a trait can silently override a private method that was defined in the trait, causing the new implementation to be used instead (note this does not happen when extending a class). Today, final private is the only way to have the type checker signal a problem (although the runtime would allow the override, the developer would still be forced to change the trait because of Hack).

Long term, @periodic1236 advocates for changing both traits and final:

Personally, I wish we could have a concept of trait-private methods, require the remainder of such methods to be protected if they need to be used from users of the trait (unfortunately this changes the copy-paste nature of trait code, but meh), and also change methods to be final by default (requiring a keyword like "open" to allow a method to be overridden).

[Wilfred]

After doing some work on this, I kinda think we should ban it on traits too. If we only ban final private methods on classes, hh will still accept this code that HHVM rejects:

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 {}
}

@lexidor do you have any opinions here?

[lexidor]

Hmmm, almost all private methods in traits are (or should) final though. Overriding a private trait method is very counterintuitive. The trait author can not call private methods from the the trait without the using class getting a chance to override. final private although an oximoron, is the only valid defense. Unless the rules of the language change so that private trait methods can not be overridden by a using class / trait, trait authors will be forced to say final protected even though they most likely didn't want the method to be visible at all. I like the truly private trait method proposal.

[lexidor]

Linking context from the commit log: c8faa1a
This commit bans private final on classes.

[lexidor]

Exported the trait issue to #9046. Private final is banned on classes, so the title of this issue has been resolved. Closing to minimize noise for people dealing with the trait case.