Add ReflectionReference::getRefcount()

[nikic]

And don't return null for rc=1 references. Leave it to the user to decide whether or not they want to consider these as references or not.

This is for https://bugs.php.net/bug.php?id=78263. /cc @nicolas-grekas

[nicolas-grekas]

Good idea, I'll try this once it's merged! (I'm using devilbox-php-fpm-7-4 to run my experiments :) )

[nikic]

Merged as 428cfdd. Please report back if there's issues ... we still have time to change the design here :)

[nicolas-grekas]

Would it make sense to have ReflectionReference NOT increase the refcount?
We could just snapshot Z_REF(intern->obj) when creating the instance?
Yes, this could mean that you could snapshot, destroy the ref, and still use the ReflectionReference.
But I think that's OK, at least better than the other unexpected side-effect described in https://bugs.php.net/77951

[nicolas-grekas]

this line should be removed too :)

[nikic]

Would it make sense to have ReflectionReference NOT increase the refcount?
We could just snapshot Z_REF(intern->obj) when creating the instance?

It's possible, but I think having a ReflectionReference that refers to a no longer existing reference may be rather unexpected. For example there may then be two ReflectionReference instances returning the same ID (one of them referring to a dead reference whose address was reused). It would also preclude future API extensions that need to access the contents of the reference.

I think that once again it is better to leave this choice to the user -- if they don't want the ReflectionReference to hold the reference, destroy it. More or less do ReflectionReference::fromArrayElement(...)->getId() and don't store the ReflectionReference instance (in which case it is your own responsibility to keep the reference alive to avoid ID conflicts).

Yes, this could mean that you could snapshot, destroy the ref, and still use the ReflectionReference.
But I think that's OK, at least better than the other unexpected side-effect described in https://bugs.php.net/77951

What is the unexpected side-effect? (Edit: Ah okay, I see what you mean.)

[nicolas-grekas]

there may then be two ReflectionReference instances returning the same ID (one of them referring to a dead reference whose address was reused)

I get that but I fail to see where this could matter: one doesn't inspect structures and destroys them at the same time. See e.g. spl_object_hash() which has the same "issue", and all is fine in practice.

It would also preclude future API extensions that need to access the contents of the reference.

I think that's not a desired extension: when you create the reflector, you DO have access to both the reference and its value. If one wants to do something with them, one should just keep them around. Having them in the reflector too wouldn't open anything not doable without.

[nikic]

It would also preclude future API extensions that need to access the contents of the reference.

I think that's not a desired extension: when you create the reflector, you DO have access to both the reference and its value. If one wants to do something with them, one should just keep them around. Having them in the reflector too wouldn't open anything not doable without.

I meant value here in a more general sense as in everything part of the reference (which is the value itself, the refcount, referenced typed properties and some GC information). This is already a problem with the getRefcount() API added here, which would then return a static value rather than track the current state of the reference. Similar for a possible future getReferencedProperties() API. This goes against what other Reflection APIs do, which track current state rather than state at the time of construction. If you create a ReflectionObject, change the object and then ask it for properties, you'll get the changed properties rather than the original ones.

From my perspective keeping hold of the reference is both more consistent overall and more powerful in what can be done -- it gives you both the option of tracking current state (keep ReflectionReference and query later) or getting the state at construction (query state and discard ReflectionReference).

[nicolas-grekas]

Here is my trouble:

$a = [0, 1];

foreach ($a as &$v) {
}

$a[] = &$v;

$b = [];
$b[] = &$b;

$a[] = $b;
unset($b);

var_dump(ReflectionReference::fromArrayElement($a, 0)->getRefcount());
var_dump(ReflectionReference::fromArrayElement($a[3], 0)->getRefcount());

$b = $a;
$b[0] = 123;
var_dump($a[0]); // echoes int(0)

$b = $a[3];
$b[0] = 123;
var_dump($a[3][0]); // echoes int(123)

Apparently, both refs have the same refcount, but they behave differently. How do I differentiate both, without modifying an array copy, which is incompatible with typed properties?

[nicolas-grekas]

I managed to detect the situation by mutating the ref when it is an array with refcount=1
This can only happen to arrays, isn't it?
See https://github.com/symfony/symfony/pull/31303/files#r305714577

[nikic]

@nicolas-grekas Took me a while to understand this ... turns out we have a special-case for exactly that case in place here:

php-src/Zend/zend_hash.c

Lines 1968 to 1970 in 92d6720

	if (Z_ISREF_P(data) && Z_REFCOUNT_P(data) == 1 &&
	(Z_TYPE_P(Z_REFVAL_P(data)) != IS_ARRAY ||
	Z_ARRVAL_P(Z_REFVAL_P(data)) != source)) {

I'm not sure what to do about this. I don't think it should fall to you to detect this hack, and it might not work in the future (typed arrays).

Thanks to the fromArrayElem() constructor we could easily detect this case though. I'm wondering if we should go back to the previous variant where singleton references are ignored, apart from this one special case? Or else provide a method to property handle this case (not sure what it should be called).

[nicolas-grekas]

For typed arrays, maybe they would be implemented as objects, so this wouldn't affect the is_array() call?
But yes:

I'm wondering if we should go back to the previous variant where singleton references are ignored, apart from this one special case?

that would make sense to me, because this is really unexpected (from the pov of ReflectionReference), and while I have a test case that reminds me about this, many will not know and fall into the trap.
(we could still keep getRefcount if that's useful to some? doesn't hurt at least, isn't it?)

[nikic]

I've reverted this change in 19588a8 and added the special case in c817b80. We can add getRefcount() back if someone has a use-case, but until then I'd like to keep API surface small.