Convert iterable into an internal alias for Traversable|array #7309

Girgias · 2021-07-26T22:24:37Z

There is probably some more clean-up which can be done in zend_execute.c/JIT/OpCache from the handling of iterable, but I am wondering if this the correct way of approaching this.

nikic · 2021-07-27T07:25:01Z

Looks basically okay -- one thing I'm unsure about is whether we need to try to convert this back to iterable for reflection purposes to avoid breaking code that uses iterable but does not handle union types.

nikic · 2021-08-02T10:08:28Z

A problem here is that existing arginfo for internal functions may have MAY_BE_ITERABLE, in which case it's going to misbehave now. We'd have to either adjust that when registering internal functions, or require arginfo to be updated to not use iterable (in which case we should drop/rename MAY_BE_ITERABLE to cause a build failure).

kocsismate · 2021-09-14T18:37:54Z

@Girgias For gen_stub.php, you'll need something along the lines of:

Index: build/gen_stub.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build/gen_stub.php b/build/gen_stub.php
--- a/build/gen_stub.php	(revision 524a116cf7399437f69eb1c227d94953cea8b74a)
+++ b/build/gen_stub.php	(date 1631644450337)
@@ -203,7 +203,6 @@
             case "float":
             case "string":
             case "callable":
-            case "iterable":
             case "object":
             case "resource":
             case "mixed":
@@ -278,8 +277,6 @@
                 return "IS_VOID";
             case "callable":
                 return "IS_CALLABLE";
-            case "iterable":
-                return "IS_ITERABLE";
             case "mixed":
                 return "IS_MIXED";
             case "static":
@@ -313,8 +310,6 @@
                 return "MAY_BE_OBJECT";
             case "callable":
                 return "MAY_BE_CALLABLE";
-            case "iterable":
-                return "MAY_BE_ITERABLE";
             case "mixed":
                 return "MAY_BE_ANY";
             case "void":
@@ -427,6 +422,15 @@
             );
         }
 
+        if ($node instanceof Node\Identifier && $node->toLowerString() === "iterable") {
+            return new Type(
+                [
+                    SimpleType::fromString("Traversable"),
+                    ArrayType::createGenericArray(),
+                ]
+            );
+        }
+
         return new Type([SimpleType::fromNode($node)]);
     }

This patch only deals with iterable when it's a simple type, but the union type case (including the nullable one) should also be accounted for. Unfortunately, I couldn't do it elegantly easily, so I'll leave it as an exercise for you. 😅

Girgias · 2021-09-14T19:20:31Z

@Girgias For gen_stub.php, you'll need something along the lines of:

Index: build/gen_stub.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build/gen_stub.php b/build/gen_stub.php
--- a/build/gen_stub.php	(revision 524a116cf7399437f69eb1c227d94953cea8b74a)
+++ b/build/gen_stub.php	(date 1631644450337)
@@ -203,7 +203,6 @@
             case "float":
             case "string":
             case "callable":
-            case "iterable":
             case "object":
             case "resource":
             case "mixed":
@@ -278,8 +277,6 @@
                 return "IS_VOID";
             case "callable":
                 return "IS_CALLABLE";
-            case "iterable":
-                return "IS_ITERABLE";
             case "mixed":
                 return "IS_MIXED";
             case "static":
@@ -313,8 +310,6 @@
                 return "MAY_BE_OBJECT";
             case "callable":
                 return "MAY_BE_CALLABLE";
-            case "iterable":
-                return "MAY_BE_ITERABLE";
             case "mixed":
                 return "MAY_BE_ANY";
             case "void":
@@ -427,6 +422,15 @@
             );
         }
 
+        if ($node instanceof Node\Identifier && $node->toLowerString() === "iterable") {
+            return new Type(
+                [
+                    SimpleType::fromString("Traversable"),
+                    ArrayType::createGenericArray(),
+                ]
+            );
+        }
+
         return new Type([SimpleType::fromNode($node)]);
     }

This patch only deals with iterable when it's a simple type, but the union type case (including the nullable one) should also be accounted for. Unfortunately, I couldn't do it elegantly easily, so I'll leave it as an exercise for you. sweat_smile

Well, I think I've got something which works, but not sure, might want to build a test suite for it.

Girgias · 2021-09-15T15:37:55Z

@nikic I'm wondering if it still makes sense to make Reflection behave properly with iterable as it seems unlikely that PHP 8.2 code won't need to handle intersection/union types.

The fallback for internal functions using it still makes sense though, at least in the short term, but in any cases both things should be documented in their respective UPGRADING docs.

kocsismate · 2022-04-21T07:32:25Z

Looks good to me as well

Zend/zend_types.h

iluuu1994 · 2022-04-21T10:59:17Z

Zend/zend_compile.c

@@ -1189,22 +1189,40 @@ static zend_string *resolve_class_name(zend_string *name, zend_class_entry *scop

 zend_string *zend_type_to_string_resolved(zend_type type, zend_class_entry *scope) {
 	zend_string *str = NULL;
+	uint32_t type_mask = ZEND_TYPE_PURE_MASK(type);


Something here does not seem to work when iterable appears first in the union.

function test(): iterable|Foo {} test();

Fatal error: Uncaught TypeError: test(): Return value must be of type Traversable|Foo|array, none returned

The type representation looks like this:

zend_type mask: 5505152 (ZEND_TYPE_LIST_BIT | _ZEND_TYPE_UNION_BIT | MAY_BE_ARRAY) ptr: zend_type_list (2): zend_type: mask: 16777216 (_ZEND_TYPE_NAME_BIT) ptr: Traversable zend_type: mask: 16777216 (_ZEND_TYPE_NAME_BIT) ptr: Foo

Whereas Foo|iterable looks like this:

zend_type mask: 5505152 (ZEND_TYPE_LIST_BIT | _ZEND_TYPE_UNION_BIT | MAY_BE_ARRAY) ptr: zend_type_list (2): zend_type: mask: 16777216 (_ZEND_TYPE_NAME_BIT) ptr: Foo zend_type: mask: 19136512 (_ZEND_TYPE_NAME_BIT | _ZEND_TYPE_UNION_BIT | _ZEND_TYPE_INTERSECTION_BIT) ptr: Traversable

So it looks like the _ZEND_TYPE_ITERABLE_BIT flag is lost. The second one looks fishy too, as the Traversable type has the _ZEND_TYPE_UNION_BIT set when it's not really a union. It's unclear (to me) whether the _ZEND_TYPE_ITERABLE_BIT should be set on the root type.

I wonder if trying to maintain BC is worthwhile at all. For type aliases, I was planning to replace the types at runtime which would also result in the resolved types when printing them. If iterable could be made a type alias at some point the behavior would change again.

I really don't know if it is worthwile as I didn't bother with it initially, maybe it is better to just have compatibility for Reflection when it is only iterable (or ?iterable) ?

@Girgias IMO it should work for all cases or we should drop it altogether.

I'll go back then to how it was initially, as I would think code running for 8.2 would be adapted to handle union types, but that will need to be put in the migration guide anyway.

@Girgias Maybe keep the code around, just in case we'll need the BC solution anyway. I think with a BC break this will be a bit more questionable and should at least be mentioned on the mailing list.

I'll email internals then

Zend/zend_compile.c

ext/reflection/php_reflection.c

Zend/zend_types.h

Girgias · 2022-05-18T11:07:39Z

I would like to merge within a week.

This segfaults and I don't know why

Clarify comment Remove unnecessary variable Use ZEND_TYPE_INIT_CLASS_CONST_MASK in zend_compile.c

Follow-up from #7309 as I didn't change the ZPP TypeError wording.

Girgias force-pushed the alias-iterable branch from 7f3a6dd to 4205cb9 Compare July 26, 2021 23:01

krakjoe added Feature Waiting on Author labels Jul 28, 2021

Girgias force-pushed the alias-iterable branch from 4205cb9 to d19e2f0 Compare July 28, 2021 17:53

Girgias removed the Waiting on Author label Jul 28, 2021

Girgias force-pushed the alias-iterable branch 2 times, most recently from 605a695 to a70db6b Compare August 12, 2021 01:39

Girgias mentioned this pull request Aug 12, 2021

Disjunctive normal form types Girgias/php-src#10

Closed

Girgias force-pushed the alias-iterable branch from a70db6b to e2ad89c Compare September 14, 2021 15:18

Girgias force-pushed the alias-iterable branch from e17d3a7 to d9db3e0 Compare December 3, 2021 13:28

Girgias force-pushed the alias-iterable branch from d9db3e0 to 219908d Compare December 20, 2021 18:45

Girgias force-pushed the alias-iterable branch from 219908d to 528b05c Compare March 19, 2022 16:30

Girgias force-pushed the alias-iterable branch from 528b05c to 8a69ac2 Compare April 20, 2022 09:47

Girgias requested review from kocsismate and iluuu1994 April 20, 2022 11:32

iluuu1994 reviewed Apr 21, 2022

View reviewed changes

Girgias force-pushed the alias-iterable branch 2 times, most recently from ca4a14d to 1ac37d9 Compare April 23, 2022 12:49

nikic reviewed Apr 24, 2022

View reviewed changes

Zend/zend_types.h Show resolved Hide resolved

Girgias force-pushed the alias-iterable branch 3 times, most recently from d217311 to b7a616f Compare May 3, 2022 21:04

Girgias requested a review from nikic May 4, 2022 09:45

Girgias added 23 commits June 7, 2022 12:37

Point to iterable for incompatible type in intersection

fd55485

Fix iterable type for internal functions

e427ef8

Start of a working stub generation

3635821

Possibly generic handling of types?

3a9326a

Ammend test3

280cbf0

Use iterable instead of array|Traversable when usage of single type

1ff4c99

Handle iterable in complex union types

fd14604

Fix opcache test

fcecc29

Address review and drop BC shim for unions

8b548a2

Remove shim for iterable BC

569ca2c

Make invalid types in intersection more robust

39b2258

Fix opcache test

307b663

Check for legacy iterable arg_info during function registration

37b7141

This segfaults and I don't know why

Warn also if return type is iterable

f29b2c7

Do not test statup in effect

8b479f0

Do not warn but enable legacy cnbversion test

cc0291c

Reflection iterable BC for named types

bbdb793

Nit + whitespaces

be44175

Address review comments

e0bf71b

Clarify comment Remove unnecessary variable Use ZEND_TYPE_INIT_CLASS_CONST_MASK in zend_compile.c

Whitespace revert + code move revert

78129bd

Do not memcpy when not needed

ae53b04

Do not use UNION bit for iterable as it is not a type list

d4056a9

last whitespaces reverts

9adcadc

Girgias force-pushed the alias-iterable branch from a69ad66 to 9adcadc Compare June 7, 2022 11:39

Girgias merged commit b40ae80 into php:master Jun 7, 2022

This was referenced Jun 17, 2022

RFC: Make the iterator_*() family accept all iterables #8819

Merged

Use same type error wording for alias iterable in ZPP #8838

Merged

Girgias added a commit that referenced this pull request Jun 21, 2022

Use same type error wording for alias iterable in ZPP (#8838)

280b3db

Follow-up from #7309 as I didn't change the ZPP TypeError wording.

Girgias deleted the alias-iterable branch June 29, 2022 13:52

iluuu1994 mentioned this pull request Jul 26, 2023

PHP 8.2: Issue with type declaration for iterable|object #11797

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert iterable into an internal alias for Traversable|array #7309

Convert iterable into an internal alias for Traversable|array #7309

Girgias commented Jul 26, 2021 •

edited

Loading

nikic commented Jul 27, 2021

nikic commented Aug 2, 2021

kocsismate commented Sep 14, 2021

Girgias commented Sep 14, 2021

Girgias commented Sep 15, 2021

kocsismate commented Apr 21, 2022

iluuu1994 Apr 21, 2022

Girgias Apr 23, 2022

iluuu1994 Apr 23, 2022

Girgias Apr 23, 2022

iluuu1994 Apr 23, 2022

Girgias Apr 23, 2022

Girgias commented May 18, 2022

Convert iterable into an internal alias for Traversable|array #7309

Convert iterable into an internal alias for Traversable|array #7309

Conversation

Girgias commented Jul 26, 2021 • edited Loading

nikic commented Jul 27, 2021

nikic commented Aug 2, 2021

kocsismate commented Sep 14, 2021

Girgias commented Sep 14, 2021

Girgias commented Sep 15, 2021

kocsismate commented Apr 21, 2022

iluuu1994 Apr 21, 2022

Choose a reason for hiding this comment

Girgias Apr 23, 2022

Choose a reason for hiding this comment

iluuu1994 Apr 23, 2022

Choose a reason for hiding this comment

Girgias Apr 23, 2022

Choose a reason for hiding this comment

iluuu1994 Apr 23, 2022

Choose a reason for hiding this comment

Girgias Apr 23, 2022

Choose a reason for hiding this comment

Girgias commented May 18, 2022

Girgias commented Jul 26, 2021 •

edited

Loading