-
Notifications
You must be signed in to change notification settings - Fork 65
Support conditional types #106
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
Conversation
90c9df6 to
7402e40
Compare
34e7730 to
6e9c452
Compare
6e9c452 to
44ab055
Compare
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.
Also please check this article: https://psalm.dev/articles/conditional-love
From what I understand, you implemented this syntax in this PR:
* @psalm-return ( * T is int * ? static * : array<static> * )
I'd also like to support this:
* @psalm-return (
* $id is int
* ? static
* : array<static>
* ) Since $id is not a type, feel free to do it as a completely different AST node.
Thanks! This looks promising.
src/Ast/Type/ConditionalTypeNode.php
Outdated
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'd rather have if type and else type, like here: https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/Node/Expr/Ternary.php
src/Parser/TokenIterator.php
Outdated
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.
What is this change for?
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 supports e.g. T IS NOT NULL. If you prefer only to support lowercase (i.e. T is not null) I'll revert 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.
Yes, please revert, I'm fine with supporting only a single variant :)
src/Parser/TypeParser.php
Outdated
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 surely has some repercussions for some other parsed types. Can you add some tests demonstrating that?
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.
Sure. I could also provide a separate PR for this change?
I'd also like to support this:
* @psalm-return ( * $id is int * ? static * : array<static> * )
I was planning to implement that syntax separately in a follow-up PR, but I'm fine with including it here as well.
Since
$idis not a type, feel free to do it as a completely different AST node.
Do you mean a separate ConditionalTypeForVariable node with a plain $subjectName property, or have $subject in ConditionalType be a union of TypeNode|ParameterReferenceNode? (All names up for debate).
Separate ConditionalTypeForParameter with a plain string $parameterName property :) Thank you very much.
44ab055 to
903d989
Compare
It'll be easier to implement this for more types properly separately
I believe I addresses everything. Please let me know if more changes are necessary
Thank you!
Part of implementation for phpstan/phpstan#3853, together with phpstan/phpdoc-parser#106. This is a prototype implementation. Comments are welcome, and if I'm too ambitious consider just closing 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.
not sure how/wheter these abnf file ares sensitive to whitespace, but the newly introduced lines use spaces, not tabs
Part of implementation for phpstan/phpstan#3853