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

Intersection types in generated code #535

Open
ciaranmcnulty opened this issue Aug 10, 2021 · 11 comments
Open

Intersection types in generated code #535

ciaranmcnulty opened this issue Aug 10, 2021 · 11 comments
Labels

Comments

@ciaranmcnulty
Copy link
Member

We need to ensure that if a class uses intersection return or argument type hints, that we generate the correct code when it's doubled.

Based on the work for union types this could be complex :/

https://php.watch/versions/8.1/intersection-types

@kschatzle
Copy link
Contributor

Somewhat looked into this. Going to throw a wild idea out there.

The slow incorporation of a code generation tool.

Incorporating a code generation tool into prophecy may allow for a faster development of the core mock'ing system currently in place. Code generation and parsing are a subject that has a depth and complexity that pushes the limit of casual understanding. Simply put, there's only so many hours in the day, and code generation vs unit testing seems to be two different buckets of work.

While looking to integrate intersection-type I immediately recognized the need to separate "UnionType" and "IntersectionType". Similar to how you guys have done with NodeTypeAbstract and children Return/Parameter. I also see attributes of types "nullable" being useful. My underlying point is that I could personally see the codebase moving forward with a class hierarchy similar to other code generation libraries.

That begs the question, build it or utilize a library.

A code generation tool that has the attributes and associations built in are pretty close to what would be built regardless. Utilizing another library I think makes sense.

That said PHP-Parser is the tool that I would look into at the moment. The library is used in other contexts that require reading and writing of PHP code. I'm sure you guys could name them offhand (PHPStan, Rector, etc..)

I personally would not use every capability of PHP-Parser outright A certain amount of caution is called for with a user base as large as prophecy's. I could see a good first step in simply using PHP-Parser's Node system as a replacement for the current Prophecy's Node system. The ClassMirror and ClassCodeGenerator would isolate PHP-Parser to ensure it can be removed if necessary at a future point. The changes would also not use the parsing or code generation capability of PHP-Parser. More time and resources may be needed to see if using those features even makes sense for Prophecy. Even if it does, a new major version may be necessary. Suffice to say that it seems like a larger undertaking when the immediate goals that I'd like to help with would be PHP version support.

I'm comparing that with the simplicity that you guys have with the current code generation. If you did not want to use PHP-Parser I would most likely mimic the hierarchy of their node systems combined with the current ClassMirror/Generation.

I will probably put a pull request that at least mimics the signatures of the PHP-Parser node system in the very least unless you guys have a strong opinion one way or the other.

What are your thoughts?

@kschatzle
Copy link
Contributor

@ciaranmcnulty / @stof

@stof
Copy link
Member

stof commented Sep 16, 2022

The issue we have is that our representation of types in TypeNodeAbstract is based on an array of strings, which is too simplistic to support both union and intersection types. That's what we need to solve (and then update the code generation to handle it).

Switching to PHP-Parser does not help solving the issue to me. We don't have any issue about parsing PHP today (because we don't parse it at all).

@kschatzle
Copy link
Contributor

kschatzle commented Sep 16, 2022

TypeNodeAbstract is based on an array of strings, which is too simplistic to support both union and intersection types.

I agree. I believe that PHP-Parser either has a solution we can use or has at least inspired me to utilize their hierarchy (aka PHP's hierarchy) Here's one of two solutions I'll put forth.

Reuse PhpParser Node classes right away. I would utilize the following:

  • Node\ComplexType.php Subtype for when you have to do more work to get the actual value.
  • Node\Identifier.php A classname that represents a class in a typehint. Like the A in A $class.
  • Node\Name.php Similar to Identifier, but is a namespaced typehint.
  • Node\IntersectionType.php Container for an array of Name|Identifier's. Can product A|B.
  • Node\Nullable.php Wrapper for Identifier|Name that says it can be null. ?A
  • Node\Param.php One "parameter" that can have type null|string|Identifier|Name|ComplexType.
  • Node\UnionType.php. Container for array of (Name|Identifier|IntersectionType). (A&B)
  • Node\Stmt\ClassNode.php. Represents a class.
  • Node\Stmt\ClassMethod.php. Represents a class's method.

new IntersectionType(new Name(A::class), new Name(B::class))->__toString() may produce A|B.

new UnionType(new Name(A::class), new Name(B::class), new IntersectionType(new Name(C::class), new Name(D::class)))->__toString(); may produce A&B&(C|D). <--- (this is invalid, my mistake)

Or build our own inspired by the PHP-Parser hierarchy. Same concept except I would only use the signatures, not the guts. The guts I would create, inspired by PHP-Parser. We don't need all of the capability, just enough to build the signatures.

  • Node\ComplexType.php
  • Node\Identifier.php
  • Node\IntersectionType.php
  • Node\Name.php
  • Node\Nullable.php
  • Node\Param.php
  • Node\UnionType.php.
  • Node\Stmt\ClassNode.php.
  • Node\Stmt\ClassMethod.php.

@kschatzle
Copy link
Contributor

kschatzle commented Sep 16, 2022

We don't have any issue about parsing PHP today (because we don't parse it at all).

I would keep ClassMirror to be responsible for turning Reflection into some of the nodes above. Then use ClassCodeGenerator to turn those nodes into PHP in almost the same way we do now.

@stof
Copy link
Member

stof commented Sep 16, 2022

To be, we don't need to replace the existing ClassNode, MethodNode or ArgumentNode, which already match the needs of prophecy.
The only thing we need to replace is the representation of types, i.e. TypeNodeAbstract and its subclasses. And we don't need to care about the distinction between Name and Identifier that PHP-Parser has for instance.
To me, our representation of types should be inspired by the one in Reflection:

  • ReflectionNamedType: a simple type (class name or builtin type, which we might want to distinguish in prophecy to simplify the code, as builtin types should not be generated with a \ prepended while class names should)
  • ReflectionIntersectionType: an intersection type, containing ReflectionNamedType instances
  • ReflectionUnionType: a union type, containing ReflectionNamedType or ReflectionIntersectionType instances

Trying to represent a PHP AST is totally overkill.

@kschatzle
Copy link
Contributor

Agreed. A lot of it would be overkill. The relationships that both Reflection and PHP-Parser have reflect the underlying AST at some level. The relationships are really what are needed though. I'll give it a go.

@stof
Copy link
Member

stof commented Sep 16, 2022

Well, for the relation between class, methods and arguments, our existing nodes already represent them (for what we need).

@smyhill
Copy link

smyhill commented Jul 31, 2023

@ciaranmcnulty @stof Just curious what the outlook is for doubling intersection types? It would be very useful for some classes I am currently working with. Would love to help out where I can!

@maxnz
Copy link

maxnz commented Feb 19, 2024

@ciaranmcnulty @stof @kschatzle Bumping this thread because Doctrine ORM v3.0.0 added a return type to EntityRepository:matching which causes prophesizing an EntityRepository to fail with a ClassMirrorException.

I'm also happy to help however possible

@stof
Copy link
Member

stof commented Feb 29, 2024

There was a proposal at #569 but the work has not been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants