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 support for Disjoint Normal Form (DNF) types #8725

Merged
merged 13 commits into from
Jul 8, 2022

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 7, 2022

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

This allows to combine union and intersection types together in the following form (A&B)|(X&Y)|T but not of the form (X|A)&(Y|B) or (X|A)&(Y|B)|T.

@Girgias Girgias added this to the PHP 8.2 milestone Jun 7, 2022
@Girgias Girgias requested review from nikic, arnaud-lb and iluuu1994 June 7, 2022 13:01
@Girgias Girgias force-pushed the disjoint-union-types branch 2 times, most recently from e720740 to 90206ab Compare June 9, 2022 13:54
Comment on lines 6264 to 6276
zend_error_noreturn(E_COMPILE_ERROR, "Type %s is redundant with type %s",
ZSTR_VAL(r_type_str), ZSTR_VAL(l_type_str));
} else {
zend_error_noreturn(E_COMPILE_ERROR, "Type %s is redundant with type %s",
ZSTR_VAL(l_type_str), ZSTR_VAL(r_type_str));
}
} else {
if (flipped) {
zend_error_noreturn(E_COMPILE_ERROR, "Type %s is less restrictive than type %s",
ZSTR_VAL(r_type_str), ZSTR_VAL(l_type_str));
} else {
zend_error_noreturn(E_COMPILE_ERROR, "Type %s is less restrictive than type %s",
ZSTR_VAL(l_type_str), ZSTR_VAL(r_type_str));
Copy link
Member

Choose a reason for hiding this comment

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

In the second form it may be unclear why the type being less restrictive is a problem. At first I though it was going to be a variance issue when reading https://github.com/php/php-src/pull/8725/files#diff-d73c373701c7d0e7cb2a5fbc3b3a0843746d028029531ad73dd3024676a0a6d0

Something like "Type A&B is redundant because it is less restrictive than A" would make this clearer to me. This also puts the emphasis on A&B, and gives a hint at how the problem could be resolved (by removing the redundant type).

Maybe displaying the full union type would help as well: "Member A&B in union A&B|A is redundant because it is less restrictive than A". (Other union/intersection messages do not appear to display the full union type though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is we might not know the full union type indeed imagine a union type of the form:
A|(A&B)|C the compile warning would be emitted when parsing (A&B) but at this point we have no knowledge of the |C part and would not be able to display the complete type anyway.

But I know that I read the error message again I do agree it is kinda confusing so I will improve it

@Girgias Girgias force-pushed the disjoint-union-types branch from 90206ab to 7583fb4 Compare June 10, 2022 12:20
@Girgias
Copy link
Member Author

Girgias commented Jun 10, 2022

Note: To fail the XFAIL test use the: make test TESTS="Zend/tests/type_declarations/ -j8 -d opcache.enable_cli=1" command.
I've narrowed down the problem to the inheritance cache, but I can't figure out where the issue is.

The Travis CI Failure can be reproduced with: make test USE_ZEND_ALLOC=0 TESTS="ext/reflection/tests/types/ --repeat 2 -j8 -d opcache.enable_cli=1 --asan"

@arnaud-lb
Copy link
Member

On the XFAIL: OPcache expects class names in the dependency list to be shm-allocated / persisted. The name that fails the assertion was added to the list by track_class_dependency(), called by zend_is_intersection_subtype_of_class(), and comes from a method arg infos. Normally the class names in arg infos are persisted by OPcache before this step, but here it didn't happen. I think that we need to update zend_persist_type(): it doesn't appear to recurse in nested lists (zend_persist_type_calc() would need to be updated as well).

@arnaud-lb
Copy link
Member

arnaud-lb commented Jun 11, 2022

There are a few other places that may or may not need to be updated: zend_mark_function_as_generator, zend_type_permits_self, zend_declare_typed_property, and so on.

There could be a use-case for a ZEND_TYPE_FOREACH_CLASS_NAME() macro.

@Girgias
Copy link
Member Author

Girgias commented Jun 11, 2022

On the XFAIL: OPcache expects class names in the dependency list to be shm-allocated / persisted. The name that fails the assertion was added to the list by track_class_dependency(), called by zend_is_intersection_subtype_of_class(), and comes from a method arg infos. Normally the class names in arg infos are persisted by OPcache before this step, but here it didn't happen. I think that we need to update zend_persist_type(): it doesn't appear to recurse in nested lists (zend_persist_type_calc() would need to be updated as well).

So that was the issue, I was looking at the wrong place then.
I think I fixed the issues for that now then.

There are a few other places that may or may not need to be updated: zend_mark_function_as_generator, zend_type_permits_self, zend_declare_typed_property, and so on.

There could be a use-case for a ZEND_TYPE_FOREACH_CLASS_NAME() macro.

I'm not sure how a ZEND_TYPE_FOREACH_CLASS_NAME() macro would work as I think it's a general problem with ZEND_TYPE_FOREACH() which needs to cater to the nested types.

I will try to come up with test cases for every other instance as those probably do have some issue lingering in disguise

@sgolemon
Copy link
Member

It's not letting me comment direcly on the patch, but I had some minor thoughts about making the parser a tiny bit more readable (less verbose):

union_type_element:
                type { $$ = $1; }
        |        '(' intersection_type ')' { $$ = $2; }
;

union_type:
		union_type_element '|' union_type_element
			{ $$ = zend_ast_create_list(2, ZEND_AST_TYPE_UNION, $1, $3); }
	|	union_type '|' union_type_element
			{ $$ = zend_ast_list_add($1, $3); }
;

And of course:

union_type_without_static_element:
                type_without_static { $$ = $1; }
        |        '(' intersection_type_without_static ')' { $$ = $2; }
;

union_type_without_static:
		union_type_without_static_element '|' union_type_without_static_element
			{ $$ = zend_ast_create_list(2, ZEND_AST_TYPE_UNION, $1, $3); }
	|	union_type_without_static '|' union_type_without_static_element
			{ $$ = zend_ast_list_add($1, $3); }
;

The lower verbosity may also help us out later when/if we add more complexity to algebraic types.

@Girgias Girgias merged commit f905590 into php:master Jul 8, 2022
@Girgias Girgias deleted the disjoint-union-types branch July 8, 2022 10:30
@TimWolla
Copy link
Member

Note: This is not yet mentioned in UPGRADING / NEWS.

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jul 16, 2022
TimWolla added a commit to TimWolla/php-src that referenced this pull request Jul 16, 2022
Girgias pushed a commit that referenced this pull request Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants