Skip to content

Add support for final constants #6878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kocsismate wants to merge 6 commits into from
Closed

Add support for final constants #6878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a6fc677
Add support for final constants
kocsismate Apr 18, 2021
14c05ea
Make interface constants overridable by default
kocsismate May 13, 2021
22244d1
Address review comments
kocsismate Jul 3, 2021
cd2506f
Fix constant inheritance check
nikic Jul 6, 2021
b46cf28
Use correct constants table
nikic Jul 6, 2021
0fbf3fe
Improve error message
nikic Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add support for final constants
  • Loading branch information
@kocsismate @nikic
kocsismate authored and nikic committed Jul 6, 2021
commit a6fc677f3102edbeff547c415504d623d561dcdc
13 changes: 13 additions & 0 deletions Zend/tests/constants/final_constants/final_const1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
--TEST--
Class constants support the final modifier
--FILE--
<?php

class Foo
{
final const A = "foo";
final public const B = "foo";
}

?>
--EXPECT--
18 changes: 18 additions & 0 deletions Zend/tests/constants/final_constants/final_const2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
--TEST--
Final class constants cannot be overridden
--FILE--
<?php

class Foo
{
final const A = "foo";
}

class Bar extends Foo
{
const A = "bar";
}

?>
--EXPECTF--
Fatal error: Bar::A cannot override final constant Foo::A in %s on line %d
13 changes: 13 additions & 0 deletions Zend/tests/constants/final_constants/final_const3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
--TEST--
Private class constants cannot be final
--FILE--
<?php

class Foo
{
private final const A = "foo";
}

?>
--EXPECTF--
Fatal error: Private constant Foo::A cannot be final as it is never overridden in %s on line %d
Copy link
Contributor

@TysonAndre TysonAndre May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@kocsismate kocsismate May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

2 changes: 1 addition & 1 deletion Zend/tests/errmsg_038.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ class test {
echo "Done\n";
?>
--EXPECTF--
Fatal error: Cannot declare property test::$var final, the final modifier is allowed only for methods and classes in %s on line %d
Fatal error: Cannot declare property test::$var final, the final modifier is allowed only for methods, classes, and class constants in %s on line %d
11 changes: 9 additions & 2 deletions Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7270,7 +7270,7 @@ void zend_compile_prop_decl(zend_ast *ast, zend_ast *type_ast, uint32_t flags, z

if (flags & ZEND_ACC_FINAL) {
zend_error_noreturn(E_COMPILE_ERROR, "Cannot declare property %s::$%s final, "
"the final modifier is allowed only for methods and classes",
"the final modifier is allowed only for methods, classes, and class constants",
ZSTR_VAL(ce->name), ZSTR_VAL(name));
}

Expand Down Expand Up @@ -7358,10 +7358,17 @@ void zend_compile_class_const_decl(zend_ast *ast, uint32_t flags, zend_ast *attr
zend_string *doc_comment = doc_comment_ast ? zend_string_copy(zend_ast_get_str(doc_comment_ast)) : NULL;
zval value_zv;

if (UNEXPECTED(flags & (ZEND_ACC_STATIC|ZEND_ACC_ABSTRACT|ZEND_ACC_FINAL))) {
if (UNEXPECTED(flags & (ZEND_ACC_STATIC|ZEND_ACC_ABSTRACT))) {
zend_check_const_and_trait_alias_attr(flags, "constant");
}

if (UNEXPECTED((flags & ZEND_ACC_PRIVATE) && (flags & ZEND_ACC_FINAL))) {
zend_error_noreturn(
E_COMPILE_ERROR, "Private constant %s::%s cannot be final as it is never overridden",
ZSTR_VAL(ce->name), ZSTR_VAL(name)
);
}

zend_const_expr_to_zval(&value_zv, value_ast_ptr);
c = zend_declare_class_constant_ex(ce, name, &value_zv, flags, doc_comment);

Expand Down
7 changes: 7 additions & 0 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,13 @@ static void do_inherit_class_constant(zend_string *name, zend_class_constant *pa
zend_error_noreturn(E_COMPILE_ERROR, "Access level to %s::%s must be %s (as in class %s)%s",
ZSTR_VAL(ce->name), ZSTR_VAL(name), zend_visibility_string(ZEND_CLASS_CONST_FLAGS(parent_const)), ZSTR_VAL(parent_const->ce->name), (ZEND_CLASS_CONST_FLAGS(parent_const) & ZEND_ACC_PUBLIC) ? "" : " or weaker");
}

if (UNEXPECTED((ZEND_CLASS_CONST_FLAGS(parent_const) & ZEND_ACC_FINAL))) {
zend_error_noreturn(
E_COMPILE_ERROR, "%s::%s cannot override final constant %s::%s",
ZSTR_VAL(ce->name), ZSTR_VAL(name), ZSTR_VAL(parent_const->ce->name), ZSTR_VAL(name)
);
}
} else if (!(ZEND_CLASS_CONST_FLAGS(parent_const) & ZEND_ACC_PRIVATE)) {
if (Z_TYPE(parent_const->value) == IS_CONSTANT_AST) {
ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED;
Expand Down
6 changes: 6 additions & 0 deletions ext/reflection/php_reflection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3831,6 +3831,12 @@ ZEND_METHOD(ReflectionClassConstant, isProtected)
}
/* }}} */

/* Returns whether this constant is final */
ZEND_METHOD(ReflectionClassConstant, isFinal)
{
_class_constant_check_flag(INTERNAL_FUNCTION_PARAM_PASSTHRU, ZEND_ACC_FINAL);
}

/* {{{ Returns a bitfield of the access modifiers for this constant */
ZEND_METHOD(ReflectionClassConstant, getModifiers)
{
Expand Down
2 changes: 2 additions & 0 deletions ext/reflection/php_reflection.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,8 @@ public function isPrivate(): bool {}
/** @tentative-return-type */
public function isProtected(): bool {}

public function isFinal(): bool {}

/** @tentative-return-type */
public function getModifiers(): int {}

Expand Down
6 changes: 5 additions & 1 deletion ext/reflection/php_reflection_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: d8e686125cf213e019c1d706867e3c178fa057d2 */
* Stub hash: 2564122a201ca462ee35ead1562c94da3ea3c8a3 */

ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(arginfo_class_Reflection_getModifierNames, 0, 1, IS_ARRAY, 0)
ZEND_ARG_TYPE_INFO(0, modifiers, IS_LONG, 0)
Expand Down Expand Up @@ -396,6 +396,8 @@ ZEND_END_ARG_INFO()

#define arginfo_class_ReflectionClassConstant_isProtected arginfo_class_ReflectionFunctionAbstract_inNamespace

#define arginfo_class_ReflectionClassConstant_isFinal arginfo_class_ReflectionFunctionAbstract_hasTentativeReturnType

#define arginfo_class_ReflectionClassConstant_getModifiers arginfo_class_ReflectionFunctionAbstract_getNumberOfParameters

#define arginfo_class_ReflectionClassConstant_getDeclaringClass arginfo_class_ReflectionMethod_getDeclaringClass
Expand Down Expand Up @@ -736,6 +738,7 @@ ZEND_METHOD(ReflectionClassConstant, getValue);
ZEND_METHOD(ReflectionClassConstant, isPublic);
ZEND_METHOD(ReflectionClassConstant, isPrivate);
ZEND_METHOD(ReflectionClassConstant, isProtected);
ZEND_METHOD(ReflectionClassConstant, isFinal);
ZEND_METHOD(ReflectionClassConstant, getModifiers);
ZEND_METHOD(ReflectionClassConstant, getDeclaringClass);
ZEND_METHOD(ReflectionClassConstant, getDocComment);
Expand Down Expand Up @@ -1015,6 +1018,7 @@ static const zend_function_entry class_ReflectionClassConstant_methods[] = {
ZEND_ME(ReflectionClassConstant, isPublic, arginfo_class_ReflectionClassConstant_isPublic, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, isPrivate, arginfo_class_ReflectionClassConstant_isPrivate, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, isProtected, arginfo_class_ReflectionClassConstant_isProtected, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, isFinal, arginfo_class_ReflectionClassConstant_isFinal, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, getModifiers, arginfo_class_ReflectionClassConstant_getModifiers, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, getDeclaringClass, arginfo_class_ReflectionClassConstant_getDeclaringClass, ZEND_ACC_PUBLIC)
ZEND_ME(ReflectionClassConstant, getDocComment, arginfo_class_ReflectionClassConstant_getDocComment, ZEND_ACC_PUBLIC)
Expand Down
41 changes: 41 additions & 0 deletions ext/reflection/tests/ReflectionClassConstant_basic1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ function reflectClassConstant($base, $constant) {
var_dump($constInfo->isPrivate());
echo "isProtected():\n";
var_dump($constInfo->isProtected());
echo "isFinal():\n";
var_dump($constInfo->isFinal());
echo "getModifiers():\n";
var_dump($constInfo->getModifiers());
echo "getDeclaringClass():\n";
Expand All @@ -34,12 +36,14 @@ class TestClass {
/** Another doc comment */
protected const PROT = 4;
private const PRIV = "keepOut";
public final const FINAL = "foo";
}
$instance = new TestClass();

reflectClassConstant("TestClass", "PUB");
reflectClassConstant("TestClass", "PROT");
reflectClassConstant("TestClass", "PRIV");
reflectClassConstant("TestClass", "FINAL");
reflectClassConstant($instance, "PRIV");
reflectClassConstant($instance, "BAD_CONST");

Expand All @@ -61,6 +65,8 @@ isPrivate():
bool(false)
isProtected():
bool(false)
isFinal():
bool(false)
getModifiers():
int(1)
getDeclaringClass():
Expand Down Expand Up @@ -88,6 +94,8 @@ isPrivate():
bool(false)
isProtected():
bool(true)
isFinal():
bool(false)
getModifiers():
int(2)
getDeclaringClass():
Expand Down Expand Up @@ -115,6 +123,8 @@ isPrivate():
bool(true)
isProtected():
bool(false)
isFinal():
bool(false)
getModifiers():
int(4)
getDeclaringClass():
Expand All @@ -125,6 +135,35 @@ object(ReflectionClass)#3 (1) {
getDocComment():
bool(false)

**********************************
**********************************
Reflecting on class constant TestClass::FINAL

__toString():
string(41) "Constant [ public string FINAL ] { foo }
"
getName():
string(5) "FINAL"
getValue():
string(3) "foo"
isPublic():
bool(true)
isPrivate():
bool(false)
isProtected():
bool(false)
isFinal():
bool(true)
getModifiers():
int(1)
getDeclaringClass():
object(ReflectionClass)#3 (1) {
["name"]=>
string(9) "TestClass"
}
getDocComment():
bool(false)

**********************************
**********************************
Reflecting on class constant TestClass::PRIV
Expand All @@ -142,6 +181,8 @@ isPrivate():
bool(true)
isProtected():
bool(false)
isFinal():
bool(false)
getModifiers():
int(4)
getDeclaringClass():
Expand Down
10 changes: 0 additions & 10 deletions tests/classes/constants_visibility_007.phpt
View file
Open in desktop

This file was deleted.