phpspec / prophecy

Highly opinionated mocking framework for PHP 5.3+
MIT License
8.53k stars 241 forks source link

Intersection types in generated code #535

Open ciaranmcnulty opened 3 years ago

ciaranmcnulty commented 3 years ago

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 commented 1 year ago

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 commented 1 year ago

@ciaranmcnulty / @stof

stof commented 1 year ago

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 commented 1 year ago

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:

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.

kschatzle commented 1 year ago

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 commented 1 year ago

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:

Trying to represent a PHP AST is totally overkill.

kschatzle commented 1 year ago

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 commented 1 year ago

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

smyhill commented 1 year ago

@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 commented 6 months ago

@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 commented 6 months ago

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

lyrixx commented 2 weeks ago

Hello, I juste faced this issue with doctrine orm 3 too.

I'm wondering, would https://github.com/symfony/type-info does the job?