Initial idea for Intersection/Union types #569

kschatzle · 2022-09-28T01:23:09Z

Not close to finishing, but what do you think about the direction before I get everything ironed out? (Not all unit tests pass yet).

#535

Adding void return type fixture and test.

Removing naming normalization of types from node to ClassMirror.php.

Temp commit.

stof · 2022-09-28T08:44:58Z

src/Prophecy/Doubler/Generator/Node/Type/UnionTypeNode.php

+    /**
+     * @param NamedTypeNode ...$types
+     */
+    public function __construct(bool $allowsNull = false, NamedTypeNode ...$types)


I would make it an array rather than a variadic argument. Otherwise, we have no way to add more parameters in the signature in the future.

stof · 2022-09-28T08:45:40Z

src/Prophecy/Doubler/Generator/Node/Type/UnionTypeNode.php

+    {
+        $string = '';
+        foreach ($this->types as $type) {
+            $string .= $type . '|';


The code would be much simpler by using return implode('|', $this->types)

stof · 2022-09-28T08:47:26Z

src/Prophecy/Doubler/Generator/Node/Type/NamedTypeNode.php

+/**
+ * @see \ReflectionNamedType
+ */
+class NamedTypeNode extends TypeNodeAbstract implements Type


extending TypeNodeAbstract without calling the parent constructor is a no-go to me, as it does not initialize the parent properties properly. To me, there is no reason to extend TypeNodeAbstract here

Refactored some of the Type's. Now the TypeNodeAbstract does not have a __constrcut.

stof · 2022-09-28T08:48:58Z

src/Prophecy/Doubler/Generator/Node/Type/UnionTypeNode.php

+class UnionTypeNode extends TypeNodeAbstract implements Type
+{
+    /**
+     * @var NamedTypeNode[]


This should be Type[] as PHP 8.2 supports putting intersections inside unions

This was targeted for PHP81 intersection at the moment. The extra pull request for DNF is pretty easy and would include this.

Well, I would work on both at the same time actually (otherwise, your PR needs to include a nice handling of DNF to fail with a good error message, which is not worth it IMO). Thus, as those classes are not marked as internal, you would still need to do the work in the same release to avoid having to introduce BC layers.

stof · 2022-09-28T08:49:41Z

src/Prophecy/Doubler/Generator/ClassCodeGenerator.php

            return '';
        }

        // When we require PHP 8 we can stop generating ?foo nullables and remove this first block
        if ($typeNode->canUseNullShorthand()) {
-            return sprintf( '?%s', $typeNode->getNonNullTypes()[0]);
+            return sprintf( '?%s', $typeNode->getType()->getName());


this is broken if $typeNode->getType() is not a NamedTypeNode`

That's baked into $typeNode->canUseNullShorthand() since only NamedType can be shorthand.

stof · 2022-09-28T08:52:19Z

src/Prophecy/Doubler/Generator/Node/Type/IntersectionTypeNode.php

+        foreach ($this->types as $type) {
+            $string .= $type . '&';
+        }
+        return '(' . rtrim($string, '&') . ')';


those braces will be broken in PHP 8.1, and will be useless in PHP 8.2+ when the intersection is not part of a union

stof · 2022-09-28T08:56:52Z

src/Prophecy/Doubler/Generator/Node/ReturnTypeNode.php

-        if (isset($this->types['never']) && count($this->types) !== 1) {
-            throw new DoubleException('never cannot be part of a union');
+        if ($this->type instanceof UnionTypeNode) {
+            /** @var NamedTypeNode $type */


Assuming this here will not support DNF types. Instead, you should have a check if (!$type instanceof NamedTypeNode) continue;

stof · 2022-09-28T08:57:49Z

src/Prophecy/Doubler/Generator/Node/Type/IntersectionTypeNode.php

+/**
+ * @see \ReflectionIntersectionType
+ */
+class IntersectionTypeNode extends TypeNodeAbstract implements Type


I would remove Node from the names of those classes representing types.

stof · 2022-09-28T08:59:31Z

src/Prophecy/Doubler/Generator/Node/Type/TypeNodeAbstract.php

+        return $this->allowsNull;
+    }
+
+    public function getTypes(): array


I would not add this method here. It makes no sense for NamedTypeNode to define it. And having it only in specific classes would force to check the type before using it, which is good because subtypes of a union or of an intersection should almost never be used the same.

It's been removed during a refactor.

stof · 2022-09-28T09:00:13Z

src/Prophecy/Doubler/Generator/Node/Type.php

+
+namespace Prophecy\Doubler\Generator\Node;
+
+interface Type


this interface needs to enforce types being stringable, as the code relies on it.

kschatzle · 2022-09-29T14:18:19Z

Awesome, thanks for the feedback. Seems like you're ok with the BC breaks in signatures.

stof · 2022-09-29T15:00:05Z

src/Prophecy/Doubler/Generator/Node/Type/NamedTypeNode.php

+
+    public function __toString(): string
+    {
+        return $this->name;


NamedTypeNode should not require passing the leading \ IMO. Otherwise, it makes it impossible to use it with Foo::class, which would be a confusing API when building class patches.

stof · 2022-09-29T15:01:59Z

@kschatzle it would be better if ArgumentTypeNode and ReturnTypeNode could have a BC layer for their getTypes method (this BC layer can throw an exception when an intersection type is found, hinting at using the new API to support it). And even better if we can provide a BC layer for the existing constructor signature (with deprecation warnings).

smyhill · 2023-07-31T23:11:18Z

@kschatzle Is this something still actively being worked on. Can I help out in any way?

kschatzle · 2023-08-15T13:32:55Z

I need to get back to this eventually when I have time. Some IRL stuff.

dannyvw · 2024-03-27T13:55:01Z

Doctrine has changed the return type from ObjectRepository to EntityRepository https://github.com/doctrine/DoctrineBundle/blob/2.12.x/src/Repository/RepositoryFactoryCompatibility.php (in ORM 3.x). This triggers now the following error in phpspec "doubling intersection types is not supported"

stof · 2024-03-27T14:26:34Z

@dannyvw how is this related ? There is no union type nor intersection type in this trait

dannyvw · 2024-03-27T15:04:37Z

@stof The issue is related to the changes inside EntityRepository (https://github.com/doctrine/orm/blob/3.1.x/src/EntityRepository.php) and the return type change. If you have a test case like below then it triggers this error "doubling intersection types is not supported".

    public function let(EntityManagerInterface $entityManager, EntityRepository $repository): void
    {
        $entityManager->getRepository(SomeEntity::class)->willReturn($repository);
    }

stof · 2024-03-27T15:18:38Z

I doubt the code you linked is relevant then, as there is no intersection type anywhere in this file.

Anyway, this PR is not ready to be merged. If you want this to move forward without waiting for the author to get back to it, an option is to take over the work to finish it.

dannyvw · 2024-03-27T15:30:05Z

Not sure either but the EntityRepository was the class inside the error. This error "doubling intersection types is not supported" is only triggered when upgrading doctrine/orm from 2.19 to 3.x.

jdreesen · 2024-03-27T15:36:18Z

There's this: https://github.com/doctrine/orm/blob/3.1.x/src/EntityRepository.php#L199

massimilianobraglia · 2024-10-01T09:32:15Z

Hey all, is this pull request still in progress? What's missing?

stof · 2024-10-29T14:59:23Z

none of the review comments have been taken into account for now.

kschatzle added 3 commits September 26, 2022 08:46

Intersection types in generated code

71cf341

Adding void return type fixture and test.

Intersection types in generated code

8d3935b

Removing naming normalization of types from node to ClassMirror.php.

Intersection types in generated code

05faade

Temp commit.

stof reviewed Sep 28, 2022

View reviewed changes

stof reviewed Sep 29, 2022

View reviewed changes

ciaranmcnulty changed the title ~~Initial idea for Issues/535~~ Initial idea for Intersection/Union types Nov 29, 2022

stof mentioned this pull request Feb 29, 2024

Intersection types in generated code #535

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial idea for Intersection/Union types #569

Initial idea for Intersection/Union types #569

kschatzle commented Sep 28, 2022 •

edited by stof

Loading

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

kschatzle Sep 28, 2022

stof Sep 28, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

stof Sep 28, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

stof Sep 28, 2022

kschatzle Oct 3, 2022

kschatzle commented Sep 29, 2022

stof Sep 29, 2022

stof commented Sep 29, 2022

smyhill commented Jul 31, 2023

kschatzle commented Aug 15, 2023

dannyvw commented Mar 27, 2024 •

edited

Loading

stof commented Mar 27, 2024

dannyvw commented Mar 27, 2024 •

edited

Loading

stof commented Mar 27, 2024

dannyvw commented Mar 27, 2024

jdreesen commented Mar 27, 2024

massimilianobraglia commented Oct 1, 2024

stof commented Oct 29, 2024

Initial idea for Intersection/Union types #569

Are you sure you want to change the base?

Initial idea for Intersection/Union types #569

Conversation

kschatzle commented Sep 28, 2022 • edited by stof Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschatzle commented Sep 29, 2022

Choose a reason for hiding this comment

stof commented Sep 29, 2022

smyhill commented Jul 31, 2023

kschatzle commented Aug 15, 2023

dannyvw commented Mar 27, 2024 • edited Loading

stof commented Mar 27, 2024

dannyvw commented Mar 27, 2024 • edited Loading

stof commented Mar 27, 2024

dannyvw commented Mar 27, 2024

jdreesen commented Mar 27, 2024

massimilianobraglia commented Oct 1, 2024

stof commented Oct 29, 2024

kschatzle commented Sep 28, 2022 •

edited by stof

Loading

dannyvw commented Mar 27, 2024 •

edited

Loading

dannyvw commented Mar 27, 2024 •

edited

Loading