Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
class FooBar {
const bar = ["bar" => 3]["bar"];
}
It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?
Also I found a constant expression related bug, that leads to unpredictable
crashes from time to time. Previously we had IS_CONSTANT_ARRAY that was
handled in a special way. When you replaced it with IS_ARRAY, you missed
this handling, and I missed it as well when reviewed your patch. Now
IS_ARRAY constants might be incompletely copied from OPCache shared memory
and modified (incremented/decremented reference counter) directly in SHM.
Such modifications occur in simultaneously running processes and this
finally leads to crash on some race condition.
It's possible to emulate the problem running the following script with
opcache.protect_memory=1
<?php
function foo($query = NULL, array $exclude = array('q')) {}
foo();
?>
I propose a simple fix: https://gist.github.com/dstogov/b73884e252b376957ebc
Please review and apply if agree.
Thanks. Dmitry.
I'm wondered why you introduced this wired syntax in PHP-5.6.
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?
Does it harm?
This example doesn’t make much sense, but intention of such code is unambiguous. It is clear what should happen.
So, I’m +0 to leave it if tests are written.
--
Alexey Zakhlestin
CTO at Grids.by/you
https://github.com/indeyets
PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc
at least we didn't have a single test for this new syntax.
Me and probably many others just didn't know about it.
I just noticed Xinchen's commit, that fixed related bug.
May be it's safe, may be not. I cant be sure.
Thanks. Dmitry.
On Mon, Jun 30, 2014 at 11:17 PM, Alexey Zakhlestin indeyets@gmail.com
wrote:
I'm wondered why you introduced this wired syntax in PHP-5.6.
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?Does it harm?
This example doesn’t make much sense, but intention of such code is
unambiguous. It is clear what should happen.
So, I’m +0 to leave it if tests are written.--
Alexey Zakhlestin
CTO at Grids.by/you
https://github.com/indeyets
PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc
Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
Do you mean weird?
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?
I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}
?: and ? only work when there are just two possibilities.
It is surprising that it wasn’t in the RFC though, I suspect that was just an oversight on Bob’s part.
Andrea Faulds
http://ajf.me/
That wasn't in the RFC because there still was the IS_CONSTANT_ARRAY I removed only months later (and replaced with AST) to fix some bugs.
It's only at that moment where I now could add that syntax.
Bob
Am 30.6.2014 um 23:05 schrieb Andrea Faulds ajf@ajf.me:
Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
Do you mean weird?
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}?: and ? only work when there are just two possibilities.
It is surprising that it wasn’t in the RFC though, I suspect that was just an oversight on Bob’s part.
Andrea Faulds
http://ajf.me/
Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
Do you mean weird?
yes :) sorry
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}?: and ? only work when there are just two possibilities.
?: may work with brackets, but I have to admit that it's less readable.
class Foo {
const X = 2;
const Y = (Foo::X == 0 ? 1 :
(Foo::X == 1 ? 2 :
(Foo::X == 2 ? 3 :
(Foo::X == 3 ? 4 :
5))));
}
The situation is really inconsistent because we allow expressions on
constant arrays but at the same time prohibit array usage.
Look into the following scripts:
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?>
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed
in constants at run-time
?>
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
// This works again! We may declare array class constants if we don't use
them?
?>
Where is the logic?
Thanks. Dmitry.
It is surprising that it wasn’t in the RFC though, I suspect that was just
an oversight on Bob’s part.Andrea Faulds
http://ajf.me/
Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
Do you mean weird?
yes :) sorry
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}?: and ? only work when there are just two possibilities.
?: may work with brackets, but I have to admit that it's less readable.
class Foo {
const X = 2;
const Y = (Foo::X == 0 ? 1 :
(Foo::X == 1 ? 2 :
(Foo::X == 2 ? 3 :
(Foo::X == 3 ? 4 :
5))));
}The situation is really inconsistent because we allow expressions on
constant arrays but at the same time prohibit array usage.
As we support the [..., ...][...] syntax in normal PHP code as well, I
think it's reasonable to allow it here as well. Of course it isn't very
practically useful, but there doesn't seem much point to explicitly
disallowing it here.
Look into the following scripts:
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed
in constants at run-time
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
// This works again! We may declare array class constants if we don't use
them?
?>Where is the logic?
The reason here is probably that we cannot detect whether something will be
an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 :
[1] or similar. However we might want to detect the case where an array is
the root of the AST, as that is invalid for sure and the most common case.
Another small issue with the constant expressions I noticed is that our
recursion detection doesn't properly work for constant ASTs. For example:
<?php
class A {
const FOO = [self::BAR];
const BAR = [self::FOO];
}
var_dump(A::FOO);
This will result in a stack overflow instead of a fatal error about
self-referencing constants.
Nikita
Hi Bob,
I'm wondered why you introduced this wired syntax in PHP-5.6.
Do you mean weird?
yes :) sorry
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}?: and ? only work when there are just two possibilities.
?: may work with brackets, but I have to admit that it's less readable.
class Foo {
const X = 2;
const Y = (Foo::X == 0 ? 1 :
(Foo::X == 1 ? 2 :
(Foo::X == 2 ? 3 :
(Foo::X == 3 ? 4 :
5))));
}The situation is really inconsistent because we allow expressions on
constant arrays but at the same time prohibit array usage.As we support the [..., ...][...] syntax in normal PHP code as well, I think
it's reasonable to allow it here as well. Of course it isn't very
practically useful, but there doesn't seem much point to explicitly
disallowing it here.Look into the following scripts:
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed
in constants at run-time
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
// This works again! We may declare array class constants if we don't use
them?
?>Where is the logic?
The reason here is probably that we cannot detect whether something will be
an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 :
[1] or similar. However we might want to detect the case where an array is
the root of the AST, as that is invalid for sure and the most common case.Another small issue with the constant expressions I noticed is that our
recursion detection doesn't properly work for constant ASTs. For example:<?php
class A {
const FOO = [self::BAR];
const BAR = [self::FOO];
}
var_dump(A::FOO);This will result in a stack overflow instead of a fatal error about
self-referencing constants.
So what's the status ?
We release RC2 this week. Final is not far, and as I can read, it's not stable.
So I suggest we use more RCs to try to make this stable (and I mean
with OPCache activated).
If we can't make it in reasonnable time, then the feature will have to
be reverted.
Please, note we won't have too many RCs as we are already late on our
original timetable.
Julien Pauli
Am 1.7.2014 um 09:12 schrieb Nikita Popov nikita.ppv@gmail.com:
class FooBar {
const bar = ["bar" => 3]["bar"];
}It wasn't a part of RFC, it wasn't covered by tests, and it actually
doesn't make a lot of sense. May be it's better to remove it?I disagree, it makes perfect sense:
class FooBar {
const FOO = 3;
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][FOO];
}?: and ? only work when there are just two possibilities.
?: may work with brackets, but I have to admit that it's less readable.
class Foo {
const X = 2;
const Y = (Foo::X == 0 ? 1 :
(Foo::X == 1 ? 2 :
(Foo::X == 2 ? 3 :
(Foo::X == 3 ? 4 :
5))));
}The situation is really inconsistent because we allow expressions on
constant arrays but at the same time prohibit array usage.As we support the [..., ...][...] syntax in normal PHP code as well, I think it's reasonable to allow it here as well. Of course it isn't very practically useful, but there doesn't seem much point to explicitly disallowing it here.
That's exactly my point.
Look into the following scripts:
<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed
in constants at run-time
?><?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
}
// This works again! We may declare array class constants if we don't use
them?
?>Where is the logic?
The reason here is probably that we cannot detect whether something will be an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 : [1] or similar. However we might want to detect the case where an array is the root of the AST, as that is invalid for sure and the most common case.
Not sure if we should check that. That would be adding a new arbitrary restriction. Now the following works:
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
const BAZ = FOO[4];
}
var_dump(Foo::BAZ);
Another small issue with the constant expressions I noticed is that our recursion detection doesn't properly work for constant ASTs. For example:
<?php
class A {
const FOO = [self::BAR];
const BAR = [self::FOO];
}
var_dump(A::FOO);This will result in a stack overflow instead of a fatal error about self-referencing constants.
Nikita
I think that this is caused by some bugfix for opcache.
http://lxr.php.net/xref/PHP_5_6/Zend/zend_ast.c#255
That zval_copy_ctor call duplicates the AST to save it for opcache so that opcache can store it later.
But as that is called before the zend_ast_evaluate, the effects of MARK_CONSTANT_VISITED() won't affect the contents of the ast (and the zval with the constant) copied before…
@Dmitry: Removing this call (and the dtoring in the ast destructor) will break opcache and fix this bug. How to fix both now? I don't immediately see a good fix here?
Bob
Am 1.7.2014 um 13:25 schrieb Bob Weinand bobwei9@hotmail.com:
Not sure if we should check that. That would be adding a new arbitrary restriction. Now the following works:
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];const BAZ = FOO[4];
}var_dump(Foo::BAZ);
Woops, mistake in code… written too fast...
Should be:
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
];
const BAZ = Foo::BAR[4];
}
var_dump(Foo::BAZ);
http://3v4l.org/78S2C#v560beta2
Bob