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

add psalm types #647

Closed
wants to merge 2 commits into from
Closed

add psalm types #647

wants to merge 2 commits into from

Conversation

orklah
Copy link

@orklah orklah commented Jan 13, 2020

Hi,

Working on Roave/BetterReflection#525, I needed to refine some types in PHP-Parser.

I tried to either keep readable annotations for IDE like PHPStorm or add a @psalm- annotation.

I take the opportunity to thank you for all the good you did to PHP !

orklah referenced this pull request in vimeo/psalm Oct 28, 2020
@staabm
Copy link
Contributor

staabm commented Oct 28, 2020

This goes into the same direction like I requested in #726

Tools like psam, phpstan, rector the phpstan/psalm ecosystem (namely plugins) etc. Would really benefit from getting types into this lib.

Ps: „psalm-„ prefixed annotations also work for phpstan, so we don‘t need to define redundant things for all the tools

Comment on lines 451 to +452
$kind = $node->getAttribute('kind', Cast\Double::KIND_DOUBLE);
$cast = '';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$kind = $node->getAttribute('kind', Cast\Double::KIND_DOUBLE);
$cast = '';
/** @psalm-var Cast\Double::KIND_* $kind */
$kind = $node->getAttribute('kind', Cast\Double::KIND_DOUBLE);

this will remove the issue about possibly undeclared $cast, tell psalm that $cast is always non-empty-string, and also help catch bugs when comparing it to other types :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I knew way less about Psalm then, I could fix that better now :)

Thanks for the review, but I'm not sure @nikic is interested by improving type documentation in this repo. This PR has waited for months without answers, I kinda made my peace with that!

* @param Node\Const_[] $consts Constant declarations
* @param int $flags Modifiers
* @param array $attributes Additional attributes
* @param list<Node\Const_> $consts Constant declarations

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need the psalm prefix when using list, right?
@psalm-param list<...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends what tool you're aiming. Psalm, Phpstan, Phpstorm read this notation just right. Not sure about other tools

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I thought list<> was psalm's syntax, therefore to be able to use it you needed to prefix it. But maybe I am wrong. In that case, It's fine :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is, but it's now recognized by other tools and IDEs (phpstan considers it an alias of array<int, XXX> and phpstorm sees it as an alias of XXX[])

/**
* @var Node\Const_[] Constant declarations
* @psalm-var list<Node\Const_>
*/
public $consts;

/**
* Constructs a class const list node.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this comment. What do you think?

Comment on lines +12 to +13
* @var Node\Const_[] Constant declarations
* @psalm-var list<Node\Const_>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keeping only @psalm-var list<Node\Const_> Constant declarations would be sufficient.
There is no need to keep the other one, right?

@orklah
Copy link
Author

orklah commented Jan 8, 2022

I'm closing this, it's way too old now and it was very incomplete.

As a Psalm maintainer though, I'd very much like to do a full PR with all the types needed for third party tools to consume. Psalm itself has a lot of dedicated code, configs and stubs in order to fully understand PHP-Parser and it would be easier to maintain them upstream instead of duplicating this work into every third party lib (see phpstan/phpstan-php-parser#12).

If you ever feel like it, please ping me and I'll add those in whatever format you see fit.

@orklah orklah closed this Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants