Add support for final constants

[kocsismate]

RFC: https://wiki.php.net/rfc/final_class_const

[kamil-tekiela]

Why was this not supported before?

[kocsismate]

Why was this not supported before?

I honestly don't have an idea. Maybe it didn't seem important enough (?).

[mvorisek]

https://3v4l.org/EYL34 - the final modifier is allowed only for methods and classes

That error should be updated as well.

[krakjoe]

@kocsismate could you update the opening comment with a link to the rfc, so it's front and center please.

PS, this needs a rebase.

[kocsismate]

could you update the opening comment with a link to the rfc, so it's front and center please.

Yeah, of course! I've just done so.

Thanks for the heads up with with the rebase, I'll have to implement the second half of the RFC, so I'll do it soon when I manage to come back to it. :)

[TysonAndre]

Not really a major issue since it's impossible for existing code to use private final const so there's no bc break.

It seems allowed to override private constants with public constants and static:: will use the public constants, e.g. in php 7.4

Also, the fact that private final is forbidden isn't documented in the rfc right now? https://wiki.php.net/rfc/inheritance_private_methods

php > class A { public const A = 'const A::A'; public static function dump() { var_dump(static::A); } }
php > class B extends A { public const A = 'const B::A'; }
php > B::dump();
string(10) "const B::A"


php > class X { private function foo() { echo "in X::foo\n";} public function dump() { $this->foo(); } }
php > class Y extends X { public function foo() { echo "In Y::foo\n"; } }
php > (new Y())->dump();
in X::foo

---

I guess updating the issue message may be better, e.g. as it would prevent subclasses from accessing a constant of that name but I don't know why that would be a compelling reason to forbid private final - it'd also be a compile error if those subclasses tried to declare the constant to override protected final

Somewhat reminds me of https://externals.io/message/110540#110576

---

LGTM otherwise

[kocsismate]

First of all, thank you for the review!

Also, the fact that private final is forbidden isn't documented in the rfc right now?

Yeah, this is quite unfortunate... I realized that it's not documented just after starting the vote. :(

I don't know why that would be a compelling reason to forbid private final - it'd also be a compile error if those subclasses tried to declare the constant to override protected final

The reason why final private is forbidden is to be consistent with https://wiki.php.net/rfc/inheritance_private_methods indeed. So In my opinion (which might be wrong of course) if private methods (- constructors) cannot be final then the same should apply for constants. Unfortunately, I'm not 100% sure what exact issue is that you see, so could you please clarify it?

[nikic]

The case of interfaces is interesting, and I think missing some test coverage. First case:

interface I1 {
    const C = 1;
}
interface I2 {
    const C = 2;
}
class C implements I1, I2 {
}

This should (and does) error, because the constant is inherited from two different interfaces.

interface I1 {
    const C = 1;
}
interface I2 {
    const C = 2;
}
class C implements I1, I2 {
    const C = 3;
}

This should not (and does not) error, because the constant is explicit redeclared.

interface I1 {
    const C = 1;
}
interface I2 {
    const C = 2;
}
interface I3 extends I1, I2 {
    const C = 3;
}

I believe that by analogy with the previous case, this shouldn't error, but currently does.

class C {
    const C = 1;
}
interface I {
    const C = 2;
}
class C2 extends C implements I {}

Here we have two constants, one coming from the parent class, one from the interface. Currently it doesn't error and uses the value from the class. I think this should error.

I suspect the fact that interfaces may introduce "multiple parent constants" might have been the original reason why we had the inheritance restriction there. I hadn't considered this before. As outlined above, I think we can have reasonable behavior here, but the currently implemented one isn't correct.

[nikic]

Ping :) Please tell me if you need help.

[kocsismate]

Ping :) Please tell me if you need help.

Thanks for the offer, I'll definitely ask for help if I am stuck :) TBH I don't really have much free time these days, so I haven't yet tried to fix the issues you pointed out. I'm going to take a stab this weekend, unless I completely get passed out by the 2nd vaccine ^^

[kocsismate]

I addressed all the review comments, except for the most important one :( I just couldn't find out how to adjust the inheritance rules. Or should we track whether a constant was inherited when adding them to the constant table (e.g. by using a new flag)? Or is it still not enough to do?

[nikic]

@kocsismate Does this look right to you?

[kocsismate]

Does this look right to you?

Yeah, very nice! :)

[kocsismate]

Closing via a5360e8