Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] The curious case of the comparable objects.

Posted by Sara Golemon 
Sara Golemon
[PHP-DEV] The curious case of the comparable objects.
August 10, 2018 05:20PM
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:

1. Does this matter? (I think so, it's spooky action at a distance)
2. 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)
3. 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christoph M. Becker
[PHP-DEV] Re: The curious case of the comparable objects.
August 11, 2018 03:40PM
On 10.08.2018 at 17:12, Sara Golemon wrote:

> 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:
>
> 1. Does this matter? (I think so, it's spooky action at a distance)

ACK.

> 2. 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?

> 3. 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.

[1]
<https://github.com/php/php-langspec/blob/master/spec/10-expressions.md#user-content-relational-operators>;

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sara Golemon
[PHP-DEV] Re: The curious case of the comparable objects.
August 11, 2018 05:50PM
On Sat, Aug 11, 2018 at 8:34 AM, Christoph M. Becker <[email protected]> wrote:
>> 2. 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?
>
> [1]
> <https://github.com/php/php-langspec/blob/master/spec/10-expressions.md#user-content-relational-operators>;
>
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] The curious case of the comparable objects.
August 12, 2018 07:50AM
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 :)

> 1. Does this matter? (I think so, it's spooky action at a distance)

No. There's no defined behavior in comparing random objects.

> 2. 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.

> 3. 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Re: The curious case of the comparable objects.
August 12, 2018 07:50AM
Hi!

> <https://github.com/php/php-langspec/blob/master/spec/10-expressions.md#user-content-relational-operators>;

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christoph M. Becker
Re: [PHP-DEV] Re: The curious case of the comparable objects.
August 12, 2018 12:40PM
On 12.08.2018 at 07:47, Stanislav Malyshev wrote:

>> <https://github.com/php/php-langspec/blob/master/spec/10-expressions.md#user-content-relational-operators>;
>
> 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…

[1] https://3v4l.org/NLZNm

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
<[email protected]> 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.

> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sun, Aug 12, 2018 at 6:43 PM, Sara Golemon <[email protected]> wrote:

> On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
> <[email protected]> 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
Sorry, only registered users may post in this forum.

Click here to login