[Pre-RFC] Improve language coherence for the behaviour of offsets and containers

2 years ago by Gina P. Banyard — view source — reply

unread

Hello internals,

I would like to get some initial feedback on an RFC I've been working on for the last 5–6 months.
The RFC attempts to explain, and most importantly, improve the semantics around $container[$offset] as PHP is currently widely inconsistent.

The RFC is about six thousands words, so I would recommend a cup of tea/coffee before starting to read it.

The main things which are still in flux is the naming of the new interfaces and methods, the phrasing of error messages, and the behaviour around using null as a container.
Those are generally indicated with a TODO comment.

Of note is that I will also be going on holiday for 3 weeks from the 19th of March, so ideally most of the feedback would come during that time frame so that I can finalize the RFC after my holiday.

RFC: https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

Best regards,

Gina P. Banyard

2 years ago by Larry Garfield — view source — reply

unread

Hello internals,

I would like to get some initial feedback on an RFC I've been working
on for the last 5–6 months.
The RFC attempts to explain, and most importantly, improve the
semantics around $container[$offset] as PHP is currently widely
inconsistent.

The RFC is about six thousands words, so I would recommend a cup of
tea/coffee before starting to read it.

The main things which are still in flux is the naming of the new
interfaces and methods, the phrasing of error messages, and the
behaviour around using null as a container.
Those are generally indicated with a TODO comment.

Of note is that I will also be going on holiday for 3 weeks from the
19th of March, so ideally most of the feedback would come during that
time frame so that I can finalize the RFC after my holiday.

RFC:
https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

Best regards,

Gina P. Banyard

Woof. That's my kind of RFC. :-) The extensive background helps a lot, thank you.

I am generally in favor of this, and have wanted more fine-grained ArrayAccess interfaces for a long time.

Thoughts in no particular order:

"Dimension" is clearly based on the existing engine hooks, but as a user-space dev, it's a very confusing term. Possibly documentable if there's an obvious logic for it we could present, but something more self-evident is probably better.
I am not sure combining get and exists into a single interface is right. I'm not certain it's wrong, either, though. What is the argument for combining them?
Do we have some guidance for what offsetGet() should do when offsetExists() is false? I know there isn't really any now, but it seems like the sort of thing to think about here.
You sort of skirt around but don't directly address the impact of this change on the long-standing desire to make array functions accept arrayified objects. What if any impact would this have there? Eg, could some array functions be made to accept array|DimensionRead without breaking the world?
How, if at all, does this relate to iterable? I think it has no impact, but since it's in the same problem space it's worth confirming.
You mention at one point applying shim code ArrayAccess to make it work like the new interfaces. Please expand on that. Do you mean the engine would automatically insert some shims, like how __toString() now magically implies implements Stringable? Or some trait that objects could use (either a user-space trait or an engine trait) that would provide such shims in an overridable fashion? I don't fully follow here.
Big +1 to removing the magic semi-silent casting when using weird key types.
I feel like some of the sections could benefit from more short code examples. Eg, What the heck does fetch-append on a null even look like? That would help illustrate why the current behavior is weird, or why some things need to stay non-obvious because they're used in odd cases. (Like how $a[1][2] is a by-ref fetch internally, something most people don't think about.)
What would the changes to ArrayObject mean for a backing object that uses hooks on some properties? It says __get/__set would be bypassed. Would hooks also be bypassed? (I have no clue what the preferred logic here is, honestly. Just thinking aloud and hoping hooks pass so that we have to think about this. :-) )
If I read correctly, an internal object that implements one of the dimension handlers will automagically appear to user-land as having the corresponding interface, much like __toString()/Stringable. Is that correct? It seemed implied but not fully stated. If so, a brief code example would help to make it clear in such a long RFC.

Again, thank you for your work on this, and I hope it passes easily.

--Larry Garfield

2 years ago by Gina P. Banyard — view source — reply

unread

Woof. That's my kind of RFC. :-) The extensive background helps a lot, thank you.

I am generally in favor of this, and have wanted more fine-grained ArrayAccess interfaces for a long time.

Thoughts in no particular order:

"Dimension" is clearly based on the existing engine hooks, but as a user-space dev, it's a very confusing term. Possibly documentable if there's an obvious logic for it we could present, but something more self-evident is probably better.

I am open to naming changes, but "dimension" is very much the standard and ubiquitous term for this, see multidimensional arrays, n-dimensional array, etc.

I am not sure combining get and exists into a single interface is right. I'm not certain it's wrong, either, though. What is the argument for combining them?

Do we have some guidance for what offsetGet() should do when offsetExists() is false? I know there isn't really any now, but it seems like the sort of thing to think about here.

I will answer both points together.
The reason that checking an offset exists is combined with being able to read it is that it just makes sense to be together.
If not and only the "read" operation is supported, how do you know that an offset actually exists before accessing it? You just access it and get an Exception that you need to catch?
It is also required for the null coalesce operator to function properly.

What offsetGet() does if the offset doesn't exist is up to the implementation, you could return null as a default value or throw an exception.
The only expectation is that if the offset exists, then reading the value should be possible.

If you can only write to a container, then being able to check something exists is somewhat meaningless.

You sort of skirt around but don't directly address the impact of this change on the long-standing desire to make array functions accept arrayified objects. What if any impact would this have there? Eg, could some array functions be made to accept array|DimensionRead without breaking the world?

How, if at all, does this relate to iterable? I think it has no impact, but since it's in the same problem space it's worth confirming.

The former is actually also more related to iterable, as most array functions need to be able to traverse the container to be able to do anything meaningful with it.
Or there would need to be a way to provide a "manifest" of what offsets are set for things like array_search or array_flip.

You mention at one point applying shim code ArrayAccess to make it work like the new interfaces. Please expand on that. Do you mean the engine would automatically insert some shims, like how __toString() now magically implies implements Stringable? Or some trait that objects could use (either a user-space trait or an engine trait) that would provide such shims in an overridable fashion? I don't fully follow here.

If I read correctly, an internal object that implements one of the dimension handlers will automagically appear to user-land as having the corresponding interface, much like __toString()/Stringable. Is that correct? It seemed implied but not fully stated. If so, a brief code example would help to make it clear in such a long RFC.

Move around a later point to respond to them together.
Internal objects don't actually magically implement Stringable, this is something internal objects must do themselves.
Moreover, internals objects can support string casts without implementing __toString(), see the GMP object which is the only example of this in php-src (and I should fix this or if someone else wants an easy PR to do feel free).

To understand how the shim works, I first need to explain how interfaces being implemented in a class work.

Internal interfaces can have a special handler called interface_gets_implemented which gets called during compilation of classes.
This is the mechanism used by the Throwable and the DateTimeInterface interfaces to prevent userland classes from implementing them.
The ArrayAccess interface has (and all the other Dimension ones actually have) such a handler, and the "shim" is to set the append, fetch, and fetch-append dimension handlers on the CE to magically support those operations on the class for the time being.
No methods are created on the class, the new interfaces are not implemented.
To override the behaviour of append/fetch/fetch-append the relevant new interface should be implemented.

This is conceived as a temporary measure to ease migration for userland classes that support those operations already.

Big +1 to removing the magic semi-silent casting when using weird key types.

I feel like some of the sections could benefit from more short code examples. Eg, What the heck does fetch-append on a null even look like? That would help illustrate why the current behavior is weird, or why some things need to stay non-obvious because they're used in odd cases. (Like how $a[1][2] is a by-ref fetch internally, something most people don't think about.)

I find having too many examples makes RFCs difficult to read and parse, and I prefer to use them sparingly for when they are really needed.
Just for clarity but $a[1][2] is only a by-ref fetch for write operations, if it is a read operation those are performed in sequence.
Fetch-append on null is not really weird, it just appends the given value (/null if no assignments happens) and provides a reference to it.
See https://3v4l.org/UPg3P

What would the changes to ArrayObject mean for a backing object that uses hooks on some properties? It says __get/__set would be bypassed. Would hooks also be bypassed? (I have no clue what the preferred logic here is, honestly. Just thinking aloud and hoping hooks pass so that we have to think about this. :-) )

The current behaviour by-passes __set() and __get() because it works with the property HashTable directly.
My current PR to fix all those weird behaviours is to use the write_property object handler with ZEND_GUARD_PROPERTY_SET guard set.
I don't know how hooks are implemented so Ilija should be able to determine how this works exactly, but my guess is that it should "just work" with set hooks being called.

Again, thank you for your work on this, and I hope it passes easily.

--Larry Garfield

Hope I've answered the questions you had.

Best regards,

Gina P. Banyard

2 years ago by Rob Landers — view source — reply

unread

Woof. That's my kind of RFC. :-) The extensive background helps a lot, thank you.

I am generally in favor of this, and have wanted more fine-grained ArrayAccess interfaces for a long time.

Thoughts in no particular order:

"Dimension" is clearly based on the existing engine hooks, but as a user-space dev, it's a very confusing term. Possibly documentable if there's an obvious logic for it we could present, but something more self-evident is probably better.

I am open to naming changes, but "dimension" is very much the standard and ubiquitous term for this, see multidimensional arrays, n-dimensional array, etc.

I am not sure combining get and exists into a single interface is right. I'm not certain it's wrong, either, though. What is the argument for combining them?

Do we have some guidance for what offsetGet() should do when offsetExists() is false? I know there isn't really any now, but it seems like the sort of thing to think about here.

I will answer both points together.
The reason that checking an offset exists is combined with being able to read it is that it just makes sense to be together.
If not and only the "read" operation is supported, how do you know that an offset actually exists before accessing it? You just access it and get an Exception that you need to catch?
It is also required for the null coalesce operator to function properly.

What offsetGet() does if the offset doesn't exist is up to the implementation, you could return null as a default value or throw an exception.
The only expectation is that if the offset exists, then reading the value should be possible.

If you can only write to a container, then being able to check something exists is somewhat meaningless.

You sort of skirt around but don't directly address the impact of this change on the long-standing desire to make array functions accept arrayified objects. What if any impact would this have there? Eg, could some array functions be made to accept array|DimensionRead without breaking the world?

How, if at all, does this relate to iterable? I think it has no impact, but since it's in the same problem space it's worth confirming.

The former is actually also more related to iterable, as most array functions need to be able to traverse the container to be able to do anything meaningful with it.
Or there would need to be a way to provide a "manifest" of what offsets are set for things like array_search or array_flip.

You mention at one point applying shim code ArrayAccess to make it work like the new interfaces. Please expand on that. Do you mean the engine would automatically insert some shims, like how __toString() now magically implies implements Stringable? Or some trait that objects could use (either a user-space trait or an engine trait) that would provide such shims in an overridable fashion? I don't fully follow here.

If I read correctly, an internal object that implements one of the dimension handlers will automagically appear to user-land as having the corresponding interface, much like __toString()/Stringable. Is that correct? It seemed implied but not fully stated. If so, a brief code example would help to make it clear in such a long RFC.

Move around a later point to respond to them together.
Internal objects don't actually magically implement Stringable, this is something internal objects must do themselves.
Moreover, internals objects can support string casts without implementing __toString(), see the GMP object which is the only example of this in php-src (and I should fix this or if someone else wants an easy PR to do feel free).

To understand how the shim works, I first need to explain how interfaces being implemented in a class work.

Internal interfaces can have a special handler called interface_gets_implemented which gets called during compilation of classes.
This is the mechanism used by the Throwable and the DateTimeInterface interfaces to prevent userland classes from implementing them.
The ArrayAccess interface has (and all the other Dimension ones actually have) such a handler, and the "shim" is to set the append, fetch, and fetch-append dimension handlers on the CE to magically support those operations on the class for the time being.
No methods are created on the class, the new interfaces are not implemented.
To override the behaviour of append/fetch/fetch-append the relevant new interface should be implemented.

This is conceived as a temporary measure to ease migration for userland classes that support those operations already.

Big +1 to removing the magic semi-silent casting when using weird key types.

I feel like some of the sections could benefit from more short code examples. Eg, What the heck does fetch-append on a null even look like? That would help illustrate why the current behavior is weird, or why some things need to stay non-obvious because they're used in odd cases. (Like how $a[1][2] is a by-ref fetch internally, something most people don't think about.)

I find having too many examples makes RFCs difficult to read and parse, and I prefer to use them sparingly for when they are really needed.
Just for clarity but $a[1][2] is only a by-ref fetch for write operations, if it is a read operation those are performed in sequence.
Fetch-append on null is not really weird, it just appends the given value (/null if no assignments happens) and provides a reference to it.
See https://3v4l.org/UPg3P

For what it’s worth, I think having some short one-liner examples sprinkled throughout the RFC is a good thing here. I had to constantly scroll back up to remind myself what each operation actually was in code.

There are a lot of familiar concepts, just with new and particular language; having examples along the way to remind the reader what this new language resolves to would help, not hinder.

— Rob

2 years ago by Rob Landers — view source — reply

unread

Hello internals,

I would like to get some initial feedback on an RFC I've been working on for the last 5–6 months.
The RFC attempts to explain, and most importantly, improve the semantics around $container[$offset] as PHP is currently widely inconsistent.

The RFC is about six thousands words, so I would recommend a cup of tea/coffee before starting to read it.

The main things which are still in flux is the naming of the new interfaces and methods, the phrasing of error messages, and the behaviour around using null as a container.
Those are generally indicated with a TODO comment.

Of note is that I will also be going on holiday for 3 weeks from the 19th of March, so ideally most of the feedback would come during that time frame so that I can finalize the RFC after my holiday.

RFC: https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

Best regards,

Gina P. Banyard

I’m still reading the RFC, but here: https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md#objects

You list “appending” twice.

— Rob

2 years ago by Gina P. Banyard — view source — reply

unread

I’m still reading the RFC, but here: https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md#objects

You list “appending” twice.

Indeed, fixed.

I have also fixed another small mistake in the RFC that used offsetWrite() instead of offsetSet() for the method name of the DimensionWrite interface.

Best regards,
Gina P. Banyard

1 year ago by Rowan Tommins [IMSoP] — view source — reply

unread

I would like to get some initial feedback on an RFC I've been working on for the last 5–6 months.
The RFC attempts to explain, and most importantly, improve the semantics around $container[$offset] as PHP is currently widely inconsistent.

[...]

RFC: https://github.com/Girgias/php-rfcs/blob/master/container-offset-behaviour.md

Hi Gina,

I've just read through this thoroughly, and am simultaneously impressed
with your investigation, and amazed at how many inconsistencies you found.

I think the proposed granular interfaces absolutely make sense, given
the different uses people have for such offsets. My only hesitation is
that if you want "everything", things become quite verbose:

class Foo implements DimensionFetchable, DimensionWritable,
FetchAppendable, DimensionUnsettable { ... }

function
bar(DimensionFetchable&DimensionWritable&FetchAppendable&DimensionUnsettable
$container) { ... }

Unfortunately, I can't think of an easy solution to this without some
form of type aliases.

As an experiment, I tried writing a variation of Python's "defaultdict"
[1] using all the new hooks (without actually testing it against any
implementation). Here's what I came up with:
https://gist.github.com/IMSoP/fbd60c5379ccefcab6c5af25eacc259b

Most of it is straight-forward, but a couple of things stood out:

Separating offsetFetch from offsetGet is really useful, because we can
avoid "auto-vivifying" a key that's only been read, never updated. In
other words, isset($foo['a']) can remain false after running
var_dump($foo['a']), but $foo['a']++ should still work.
The fetchAppend hook is quite confusing to implement, because it's
used in a few subtly different scenarios. For instance, if it's actually
$container[][$offset] = $value there is an implicit requirement that
fetchAppend should return array|DimensionWritable, but presumably that
has to be enforced after fetchAppend has returned. I'm not sure if
there's anything that can be improved here; it probably just needs some
examples in the user manual.

[1]
https://docs.python.org/3/library/collections.html#collections.defaultdict

Over all, I think this is a really great proposal, and hope it proceeds
smoothly.

Regards,

--
Rowan Tommins
[IMSoP]