Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add array_is_list(array $array) function (rebased) #6070

Closed
wants to merge 1 commit into from

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Sep 3, 2020

This function tests if an array contains only sequential integer keys (increasing starting from 0). While this isn't an official type, this usage is consistent with the community usage of "list" as an annotation type, cf. https://psalm.dev/docs/annotating_code/type_syntax/array_types/#lists.

This functionality can be polyfilled, but this implementation should be much faster, as it can check for a packed array.

Rebased version of #4886

RFC: https://wiki.php.net/rfc/is_list

An earlier version of this returned false for non-arrays instead of throwing a TypeError - this was updated to prevent confusion with list-like object types


This accepts any type, and deliberately returns false (with no notices) for all non-arrays, e.g. see the test output in the files changed. This is similar to is_callable(), is_numeric(), is_iterable(), etc, which also accept any type:

non-array: throw TypeError
string key: false
mixed keys: false
ordered keys: true

@carusogabriel carusogabriel added this to the PHP 8.1 milestone Sep 3, 2020
}

/* Check if the list could theoretically be repacked */
ZEND_HASH_FOREACH_KEY(array, num_idx, str_idx) {
Copy link
Member

@twose twose Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rebase against the master, and try to use ZEND_HASH_REVERSE_FOREACH_KEY, just like what you said on #4886 (comment), Thanks!

@itsjavi
Copy link

itsjavi commented Oct 14, 2020

I am concerned about the name because lists do not always contain sequential integers as indices https://www.php.net/manual/en/spl.datastructures.php

is_sequential_list would make more sense to me and would be more obvious

@TysonAndre
Copy link
Contributor Author

TysonAndre commented Dec 19, 2020

I am concerned about the name because lists do not always contain sequential integers as indices https://www.php.net/manual/en/spl.datastructures.php

  1. This is based on the definition of list in languages such as java/c - spl has a variety of issues
  2. SplDoublyLinkedList maintains elements in order. It allows you to insert into the middle of the list, but that moves existing elements forwards - the traversal order is always 0..count-1 (Ignoring the fact that it implements Iterator instead of IteratorAggregate meaning that code iterating over the SplDoublyLinkedList or calling next() affects other code iterating over that SplDoublyLinkedList, which can't be changed easily for compatibility reasons)
» php -a
Interactive shell

php > $x = new SplDoublyLinkedList();
php > $x->add(1, 'out of range');

Warning: Uncaught OutOfRangeException: Offset invalid or out of range in php shell code:1
Stack trace:
#0 php shell code(1): SplDoublyLinkedList->add(1, 'out of range')
#1 {main}
  thrown in php shell code on line 1
php > $x->add('key', 'out of range');

Warning: Uncaught OutOfRangeException: Offset invalid or out of range in php shell code:1
Stack trace:
#0 php shell code(1): SplDoublyLinkedList->add('key', 'out of range')
#1 {main}
  thrown in php shell code on line 1
php > $x->add(0, 'in range');
php > $x->add(0, 'insert before old value at key 0');
php > var_export(iterator_to_array($x));
array (
  0 => 'insert before old value at key 0',
  1 => 'in range',
)
php > var_export($x[0]);
'insert before old value at key 0'
php > var_export($x[1]);
'in range'

This function tests if an array contains only sequential integer keys. While
list isn't an official type, this usage is consistent with the community usage
of "list" as an annotation type, cf.
https://psalm.dev/docs/annotating_code/type_syntax/array_types/#lists

Rebased and modified version of php#4886

- Use .stub.php files
- Add opcache constant evaluation when argument is a constant
- Change from is_list(mixed $value) to array_is_list(array $array)

RFC: https://wiki.php.net/rfc/is_list

Co-Authored-By: Tyson Andre <tysonandre775@hotmail.com>
Co-Authored-By: Dusk <dusk@woofle.net>
@TysonAndre TysonAndre changed the title Add is_list function (rebased) Add array_is_list(array $array) function (rebased) Jan 6, 2021
@@ -1188,6 +1188,33 @@ static zend_always_inline void *zend_hash_get_current_data_ptr_ex(HashTable *ht,
ZEND_HASH_FILL_FINISH(); \
} while (0)

/* Check if an array is a list */
static zend_always_inline zend_bool zend_array_is_list(zend_array *array)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd split this into an inlined fast path (num_elements + packed without holes check) and an out of line slow path (that does the full table scan).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I guess with just two usages right now making the whole thing inline isn't terrible either.

@php-pulls php-pulls closed this in 13c430b Jan 20, 2021
@staabm
Copy link
Contributor

staabm commented Sep 24, 2021

This new function is not listed on php.net

https://www.php.net/manual-lookup.php?pattern=array_is_list&scope=quickref

@TysonAndre TysonAndre deleted the is_list branch November 25, 2021 21:25
Totktonada pushed a commit to mikhainin/tarantool-php that referenced this pull request Apr 11, 2022
The `nNextFreeElement` field is initialized with `ZEND_LONG_MIN` instead
of zero since PHP 8.0. Adjust our `php_mp_is_hash()` check accordingly.
See [1] and [2] for details.

NB: PHP 8.1 introduces `zend_array_is_list()`, which may be used here.
See [3] and [4] for details.

[1]: https://wiki.php.net/rfc/negative_array_index
[2]: php/php-src#3772
[3]: https://wiki.php.net/rfc/is_list
[4]: php/php-src#6070

Since I don't observe any other problems on PHP 8.1, closing the
relevant issue.

Fixes tarantool#171
Totktonada pushed a commit to tarantool/tarantool-php that referenced this pull request Apr 11, 2022
The `nNextFreeElement` field is initialized with `ZEND_LONG_MIN` instead
of zero since PHP 8.0. Adjust our `php_mp_is_hash()` check accordingly.
See [1] and [2] for details.

NB: PHP 8.1 introduces `zend_array_is_list()`, which may be used here.
See [3] and [4] for details.

[1]: https://wiki.php.net/rfc/negative_array_index
[2]: php/php-src#3772
[3]: https://wiki.php.net/rfc/is_list
[4]: php/php-src#6070

Since I don't observe any other problems on PHP 8.1, closing the
relevant issue.

Fixes #171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants