Add support for Disjoint Normal Form (DNF) types#8725
Conversation
e720740 to
90206ab
Compare
Zend/zend_compile.c
Outdated
| 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)); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
90206ab to
7583fb4
Compare
|
Note: To fail the XFAIL test use the: The Travis CI Failure can be reproduced with: |
|
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 |
|
There are a few other places that may or may not need to be updated: There could be a use-case for a |
7583fb4 to
ec35a11
Compare
So that was the issue, I was looking at the wrong place then.
I'm not sure how a I will try to come up with test cases for every other instance as those probably do have some issue lingering in disguise |
|
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): And of course: The lower verbosity may also help us out later when/if we add more complexity to algebraic types. |
Support for variance and Reflection
Co-authored-by: Sara Golemon <pollita@php.net>
I don't have a test as I don't know how to write one
No test as stub support to add a test case needs support in PHP-Parser
ec35a11 to
521f3f1
Compare
|
Note: This is not yet mentioned in UPGRADING / NEWS. |
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)|Tbut not of the form(X|A)&(Y|B)or(X|A)&(Y|B)|T.