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

[Nikita Popov]

On Sun, Aug 12, 2018 at 6:43 PM, Sara Golemon  wrote:

> On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
>  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.
>
> >>  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
>
>

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

[Sara Golemon]

On Sun, Aug 12, 2018 at 12:47 AM, Stanislav Malyshev
 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

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

[Christoph M. Becker]

On 12.08.2018 at 07:47, Stanislav Malyshev wrote:

>> 
> 
> 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] 

-- 
Christoph M. Becker

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

[Stanislav Malyshev]

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
smalys...@gmail.com

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

[Sara Golemon]

On Sat, Aug 11, 2018 at 8:34 AM, Christoph M. Becker  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]
> 
>
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

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

[Christoph M. Becker]

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]


-- 
Christoph M. Becker

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php