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.

Sign up for GitHub

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

Jump to bottom

Support conditional types #106

Merged
merged 9 commits into from Mar 28, 2022
Merged

Support conditional types #106

merged 9 commits into from Mar 28, 2022

Conversation

rvanvelzen
Copy link
Contributor

Part of implementation for phpstan/phpstan#3853

@rvanvelzen rvanvelzen force-pushed the conditional-type branch 2 times, most recently from 90c9df6 to 7402e40 Compare March 25, 2022 10:54
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 25, 2022
@rvanvelzen
Implement api for conditional type
7002de1 
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 :)
@rvanvelzen rvanvelzen mentioned this pull request Mar 25, 2022
Closed
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 25, 2022
@rvanvelzen
Implement api for conditional type
0da3819 
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 :)
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 25, 2022
@rvanvelzen
Implement api for conditional type
5eea9c0 
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 :)
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 25, 2022
@rvanvelzen
Implement api for conditional type
a5dcf8e 
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 :)
ondrejmirtes
ondrejmirtes requested changes Mar 28, 2022
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a 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
public $targetType;

/** @var TypeNode */
public $trueType;
Copy link
Member

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
@@ -59,7 +60,7 @@ public function currentTokenOffset(): int

public function isCurrentTokenValue(string $tokenValue): bool
{
return $this->tokens[$this->index][Lexer::VALUE_OFFSET] === $tokenValue;
return strcasecmp($this->tokens[$this->index][Lexer::VALUE_OFFSET], $tokenValue) === 0;
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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
@@ -44,7 +46,9 @@ public function parse(TokenIterator $tokens): Ast\Type\TypeNode
private function parseAtomic(TokenIterator $tokens): Ast\Type\TypeNode
{
if ($tokens->tryConsumeTokenType(Lexer::TOKEN_OPEN_PARENTHESES)) {
$tokens->tryConsumeTokenType(Lexer::TOKEN_PHPDOC_EOL);
Copy link
Member

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?

Copy link
Contributor Author

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?

@rvanvelzen
Copy link
Contributor Author

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 $id is 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).

@ondrejmirtes
Copy link
Member

Separate ConditionalTypeForParameter with a plain string $parameterName property :) Thank you very much.

@rvanvelzen
Copy link
Contributor Author

I believe I addresses everything. Please let me know if more changes are necessary

@ondrejmirtes ondrejmirtes merged commit 445ccb7 into phpstan:1.3.x Mar 28, 2022
@ondrejmirtes
Copy link
Member

Thank you!

ondrejmirtes pushed a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 28, 2022
@rvanvelzen @ondrejmirtes
Implement api for conditional type
86c7d1e 
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 :)
ondrejmirtes pushed a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 28, 2022
@rvanvelzen @ondrejmirtes
Implement api for conditional type
c48d262 
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 :)
staabm
staabm reviewed Mar 28, 2022
View reviewed changes
doc/grammars/type.abnf
Union
= 1*(TokenUnion Atomic)

Intersection
= 1*(TokenIntersection Atomic)

Conditional
= 1*ByteHorizontalWs TokenIs [TokenNot] Atomic TokenNullable Atomic TokenColon Atomic
Copy link
Contributor

@staabm staabm Mar 28, 2022 •
edited

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

@rvanvelzen rvanvelzen deleted the conditional-type branch April 13, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staabm staabm staabm left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rvanvelzen @ondrejmirtes @staabm