Resolve relative class type at compile time #11460

Girgias · 2023-06-16T04:14:48Z

The motivation for this is to potentially add methods to ReflectionType which can check if it is co/contra-variant to another ReflectionType, however self, parent, and static are problematic in that those types would usually be resolved at run time.

However, in most cases we can resolve at compile time self and parent, the exceptions being traits and unbound closures.
static is resolvable if it comes from an instance or a class, as it will resolve to that name, leaving interfaces to be an issue.

Commits are kinda a mess as it's somewhat W.I.P. as I might still need to tweak the redundancy check for DNF types.

iluuu1994

That's a shit-ton of tests 😄 I think we should condense them somehow.

ext/reflection/php_reflection.c

iluuu1994 · 2023-06-27T15:43:58Z

ext/reflection/php_reflection.c

+	bool allows_null = ZEND_TYPE_PURE_MASK(param->type) & MAY_BE_NULL;
+	zend_type resolved_type;
+	/* For static resolved name is the name of the class */
+	if (ZEND_TYPE_PURE_MASK(param->type) & MAY_BE_STATIC) {


I'm confused, how can we know static without knowing the context? (i.e. in what object instance it is called)

One cannot create a "proper" ReflectionType without getting it either from a ReflectionParameter, ReflectionProperty, or a ReflectionFunction for the return type.

Part of the change is to propagate forward the object_ce from those prior Reflection objects.

iluuu1994 · 2023-06-27T15:52:37Z

Zend/zend_compile.c

 		if (zend_string_equals_ci(ZEND_TYPE_NAME(type_list->types[i]), ZEND_TYPE_NAME(type))) {
 			zend_string *single_type_str = zend_type_to_string(type);
-			zend_error_noreturn(E_COMPILE_ERROR, "Duplicate type %s is redundant", ZSTR_VAL(single_type_str));
+			if (


Do we need this special handling for this edge-case?

Yes, because otherwise you would get an error message like Duplicate type T is redundant when self or parent gets resolved to T and it makes for some confusing stuff, or I'm misunderstanding the question.

Zend/zend_inheritance.c

iluuu1994 · 2023-06-27T16:11:42Z

Zend/zend_inheritance.c

+				fn->common.arg_info = new_arg_infos + has_return_type;
+				has_resolved_type = true;
+			}
+			new_arg_infos[i].type = resolved_type;


Shouldn't the old type be released? I think this might only be reproducible for eval'd code where the type may contain non-interned strings.

Maybe? Haven't thought about something like this as I would usually expect to be allocated on an arena. And releasing the type info for the traits doesn't make a lot of sense as it still needs to keep the unresolved types for itself.

Girgias · 2023-06-27T17:41:55Z

Yes, it is a lot of tests as there are a lot of weird cases I stumbled into what I thought would be relatively easy :|

Not really sure why we need to condense them as I tried to make them all be their own thing somewhat, and even then I've been missing cases (like using eval, or using a trait in two completely unrelated classes).

arnaud-lb · 2024-02-20T12:11:57Z

Resolving self and parent at compile time may be beneficial because the resolved name will have a CE_CACHE, although I would assume that resolving these at runtime is fast already because they are resolved from EG(This). Do you have benchmark numbers?

If we don't do compile time resolution, we could save some _ZEND_TYPE bits (we are running out of them because the 4 higher ones are used by _ZEND_IS_TENTATIVE_BIT, _ZEND_IS_PROMOTED_BIT, etc). An other benefit would be that it would require less changes overall.

Girgias · 2024-02-20T12:25:14Z

Resolving self and parent at compile time may be beneficial because the resolved name will have a CE_CACHE, although I would assume that resolving these at runtime is fast already because they are resolved from EG(This). Do you have benchmark numbers?

If we don't do compile time resolution, we could save some _ZEND_TYPE bits (we are running out of them because the 4 higher ones are used by _ZEND_IS_TENTATIVE_BIT, _ZEND_IS_PROMOTED_BIT, etc). An other benefit would be that it would require less changes overall.

The main motivation is not saving time at runtime, but to expose co/contra-variant APIs in Reflection to reduce the need in userland to implement the variance logic (at least for most cases as traits and unbound closures are still impossible)

So no, I didn't benchmark.

However, we could change the error messages to show the resolved types instead of going back to self/parent at the downside of it being a minor BC break.

ext/reflection/php_reflection.c

arnaud-lb · 2024-04-16T10:49:56Z

Zend/tests/type_declarations/intersection_types/resolved_relativre_type_valid.phpt

@@ -0,0 +1,19 @@
+--TEST--


There is a typo in the file name :)

...ests/type_declarations/union_types/redundant_types/duplicate_relative_class_parent_type.phpt

ext/reflection/php_reflection.c

arnaud-lb · 2024-04-16T11:40:26Z

ext/reflection/php_reflection.c

+		zend_class_entry *called_scope = NULL;
+		zend_object *this_obj = NULL;
+
+		Z_OBJ_HANDLER(intern->obj, get_closure)(Z_OBJ(intern->obj), &called_scope, &fptr, &this_obj, /* check_only */ true);


I think that you could avoid calling ->get_closure twice here and in reflection_type_factory:

Call ->get_closure in reflection_type_factory and store ce. Set it to called_scope for MAY_BE_STATIC, and fptr->common.scope otherwise

Then here, just use intern->ce in all cases.

An additional benefit is that it may be possible to avoid holding the closure in ->obj. For the condition above, you could add a flag in type_reference to indicate this is in a closure context.

Girgias · 2024-04-16T20:27:47Z

ext/reflection/php_reflection.c

 	intern->ce = ce;
 	if (closure_object) {
-		ZVAL_OBJ_COPY(&intern->obj, Z_OBJ_P(closure_object));
+		intern->ce = zend_ce_closure;


TODO: This looks wrong, and I need to add a test with a Closure bound within a method.

This is only possible if the type is resolvable.

Also throw a compile error if trying to use a trait using the parent type in a class not having a parent

stopping for today

Seems like a pointer not allocated via ZMM gets into the arena????

Girgias requested a review from arnaud-lb June 16, 2023 04:14

github-actions bot added Category: Engine Extension: reflection labels Jun 16, 2023

Girgias force-pushed the reflection-co-variance branch 3 times, most recently from eda873f to a9b9158 Compare June 20, 2023 14:49

Girgias force-pushed the reflection-co-variance branch from a9b9158 to b4a63c5 Compare June 24, 2023 16:09

Girgias marked this pull request as ready for review June 24, 2023 20:02

Girgias requested review from dstogov, iluuu1994 and kocsismate as code owners June 24, 2023 20:02

iluuu1994 reviewed Jun 27, 2023

View reviewed changes

Girgias force-pushed the reflection-co-variance branch from c9d830a to 1910966 Compare June 27, 2023 17:40

Girgias force-pushed the reflection-co-variance branch from 1910966 to b62f42a Compare July 25, 2023 09:49

Girgias force-pushed the reflection-co-variance branch from b62f42a to d5a51e6 Compare February 28, 2024 18:05

Girgias force-pushed the reflection-co-variance branch from 577eda4 to 616f6e7 Compare March 8, 2024 14:12

arnaud-lb reviewed Mar 8, 2024

View reviewed changes

ext/reflection/php_reflection.c Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

Girgias force-pushed the reflection-co-variance branch 2 times, most recently from 61b2568 to eafac08 Compare April 15, 2024 14:06

arnaud-lb reviewed Apr 16, 2024

View reviewed changes

Girgias force-pushed the reflection-co-variance branch from eafac08 to e77225e Compare April 16, 2024 20:08

Girgias commented Apr 16, 2024

View reviewed changes

Girgias mentioned this pull request Apr 17, 2024

ext/reflection: Add test for ReflectionParameter::getDeclaringFunction() with a ReflectionMethod creation from a Closure #13987

Merged

Girgias force-pushed the reflection-co-variance branch from eea1779 to 478464f Compare April 17, 2024 21:53

Girgias added 3 commits April 18, 2024 16:08

Add tests for relative class types

86d8f35

Resolve relative types at compile time when possible

6006c31

Support for resolving self/parent/static in ReflectionType

d18d306

This is only possible if the type is resolvable.

Girgias added 21 commits April 18, 2024 16:08

Resolve trait relative class types when added to a class

f640603

Also throw a compile error if trying to use a trait using the parent type in a class not having a parent

More tests

9894438

Only allocate new arg_infos if types are being resolved

18c6501

Only allocate new zend_type_list if types are being resolved

560b1d6

Address minor review comments

08648dd

Drop return for engine compiler error bail

ebfb5af

Change function name

c9a39aa

Add eval test

274f10b

Add test in multiple distinct unrelated classes

8969b8d

Do not preserve BC for self/parent and always resolve when possible

6c4f630

Add tests and try to resolve bound closure

d67ba9a

Solve bound closure

f40e9bf

Fix resolution for closures by propagating Closure object

eb3c7e8

Fix memory leak when calling constructor manually

41ef059

Remove self/parent as known zend_strings

118ca1c

Address review comments

c0a2da3

Remove object copy which is not need anymore

3f275c4

Add more tests

aa79d51

Add test failing assertion about memory being freed

62095c4

[skip-ci] W.I.P. changes to try and fix test

5bea1b0

stopping for today

[skip-ci] More test and assertion to try to figure out wth is going on

8fc28e9

Seems like a pointer not allocated via ZMM gets into the arena????

Girgias force-pushed the reflection-co-variance branch from 478464f to 8fc28e9 Compare April 18, 2024 15:09

This was referenced Feb 10, 2025

Zend: Resolve self and parent types at compile time #17755

Merged

Override static in return types with self in final classes #17725

Closed

Uh oh!

Resolve relative class type at compile time #11460

Are you sure you want to change the base?

Resolve relative class type at compile time #11460

Uh oh!

Conversation

Girgias commented Jun 16, 2023

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented Jun 27, 2023

Uh oh!

arnaud-lb commented Feb 20, 2024

Uh oh!

Girgias commented Feb 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants