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

Pure Intersection types #6799

Merged
merged 40 commits into from Jul 5, 2021
Merged

Pure Intersection types #6799

merged 40 commits into from Jul 5, 2021

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Mar 23, 2021

Add support for pure intersection types A&B but no mixing of union and intersection types (i.e. A&B|C).

RFC Draft is located in a separate personal repository, all discussion and amendment to that should either be a PR to the draft or a comment on the mailing list thread.

The main implementation is done and working, however there are still a couple of things missing:

  • Reflection support
  • More tests and/or combining tests together
  • OPcache support (or can this be left for later?)
  • JIT

@Girgias Girgias added the RFC label Mar 23, 2021
@mvorisek
Copy link
Contributor

Is there any reason to not allow ?A&B type?

PS: https://3v4l.org/mI6JY not allowed for |

@Girgias
Copy link
Member Author

Girgias commented Mar 23, 2021

Is there any reason to not allow ?A&B type?

PS: https://3v4l.org/mI6JY not allowed for |

Because it would be inconsistent with union types, also this makes for a bit of sigil soup and is hard to grasp what the meaning of this is, is it (?A)|B or ?(A&B), with explicit union types the second meaning is clearer to me as an intersection has higher precedence than a union but with ? the meaning I get is more (null|A)&B.

Therefore my reasoning is that is too confusing and will need to wait for composite types.

@derrabus
Copy link
Contributor

Thank you for working on this feature. I understand that you want to keep the first iteration simple and thus leave unions out of the PR.

However, if I'm not mistaken, you would create the only type that is not nullable. That feels like an unnecessary edge-case. Would it be very difficult to at least allow the union of an intersect type with null?

@mvorisek
Copy link
Contributor

mvorisek commented Mar 23, 2021

I think A&B|C and A&B|null should be allowed, there is no ambiguity, & always takes precedence over |.

UPDATE: I meant union of any number of (possibly purely intersected) types.

@Girgias
Copy link
Member Author

Girgias commented Mar 24, 2021

It has nothing to do with ambiguity but I see no point in supporting just A&B|C, it should either be full support for composite types (regardless of support for grouping) or none. As why limit it to A&B|C and not allow A&B|C&D or A&B|C|D|E it is just moving the goal post. And the moment you look into supporting A&B|C you most certainly will get support for the whole range of combination. As the issue is less about parsing nor type checking (as my initial prototype shows although far from done), but the variance rules and checks, it already needs a different nested loop order to traverse both the parent and the child type list just for the pure intersection type case, and at this time I haven't thought deeply about how to handle this nor have really any idea how to achieve this. As the largest part of this proposal has been figuring out how to perform this rather "simplistic" variance check.

Obviously support for full composite type is the end goal, however, I've clearly stated that this is future scope, it could even land in PHP 8.1 as a follow up either by myself or someone else. But I cannot guarantee that I'll get round to that prior to the end of June which is the likely cutoff date for getting an RFC included into PHP 8.1.

If internals decide the feature is incomplete so be it, but I prefer getting this restricted addition into the language upon which one can iterate instead of having perfectness being the enemy of the good.

@jiripudil
Copy link

This is brilliant! Regarding composite types and variance... should mixing intersections and unions be allowed in inheritance at this stage? E.g. the following code which I believe is valid from the LSP perspective:

interface X {}
interface Y {}

class TestOne implements X, Y {}
class TestTwo implements X, Y {}

class A
{
    public function foo(TestOne|TestTwo $param): X&Y {}
}

class B extends A
{
    public function foo(X&Y $param): TestOne|TestTwo {}
}

@azjezz
Copy link

azjezz commented Mar 24, 2021

This is brilliant! Regarding composite types and variance... should mixing intersections and unions be allowed in inheritance at this stage? E.g. the following code which I believe is valid from the LSP perspective:

interface X {}
interface Y {}

class TestOne implements X, Y {}
class TestTwo implements X, Y {}

class A
{
    public function foo(TestOne|TestTwo $param): X&Y {}
}

class B extends A
{
    public function foo(X&Y $param): TestOne|TestTwo {}
}

that's a really good question, personally i think the following should be allowed at this stage.

interface X {}
interface Y {}

class TestOne implements X, Y {}
class TestTwo implements X, Y {}

interface A
{
    public function foo(TestOne|TestTwo $param): X&Y;
}

interface B extends A
{
    public function foo(X&Y $param): TestOne|TestTwo;
}

interface C extends A
{
    public function foo(X $param): TestTwo;
}

interface D extends A
{
    public function foo(Y $param): TestOne;
}

interface E extends B
{
    public function foo(X $param): TestTwo;
}

interface F extends B
{
    public function foo(Y $param): TestOne;
}

@Girgias
Copy link
Member Author

Girgias commented Mar 24, 2021

This is brilliant! Regarding composite types and variance... should mixing intersections and unions be allowed in inheritance at this stage? E.g. the following code which I believe is valid from the LSP perspective:

interface X {}
interface Y {}

class TestOne implements X, Y {}
class TestTwo implements X, Y {}

class A
{
    public function foo(TestOne|TestTwo $param): X&Y {}
}

class B extends A
{
    public function foo(X&Y $param): TestOne|TestTwo {}
}

that's a really good question, personally i think the following should be allowed at this stage.

interface X {}
interface Y {}

class TestOne implements X, Y {}
class TestTwo implements X, Y {}

interface A
{
    public function foo(TestOne|TestTwo $param): X&Y;
}

interface B extends A
{
    public function foo(X&Y $param): TestOne|TestTwo;
}

interface C extends A
{
    public function foo(X $param): TestTwo;
}

interface D extends A
{
    public function foo(Y $param): TestOne;
}

interface E extends B
{
    public function foo(X $param): TestTwo;
}

interface F extends B
{
    public function foo(Y $param): TestOne;
}

You'll be happy to learn that this worked without any modification :-) (which means I probably got most of the variance code correct)

@Girgias Girgias force-pushed the intersection-types branch 2 times, most recently from 29637ec to d2e85d9 Compare April 3, 2021 11:41
@Girgias Girgias mentioned this pull request Apr 8, 2021
@Girgias Girgias force-pushed the intersection-types branch 3 times, most recently from fce561a to faeadd6 Compare May 4, 2021 18:21
Zend/zend_inheritance.c Outdated Show resolved Hide resolved
Zend/zend_inheritance.c Outdated Show resolved Hide resolved
Zend/zend_inheritance.c Outdated Show resolved Hide resolved
Zend/zend_language_parser.y Outdated Show resolved Hide resolved
ext/reflection/php_reflection.c Outdated Show resolved Hide resolved
ext/tokenizer/tokenizer_data.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jun 1, 2021

The variance implementation looks wrong to me, but it took me quite a while to figure out why. Consider:

<?php

class X {}
class Y {}
class Z {}

class A {
    public function test(): X|Z {}
}
class B extends A {
    public function test(): X&Y {}
}

I believe this should pass, because X&Y is a subtype of X is a subtype of X|Z, but currently is rejected, because you're effectively treating the parent type as X&Z.

@azjezz
Copy link

azjezz commented Jun 1, 2021

@nikic

I think the example you gave should error, but for a different reason, that is it's impossible to have X&Y type, since they are both classes, you can't create a new class which extends both, but it should work with interfaces.

@Girgias
Copy link
Member Author

Girgias commented Jun 1, 2021

@nikic

I think the example you gave should error, but for a different reason, that is it's impossible to have X&Y type, since they are both classes, you can't create a new class which extends both, but it should work with interfaces.

Change the classes to interfaces and the problem is still there, and obviously it will error during Runtime because it can't satisfy two classes, but we do not know this at compile time.

@Girgias Girgias force-pushed the intersection-types branch 2 times, most recently from 23497a7 to 203ec25 Compare June 2, 2021 22:43
Copy link
Contributor

@TysonAndre TysonAndre left a comment

Choose a reason for hiding this comment

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

I took a look to check if I understood any edge cases for work on a static analyzer for php

The implementation at a glance doesn't have any issues I can see, but I'm only moderately familiar with it (and I only know a bit about the opcache optimizer and memory management code this uses)

I had some nits on code comments and suggestions/questions for refactoring in a way that is equivalent to the current implementation

Zend/zend_types.h Outdated Show resolved Hide resolved
@@ -148,15 +148,21 @@ typedef struct {
/* TODO: bit 21 is not used */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I wonder if it'd make sense to start using bit 21 for the new constants, not familiar with the code in question. (whether by renumbering _ZEND_TYPE_ARENA_BIT in php 8.1 (hopefully anything actually using this uses this macro) or using 1u<<21 for the new constants

Copy link
Member Author

Choose a reason for hiding this comment

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

I think when I started the implementation this bit was still used, but I'll let the decision to renumber to someone else as I don't know if we should do.

ext/opcache/jit/zend_jit_helpers.c Outdated Show resolved Hide resolved
ext/opcache/jit/zend_jit_helpers.c Outdated Show resolved Hide resolved
ext/opcache/jit/zend_jit_helpers.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
ext/reflection/tests/intersection_types.phpt Outdated Show resolved Hide resolved
Zend/zend_inheritance.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
nikic and others added 12 commits July 5, 2021 04:14
Make zend_perform_intersection_covariant_class_type_check() a
single function that checks whether a simple type is a subtype of
a complex type.
I think this is still not quite correct :/
It wasn't immediatly obvious to me while starring at the code that the case where a parent type is not loadable is handled.

Just needed to understand that get_class_from_type() only returns NULL when it is not a class-type
Refactor a bit the handling of iterable as a by product.
@Girgias
Copy link
Member Author

Girgias commented Jul 5, 2021

The variance code seems correct to me, the only thing which felt weird was the need to load classes when object is in the parent type, so I moved that branch out.

@nikic
Copy link
Member

nikic commented Jul 5, 2021

@Girgias Loading classes for object type is necessary for forward-compatibility with type aliases. We do the same thing for other object variance checks.

@Girgias
Copy link
Member Author

Girgias commented Jul 5, 2021

Okay, I'll revert and add a comment to make sure that doesn't get reworked.

/* Currently, for object type any class name would be allowed here.
* We still perform a class lookup for forward-compatibility reasons,
* as we may have named types in the future that are not classes
* (such as enums or typedefs). */
Copy link
Member

Choose a reason for hiding this comment

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

The "enums" bit here doesn't make sense anymore :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy pasted this, so I also fixed that comment at the same time.

@Girgias Girgias merged commit 069a9fa into php:master Jul 5, 2021
@Girgias Girgias deleted the intersection-types branch July 5, 2021 12:41
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Jan 3, 2023
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Jan 3, 2023
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet