-
Notifications
You must be signed in to change notification settings - Fork 242
Intersection types #637
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 #637
Conversation
| { | ||
| if ($type instanceof ReflectionIntersectionType) { | ||
| foreach ($type->getTypes() as $innerReflectionType) { | ||
| $innerTypes[] = new SimpleType($innerReflectionType->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this missing the resolution of the type name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should use createTypeFromReflection for the inner type as well.
| /** | ||
| * @param string|TypeInterface ...$types | ||
| */ | ||
| public function __construct(string|TypeInterface ...$types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the new API allows null as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep the new API will be null|string|TypeInterface $type = null, you're absolutely right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this needs to be updated to support passing null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad it should be new BuiltinType('null') in the new API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not talking about the case where of using null as return type, but of the case of not having a return type at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new ReturnTypeNode() will work for this (just like it was before). Do you want the new API to support new ReturnTypeNode(null) ? (I though it was a good idea, not sure about it now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, new ReturnTypeNode(null) should work. Otherwise, we won't be able to remove the BC layer in the next version anyway (where we don't want to have a variadic argument anymore)
| } | ||
| } | ||
|
|
||
| if (\PHP_VERSION_ID >= 80000 && isset($this->types['mixed']) && count($this->types) !== 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removing this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question is more like why is this method guardIsValidType still here. I will remove it on cleanup. The UnionType checks this mixed-thing already.
| } | ||
|
|
||
| /** | ||
| * @todo: put this in SimpleType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot really go in SimpleType, as some of those are about the full type, not each simple type used in a union
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess each type will have its own exceptions, this still needs to be done.
| * @todo: put this in SimpleType | ||
| * @return void | ||
| */ | ||
| protected function guardIsValidType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful. ReturnTypeNode extends this class to override this method, but you still use $this->types in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After cleanup this method should not even exist anymore here.
5250a7c to
ebb7746
Compare
|
Thank you for your review @stof . And good news! I fixed the bugs in my previous implementation. (some you noticed in your review!) Keeping backward compatibility has been a real pain! But it WORKS. (test suite green 100%) It's currently a fully working PR for intersection type.... But only for return types! Some work needs to be done for cleaning... But it also needs php to be bumped to 8.1+ (which would probably be better in another PR), and a lot of cleaning. But for now, since it's a lot of work and personal investment, I'd like you to tell me if the implementation is ok before working on it again. |
e0672c9 to
facea20
Compare
| { | ||
| /** @var array<string, string> */ | ||
| protected $types = []; | ||
| protected TypeInterface|null $type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making the new property private (if child classes need to read it, there is a public getter they can use)
| /** | ||
| * @param string|TypeInterface ...$types | ||
| */ | ||
| public function __construct(string|TypeInterface ...$types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this needs to be updated to support passing null
|
|
||
| if ($type instanceof UnionType) { | ||
| return join('|', array_map( | ||
| fn (TypeInterface $type) => $this->generateSubType($type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic does not support DNF types. Parenthesis are required around the intersection types used in a union type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, and fixed!
|
@stof I updated the PR following your insights, thank you for the detailed review. ⭐ I still need to track deprecated calls and add deprecation triggers, and I will do it promptly as soon as you validate that new implementation. |
stof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach looks good. I still recommend changing the way we manage the source code representation of a given type.
Please start working on the deprecation layer as well.
| $this->types[$type] = $type; | ||
| } | ||
| $deprecation = 'Only 1 type will be supported in the future, strings are no longer supported as type.'; | ||
| if (count($types) === 1 && $types[0] instanceof TypeInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should accept count($types) === 1 && $types[0] === null as well, to assign null in the property
|
|
||
| private function prefixWithNsSeparator(string $type): string | ||
| { | ||
| // Avoid double-prefixing if already prefixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new API should not deal with that, expecting the unprefixed class name to be passed instead (which might be guarded to avoid issues). The BC layer in TypeNodeAbstract would have to handle the BC layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unmarking this as resolved, because you kept this logic (without any answer about why my comment got ignored)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done! This method is only for the __toString() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ObjectType is implemented as expecting class-string, we won't care about a type starting with \, as that won't happen. So __toString should just be return '\\'.$this->type;
And that's also solving the case of equals which currently treat new ObjectType('stdClass') and new ObjectType('\stdClass') as not equal.
Please explain what you mean because I refactored precisely with your recommandation here (the "ObjectType" vs "BuiltinType" thing in particular) |
|
@Nek- that's what I detailed more in #637 (comment) and #637 (comment) |
| public function it_can_not_double_intersection_return_types(): void | ||
| public function it_can_double_intersection_return_types(): void | ||
| { | ||
| if (PHP_VERSION_ID < 80100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like those version checks were not properly cleaned when bumping the min PHP version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to be done in a separated work?
This follow discussion on PR phpspec#637
|
@stof I'm still working on this! Removing deprecated usages is a new mini challenge and made me discover some features I forgot on the way, I will add according tests as well. But I have an issue with triggering deprecation notice. It does makes the phpspec test suite fail, I can't make it pass with the trigger working. (I guess I'm not the first one hitting this issue since other deprecated methods do not throw deprecation error) |
|
Indeed, phpspec does not have a good handling of deprecations. I opened phpspec/phpspec#1494 about it. reading the code of phpspec, I think the pattern used in |
Awesome. I will use this. Thanks! |
|
@stof finally something mergeable 😅 Thank you so much for your critical reviews. (I secretly expect to get some more again haha) |
|
@stof I updated the PR it's waiting for your review. 🙏 |
|
|
||
| private function prefixWithNsSeparator(string $type): string | ||
| { | ||
| // Avoid double-prefixing if already prefixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unmarking this as resolved, because you kept this logic (without any answer about why my comment got ignored)
This follow discussion on PR phpspec#637
0d5c75e to
1a5f210
Compare
d7098f6 to
876f8ee
Compare
|
@stof I think I did all your review. Do you want me to squash and detail a bit inside the commit what happen here? |
yes, that would be great |
876f8ee to
d044dbb
Compare
|
@stof done! |
- Adds a new layer for Types inside the generator (with keeping the old one for BC reasons!) - Introduce a way of triggering deprecation without making phpspec failing by adding symfony/deprecation-contracts to the project - Update all internal usages to make prophecy use its own new API
d044dbb to
d6d9103
Compare
|
It took time, but this is finally merged. Thanks a lot @Nek- ! |
|
Thanks @Nek- (nice to see you again) |
|
Thanks @Nek- ! |
Note about the design: I created a new sub-api for type management because the current one is not compatible with intersections but needs to exist to keep backward compatibility.
What still needs to be done:
Please have a look and tell me if this kind of implementation would be ok for you. Thanks!
Stands as replacement for #569 and should fix #535 and #558
Update: the PR is ready and waiting to be merged :)