One of the contributors for the "Because, PHP" page came up with a fun
example where the result of object comparison changes upon observation
of that object.
class A { public $a; }
class B extends A { public $b; }
$a = new B(); $a->a = 0; $a->b = 1;
$b = new B(); $b->a = 1; $b->b = 0;
var_dump($a < $b); // bool(true)
print_r($a); // Output is unimportant
var_dump($a < $b); // bool(false)
Tracked this down to the introduction of slotted object properties
introduced in PHP 5.4 and some optimistic logic contained in
zend_object_handlers.c.
TL;DR - The print_r is materializing the ->properties HashTable in the
first object. Once that happens, zend_std_compare_objects() flips
from walking the slotted properties in properties_table to
materializing both ->propoperties HashTables and using a symtable
compare. The result differs, because rebuild_object_properties walks
->properties_info which may not necessarily be in the same order as
the values ->properties_table.
See also my writeup here: https://3v4l.org/NLZNm
So the questions for us are:
- Does this matter? (I think so, it's spooky action at a distance)
- Is it a bug introduced in 5.4 that's okay to fix? Or would fixing
it count as a BC break due to how long it's been broken? (I say
fixable bug, the BC break was at 5.4) - If yes to 1&2, how far back do we fix it? Bugfix branches (7.1+)?
Or would a change like this in branch be too much? Surely at least 7.3
could be fixed.
-Sara
One of the contributors for the "Because, PHP" page came up with a fun
example where the result of object comparison changes upon observation
of that object.class A { public $a; }
class B extends A { public $b; }
$a = new B(); $a->a = 0; $a->b = 1;
$b = new B(); $b->a = 1; $b->b = 0;var_dump($a < $b); // bool(true)
print_r($a); // Output is unimportant
var_dump($a < $b); // bool(false)Tracked this down to the introduction of slotted object properties
introduced in PHP 5.4 and some optimistic logic contained in
zend_object_handlers.c.TL;DR - The print_r is materializing the ->properties HashTable in the
first object. Once that happens, zend_std_compare_objects() flips
from walking the slotted properties in properties_table to
materializing both ->propoperties HashTables and using a symtable
compare. The result differs, because rebuild_object_properties walks
->properties_info which may not necessarily be in the same order as
the values ->properties_table.See also my writeup here: https://3v4l.org/NLZNm
So the questions for us are:
- Does this matter? (I think so, it's spooky action at a distance)
ACK.
- Is it a bug introduced in 5.4 that's okay to fix? Or would fixing
it count as a BC break due to how long it's been broken? (I say
fixable bug, the BC break was at 5.4)
I tend to agree, even though the behavior is not really documented. The
php.net manual says nothing about the ordering of objects, and the
language specification isn't clear about that, since it refers to array
comparison[1], but doesn't define the order of the properties. Are
properties of parent classes inserted before the properties of child
classes? Are ad-hoc properties inserted after the predefined ones? Are
trait properties inserted where the are “use”d? Are invisible
properties also part of the comparison?
- If yes to 1&2, how far back do we fix it? Bugfix branches (7.1+)?
Or would a change like this in branch be too much? Surely at least 7.3
could be fixed.
Fixing this for 7.3 is certainly okay; I'm not sure about former versions.
--
Christoph M. Becker
- Is it a bug introduced in 5.4 that's okay to fix? Or would fixing
it count as a BC break due to how long it's been broken? (I say
fixable bug, the BC break was at 5.4)I tend to agree, even though the behavior is not really documented. The
php.net manual says nothing about the ordering of objects, and the
language specification isn't clear about that, since it refers to array
comparison[1], but doesn't define the order of the properties. Are
properties of parent classes inserted before the properties of child
classes? Are ad-hoc properties inserted after the predefined ones? Are
trait properties inserted where the are “use”d? Are invisible
properties also part of the comparison?
Yeah, that's why I'm hesitant on just slapping a bug fix on it and
apply to 7.1+ without asking for input. If this were a younger
regression I might just do it and move on, but 5.4-7.2 is a six
release branches.
FWIW, objects with ad-hoc properties are not impacted by this since
they automatically have a HashTable for their properties. This only
impacts fully defined classes (the best kind) who have not had their
shadow table materialized (also the best kind).
PR, by the way: https://github.com/php/php-src/pull/3434
-Sara
Hi!
Ah, I forgot that the spec did define it via array comparison. The spec
also says in
https://github.com/php/php-langspec/blob/master/spec/08-conversions.md#converting-to-array-type:
The order of insertion of the elements into the array is the lexical
order of the instance properties in the class-member-declarations list.
It does not explicitly say array conversion using when comparing, but
reasonable reader would certainly assume so.
So looks like this part of the spec is violated, and I stand corrected -
it's actually defined in this case, and should be fixed to follow the
spec. I still think it makes little sense, but we have the spec, so we
have to follow the spec.
In this case, it's ok to fix it in all active versions - the spec wins I
think. Or, we have to change the spec :)
Stas Malyshev
smalyshev@gmail.com
Ah, I forgot that the spec did define it via array comparison. The spec
also says in
https://github.com/php/php-langspec/blob/master/spec/08-conversions.md#converting-to-array-type:The order of insertion of the elements into the array is the lexical
order of the instance properties in the class-member-declarations list.It does not explicitly say array conversion using when comparing, but
reasonable reader would certainly assume so.
Ah, thanks! However, the given example[1] does not violate the spec,
since the spec says nothing about the order of inherited properties.
Users may rely on a certain order, so the spec should at least state
expicitly that the order of inherited properties is undefined. It might
be preferable, though, to actually specify the order of inherited
properties (first child, then parent, then grandparent, etc.) Also the
spec should either define the order of runtime-created properties, or
declare the order to be undefined. And the spec should not forget about
properties of “use”d traits…
--
Christoph M. Becker
On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:
Undefined behavior is undefined :)
(Ignoring your followup for a moment)
Even for undefined behavior, we should try to make the behavior
repeatable. I know we wouldn't need to, but we should always go for
least surprise.
It's the side-effect hiding in the array cast from print_r/var_dump or
certain other presumable "read-only" actions causing the
interpretation of of the object compare that's maximum surprise.
So looks like this part of the spec is violated, and I stand corrected -
it's actually defined in this case, and should be fixed to follow the
spec. I still think it makes little sense, but we have the spec, so we
have to follow the spec.In this case, it's ok to fix it in all active versions - the spec wins I
think. Or, we have to change the spec :)
Personally, I vote for fix on active branches, but it's not an urgent
bug, so I'll give some more time for feedback before pushing the PR
below.
Depends on what the "fix" is, I assume.
I went with https://github.com/php/php-src/pull/3434 which has the
same overall complexity, though several more operations per element so
it'll be nominally less performant. Alternate implementations
welcome.
-Sara
On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:Undefined behavior is undefined :)
(Ignoring your followup for a moment)
Even for undefined behavior, we should try to make the behavior
repeatable. I know we wouldn't need to, but we should always go for
least surprise.
It's the side-effect hiding in the array cast from print_r/var_dump or
certain other presumable "read-only" actions causing the
interpretation of of the object compare that's maximum surprise.<https://github.com/php/php-langspec/blob/master/spec/10-
expressions.md#user-content-relational-operators>So looks like this part of the spec is violated, and I stand corrected -
it's actually defined in this case, and should be fixed to follow the
spec. I still think it makes little sense, but we have the spec, so we
have to follow the spec.In this case, it's ok to fix it in all active versions - the spec wins I
think. Or, we have to change the spec :)Personally, I vote for fix on active branches, but it's not an urgent
bug, so I'll give some more time for feedback before pushing the PR
below.
I'd recommend against fixing active branches -- this is quite likely to
break some tests using sort() somewhere (even if there is no resulting
functional change), while the fix itself is probably very niche.
Nikita
Depends on what the "fix" is, I assume.
I went with https://github.com/php/php-src/pull/3434 which has the
same overall complexity, though several more operations per element so
it'll be nominally less performant. Alternate implementations
welcome.-Sara
Hi!
One of the contributors for the "Because, PHP" page came up with a fun
example where the result of object comparison changes upon observation
of that object.
Undefined behavior is undefined :)
- Does this matter? (I think so, it's spooky action at a distance)
No. There's no defined behavior in comparing random objects.
- Is it a bug introduced in 5.4 that's okay to fix? Or would fixing
it count as a BC break due to how long it's been broken? (I say
fixable bug, the BC break was at 5.4)
Depends on what you mean by "fix". I do not think we need to commit to
any defined behavior comparing two random objects. It's not an operation
that IMHO makes any sense. However, if you have any idea of improvement
here, it'd be OK to implement even if behavior changes - as I said,
undefined behavior is undefined.
- If yes to 1&2, how far back do we fix it? Bugfix branches (7.1+)?
Or would a change like this in branch be too much? Surely at least 7.3
could be fixed.
Depends on what the "fix" is, I assume.
Stas Malyshev
smalyshev@gmail.com