[RFC] Optional interfaces #17288

tontonsb · 2024-12-28T20:11:23Z

This is an implementation attempt of https://wiki.php.net/rfc/optional-interfaces

It's my first attempt at modifying PHP sources, so I am very open to any suggestions and guidance on how this should be actually solved. But for now I'll briefly explain my approach:

Types interface_name_list and interface_name are added to parser. interface_name_list is used instead of class_name_list for implements_list and interface_extends_list. The interface_name is a name that can be prefixed by a ? in which case it will set a ZEND_CLASS_NAME_OPTIONAL flag on the attr of that token.
The struct zend_interface_name is defined and used for interface_names. The difference from zend_class_name is that the zend_interface_name also holds a boolean is_optional which is set according to the ZEND_CLASS_NAME_OPTIONAL flag.
The inheritance mechanism uses the ZEND_FETCH_CLASS_SILENT flag on zend_fetch_class_by_name if the interface name is_optional. In case an optional interface fails to fetch, it's just skipped.

I'm not really sure what should be done for Opcache here and I haven't yet understood how to work with booleans there. I haven't looked into Reflection at all yet.

nielsdos

As you're asking for opcache help, this should resolve some questions and fix the tests (hopefully)

Zend/zend_inheritance.c

ext/opcache/zend_persist.c

ext/opcache/zend_file_cache.c

dstogov · 2025-01-09T18:36:34Z

The PR misses tests. The implementation looks simple. I don't see problems.
Please check if opcache inheritance cache works properly. We may have defined/unefined optional interfaces on one request and vice verse on the other.

bernard-ng · 2025-01-15T02:45:36Z

I had just read the rfc, since PHP8.3 we have the Override attribute if an interface is optional how will this attribute be managed?

tontonsb · 2025-02-09T11:38:07Z

@nielsdos @dstogov I'm trying to understand OPCache and it seems to me that it stores interface_names not interfaces. If that is correct, it would mean that optionality is stored and would accomodate the appearance and disappearance of the optional interface.

I'm trying to write a test for that. The test is passing, but I'm not sure if OPCache is working there at all. If I add a var_dump(opcache_get_status()); inside the script that's called via server, I get

<b>Fatal error</b>:  Uncaught Error: Call to undefined function opcache_get_status() in /var/app/php-src/ext/opcache/tests/optional_interfaces_script.php:3

So it seems that OPCache is not actually available to that script.? However, I was able to run into the same issue by adding var_dump(opcache_get_status()); to other OPCache test scripts running via the same php_cli_server_start e.g. here https://github.com/php/php-src/blob/master/ext/opcache/tests/gh8846.phpt#L14-L19

So maybe I'm just doing something wrong with the OPCache testing?

Other than that I'm now trying to solve an issue with the automagic Stringable.

nielsdos · 2025-02-09T12:03:51Z

The problem is that opcache is a dynamically loaded extension, and it won't get loaded unless you pass the right parameters to the php_cli_server_start call (i.e. -d zend_extension=...). That's why opcache isn't available.

tontonsb · 2025-02-09T16:29:03Z

Thank you very much @nielsdos, now it's working and I've added a check on opcache stats to make sure the cache is used.

But... does this imply that the other opcache tests that use php_cli_server_start are not actually testing OPcache? Sure, I haven't checked the bugs/issues that they are written for, but the code appears to be written with the intention to have OPcache enabled in those tests.

nielsdos · 2025-02-09T21:16:56Z

@tontonsb Huh, looks like a mistake indeed then, because the CLI server uses -n...:

php-src/ext/opcache/tests/php_cli_server.inc

Line 14 in b757fa8

    
           $cmd = [$php_executable, '-t', $doc_root, '-n', ...$ini_array, '-S', PHP_CLI_SERVER_ADDRESS];

I haven't check which ones of these need opcache enabled either, but some probably do..

iluuu1994

I see various failures with ./configure --enable-address-sanitizer & run-test.php --asan. You can also use run-tests.php -m for Valgrind. The tests that fail for me are:

Testing interface declaration using the original and alias class name [Zend/tests/class_alias/class_alias_009.phpt]
Enum cannot manually implement BackedEnum [Zend/tests/enum/no-enum-implements-backed-enum.phpt]
ZE2 A class can only implement interfaces [tests/classes/interface_class.phpt]
Enum cannot manually implement UnitEnum [Zend/tests/enum/no-enum-implements-unit-enum.phpt]
Trying to inherit a class in an interface [Zend/tests/inter_05.phpt]
Automatic Stringable implementation participates in variance [Zend/tests/type_declarations/variance/stringable.phpt]
Trying to implement a trait [Zend/tests/traits/error_008.phpt]
Enum no manual cases method [Zend/tests/enum/no-cases.phpt]
implementing a class [Zend/tests/objects/objects_012.phpt]
Enum no manual from method [Zend/tests/enum/no-from.phpt]
implementing the same interface twice [Zend/tests/objects/objects_013.phpt]
Optional interfaces are rechecked on subsequent requests [ext/opcache/tests/optional_interfaces.phpt]
extending the same interface twice [Zend/tests/objects/objects_014.phpt]
GH-7792 (Refer to enum as enum instead of class) [Zend/tests/gh7792_4.phpt]
Adding stringable is not broken with Optional interfaces [Zend/tests/optional_interfaces/stringable.phpt]

I can also have a closer look if you wish.

Zend/zend_compile.c

Zend/zend_language_parser.y

Zend/zend_inheritance.c

Zend/zend_compile.h

iluuu1994 · 2025-02-11T12:36:51Z

Zend/zend_inheritance.c

+			efree(ce->interface_names);
+		}
+
+		ce->num_interfaces = num_implementable_interfaces;


Given that num_interfaces may diverge from the unliked and linked class, we may need some additional checks when iterating interfaces. E.g. zend_accel_inheritance_cache_find(), which assumes entry->traits_and_interfaces and traits_and_interfaces has the same number of entries.

Zend/zend_inheritance.c

We need to free interfaces names before we overwrite their counter.

Co-authored-by: Ilija Tovilo <[email protected]>

tontonsb · 2025-02-14T23:31:05Z

Thanks for the thorough review an suggestions @iluuu1994

I've solved the small issues, but I'm not sure what to do about those traits_and_interfaces getting out of sync. Maybe you have some hints how to track the state properly?

Besides that I've ran into an issue that the gh9447 test fails — new self and new parent are now getting reflected as new \self and new \parent. Might be a silly typo in rewriting those comparisons, but I can't spot it. Or maybe related to that bit shifting which solved #9447?

When we have a fetch type, the name is "NOT_FQ". In practice, this just omits the leading \.

ext/opcache/tests/optional_interfaces.phpt

iluuu1994

I think this looks reasonable for now. The remaining details can be fixed if and once the RFC passes.

iluuu1994 · 2025-02-24T15:10:02Z

Zend/tests/optional_interfaces/type_checks.phpt

+--TEST--
+Only the existing interfaces pass the type checks
+--INI--
+zend.exception_ignore_args = On


Nit: We don't use spaces in the INI section.

iluuu1994 · 2025-02-24T15:23:00Z

Zend/zend_compile.h

@@ -1034,6 +1034,10 @@ ZEND_API zend_string *zend_type_to_string(zend_type type);
 #define ZEND_NAME_FQ       0
 #define ZEND_NAME_NOT_FQ   1
 #define ZEND_NAME_RELATIVE 2
+#define ZEND_NAME_QUALIFIED_MASK 0x03
+#define NAME_QUAL(type) (type & ZEND_NAME_QUALIFIED_MASK)


I think this should have some sort of Zend-prefix. At least Z_NAME_EQUAL. We don't have many non-prefixed macros.

iluuu1994 · 2025-02-24T15:23:55Z

Zend/zend_inheritance.c

+				ZEND_FETCH_CLASS_ALLOW_NEARLY_LINKED | ZEND_FETCH_CLASS_EXCEPTION |
+				(ce->interface_names[i].is_optional ? ZEND_FETCH_CLASS_SILENT : 0));
+
+			// Optional interfaces are skipped if they don't exist.


Nit: Please prefer C-style /* */ comments.

iluuu1994 · 2025-02-24T21:01:13Z

Zend/zend_inheritance.c

 			if (!iface) {
 				check_unrecoverable_load_failure(ce);
 				free_alloca(traits_and_interfaces, use_heap);
 				return NULL;
 			}
-			traits_and_interfaces[ce->num_traits + i] = iface;
+			traits_and_interfaces[ce->num_traits + num_implementable_interfaces++] = iface;


As mentioned previously, I think we should at least set the remaining slots in traits_and_interfaces to NULL if they are unused. This will prevent use-of-uninitialized-memory issues in zend_accel_inheritance_cache_find() and similar places.

derickr · 2025-03-17T11:29:04Z

Zend/zend_enum.c

@@ -180,14 +180,16 @@ void zend_enum_add_interfaces(zend_class_entry *ce)

 	ZEND_ASSERT(!(ce->ce_flags & ZEND_ACC_RESOLVED_INTERFACES));

-	ce->interface_names = erealloc(ce->interface_names, sizeof(zend_class_name) * ce->num_interfaces);
+	ce->interface_names = erealloc(ce->interface_names, sizeof(zend_interface_name) * ce->num_interfaces);


This is a change that should definitely be in UPGRADING.INTERNALS — I know of several extensions/prototypes that I have written where this would start silently causing issues.

It definitely should be mentioned in the UPGRADING.INTERNALS, but the correct solution would nevertheless be the following:

Suggested change

ce->interface_names = erealloc(ce->interface_names, sizeof(zend_interface_name) * ce->num_interfaces);

ce->interface_names = safe_erealloc(ce->interface_names, ce->num_interfaces, sizeof(*ce->interface_names), 0);

Thus not hardcoding the struct name in the first place.

iluuu1994 · 2025-03-29T18:25:37Z

Closing, as the RFC failed. @tontonsb Good work nonetheless, keep at it!

tontonsb requested a review from dstogov as a code owner December 28, 2024 20:11

github-actions bot added Category: Engine Extension: opcache ABI break labels Dec 28, 2024

tontonsb changed the title ~~Add syntax for optional interfaces~~ [RFC] Optional interfaces Dec 28, 2024

tontonsb marked this pull request as draft December 28, 2024 20:54

nielsdos reviewed Dec 29, 2024

View reviewed changes

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

ext/opcache/zend_persist.c Outdated Show resolved Hide resolved

ext/opcache/zend_file_cache.c Outdated Show resolved Hide resolved

iluuu1994 reviewed Feb 11, 2025

View reviewed changes

tontonsb and others added 16 commits February 14, 2025 23:00

Add syntax for optional interfaces

4358a05

Remove resolved comments

a0fe072

Move the num_interfaces mutation to a safer place

4ed9176

Test optional interfaces

eba9a20

Fix memory leak

1386ca2

We need to free interfaces names before we overwrite their counter.

Add test for Stringable with optional interfaces

686d2e9

Simplify test

b00e2f3

Make test line-trimming-resilient

99f0837

Test Optional interfaces on OPCache

48ab59a

Specify conflict on an OPCache test

8a90459

Generate test script

fd190aa

Enable OPCache in the optional interface test

44f1a63

Fix permissions

77f5ebc

Update Zend/zend_compile.c

9efccd2

Co-authored-by: Ilija Tovilo <[email protected]>

Clean up

a64d1e9

Make const naming more precise

35a5491

tontonsb added 2 commits February 14, 2025 23:00

Move efree back

c3efd97

Make name qualifying checks safe

d216c35

tontonsb force-pushed the optional-interfaces branch from 0ef5fd3 to d216c35 Compare February 14, 2025 21:07

Fix mask

de36608

iluuu1994 added 3 commits February 18, 2025 16:38

[skip ci] Fix spacing

74b524f

Fix ZEND_NAME_ for new class name in const expr

2f8405c

When we have a fetch type, the name is "NOT_FQ". In practice, this just omits the leading \.

Fix leaking of interface_names when interface count goes to 0

62171fd

iluuu1994 reviewed Feb 18, 2025

View reviewed changes

ext/opcache/tests/optional_interfaces.phpt Show resolved Hide resolved

iluuu1994 and others added 3 commits February 18, 2025 18:03

Fix shm modification

3e461b8

Fix optional interface test on windows

821ec0f

Rework optional interface opcache test

b7e513e

iluuu1994 reviewed Feb 24, 2025

View reviewed changes

derickr reviewed Mar 17, 2025

View reviewed changes

iluuu1994 closed this Mar 29, 2025

TimWolla added the RFC label Mar 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Optional interfaces #17288

[RFC] Optional interfaces #17288

tontonsb commented Dec 28, 2024 •

edited

Loading

nielsdos left a comment

dstogov commented Jan 9, 2025

bernard-ng commented Jan 15, 2025

tontonsb commented Feb 9, 2025

nielsdos commented Feb 9, 2025 •

edited

Loading

tontonsb commented Feb 9, 2025

nielsdos commented Feb 9, 2025 •

edited

Loading

iluuu1994 left a comment

iluuu1994 Feb 11, 2025

tontonsb commented Feb 14, 2025

iluuu1994 left a comment

iluuu1994 Feb 24, 2025

iluuu1994 Feb 24, 2025

iluuu1994 Feb 24, 2025

iluuu1994 Feb 24, 2025

derickr Mar 17, 2025

TimWolla Mar 21, 2025

iluuu1994 commented Mar 29, 2025

	ce->interface_names = erealloc(ce->interface_names, sizeof(zend_interface_name) * ce->num_interfaces);
	ce->interface_names = safe_erealloc(ce->interface_names, ce->num_interfaces, sizeof(*ce->interface_names), 0);

[RFC] Optional interfaces #17288

[RFC] Optional interfaces #17288

Conversation

tontonsb commented Dec 28, 2024 • edited Loading

nielsdos left a comment

Choose a reason for hiding this comment

dstogov commented Jan 9, 2025

bernard-ng commented Jan 15, 2025

tontonsb commented Feb 9, 2025

nielsdos commented Feb 9, 2025 • edited Loading

tontonsb commented Feb 9, 2025

nielsdos commented Feb 9, 2025 • edited Loading

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Feb 11, 2025

Choose a reason for hiding this comment

tontonsb commented Feb 14, 2025

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

iluuu1994 Feb 24, 2025

Choose a reason for hiding this comment

derickr Mar 17, 2025

Choose a reason for hiding this comment

TimWolla Mar 21, 2025

Choose a reason for hiding this comment

iluuu1994 commented Mar 29, 2025

tontonsb commented Dec 28, 2024 •

edited

Loading

nielsdos commented Feb 9, 2025 •

edited

Loading

nielsdos commented Feb 9, 2025 •

edited

Loading