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

Feature - Psalm annotations #93

Merged
merged 11 commits into from
Jul 25, 2023
Merged

Conversation

athrawes
Copy link
Contributor

Should go a ways towards #83

I've been using this library in a few of my projects, and found myself wanting these annotations.

Some notes about this PR:

  • PR adds vimeo/psalm as a require-dev dependency (^4.18 for PHP >= 7.1, ^5.13 for PHP >= 7.4). This also adds a default Psalm config file (psalm.xml).
  • Updated the return types of many functions in the main iter\ namespace to return \Generator instead of \Iterator. This is to satisfy the type requirements of makeRewindable and callRewindable, and is actually what these functions return.
  • Psalm flagged an issue in isEmpty(), as \IteratorAggregate->getIterator() only returns \Traversable, not \Iterator.
    In the edge case that the returned \Traversable is not also \Iterator, this could have caused an issue.
    This is fixed in this PR.
  • In functions that take a $levels = INF parameter or similar, changed INF to PHP_INT_MAX as INF is a float type and not int as declared. In these cases, $levels can never exceed the PHP_INT_MAX anyhow.
  • Psalm does not have support for recursive types. Functions which use recursive types have had a note added to the
    docblock to explain this.
  • I've not added docblocks to the functions in the \iter\rewindable namespace, as that would necessarily mean that any changes to the regular function would also need to be reflected in the docblocks for these methods. This does mean that these functions do not benefit from the Psalm type annotations currently.

Please do reach out if there are any questions!

Andrew Moyes added 4 commits July 18, 2023 01:02
This is mainly to help validate the type definitions.
`\IteratorAggregate->getIterator()` does not return an `\Iterator`, it returns a
`\Traversable`, which does not necessarily provide a `valid()` method.

In such cases, further checks are needed to determine what has been returned.
@nikic
Copy link
Owner

nikic commented Jul 18, 2023

High level comment: This uses a lot of @psalm- annotations, many (all?) of which don't look necessary to me. As long as the feature is supported in all of psalm, phpstan and phpstorm, it's okay to just use in a plain @param etc. We shouldn't need a psalm prefix for things like array-key or callable signatures.

Updated the return types of many functions in the main iter\ namespace to return \Generator instead of \Iterator. This is to satisfy the type requirements of makeRewindable and callRewindable, and is actually what these functions return.

I'm not a fan of this bit. The fact that these are implemented using generators is an implementation detail, not something that should be exposed as an API-level guarantee. We should reserve the right to implement these using custom Iterators in the future.

I also don't think that RewindableGenerator fundamentally requires a Generator. It should work fine with an Iterator as well. The only Generator-specific functionality it has is that it also forwards the send and throw methods. It was probably a bad idea to implement those, as they're not really relevant to this library.

I think we should either suppress those undefined methods for the non-generator case, or split up RewindableGenerator into RewindableIterator and a RewindableGenerator extension for it, where the former only requires the Iterator interface, and the latter the Generator interface.

@athrawes
Copy link
Contributor Author

Thanks for the feedback!

For the @psalm- params, I'll go ahead reduce the number of places these are used. One note though - for callable parameters, while psalm and phpstan play nicely with signatures like @param callable(T): U $function, phpstorm does not. For such parameters, I'm thinking about if it'd make sense to keep the existing @psalm-param tag and also add a corresponding @phpstan-param tag 🤔

I hear what you're saying about Generators / RewindableGenerator. The quick and dirty fix here would indeed be to simply suppress the type contradiction. That said, looking at RewindableGenerator I do see that send() and throw() aren't used by any other code here. I'm tempted by the suggestion around having a RewindableIterator implementation. I'll give this some more thought, and would love to hear your opinion on this.

On a side note, I can see that the CI actions for PHP 7.1 are failing on the composer-install step; do you happen to have any ideas on what's failing there? All I can see in the logs is that composer exited with an error code, but testing PHP 7.1 with composer v1 in a container locally seems to succeed for me.

@nikic
Copy link
Owner

nikic commented Jul 19, 2023

For the @psalm- params, I'll go ahead reduce the number of places these are used. One note though - for callable parameters, while psalm and phpstan play nicely with signatures like @param callable(T): U $function, phpstorm does not. For such parameters, I'm thinking about if it'd make sense to keep the existing @psalm-param tag and also add a corresponding @phpstan-param tag

From a quick test, this seems to at least parse correctly, so I think that's fine. As long as it doesn't get worse treatment than plain callable we're not worse off than before...

I hear what you're saying about Generators / RewindableGenerator. The quick and dirty fix here would indeed be to simply suppress the type contradiction. That said, looking at RewindableGenerator I do see that send() and throw() aren't used by any other code here. I'm tempted by the suggestion around having a RewindableIterator implementation. I'll give this some more thought, and would love to hear your opinion on this.

I'd probably go for the RewindableIterator -- it doesn't seem like this would be particularly hard to do.

On a side note, I can see that the CI actions for PHP 7.1 are failing on the composer-install step; do you happen to have any ideas on what's failing there? All I can see in the logs is that composer exited with an error code, but testing PHP 7.1 with composer v1 in a container locally seems to succeed for me.

No idea what's going on there. Maybe we could try whether updating to ramsey/composer-install@v2 fixes that?

@athrawes
Copy link
Contributor Author

Right, I've managed to sort the @psalm- tags. I've even figured out how to make recent versions of phpstorm happy with the callable syntax. These should be a lot simpler.

For the RewindableIterator, I've gone ahead and implemented it. For backwards-compatibility purposes, I've left a RewindableGenerator which extends off of the RewindableIterator and marked it as deprecated. I've also updated the makeRewindable, callRewindable, and \iter\rewindable\* functions to use the new RewindableIterator by default.

I've even managed to figure out the CI issue. Upgrading to ramsey/composer-install@v2 didn't directly fix the issue, but gave me better logs to track down the issue. It turns out that for some reason the setup-php step wasn't installing composer for the PHP 7.1 step, which I've solved by nicely asking it to do so.

Please have a look when you have a moment 😄

Copy link
Owner

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

<directory name="vendor" />
</ignoreFiles>
</projectFiles>
</psalm>
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need a phpstan config as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike psalm, phpstan does not require an explicit config. A config can be added to be explicit about things if so desired.

src/iter.func.php Outdated Show resolved Hide resolved
src/iter.php Outdated Show resolved Hide resolved
src/iter.rewindable.php Outdated Show resolved Hide resolved
@nikic nikic merged commit d9f88bc into nikic:master Jul 25, 2023
6 checks passed
@nikic nikic mentioned this pull request Jul 29, 2023
@athrawes athrawes deleted the feature/psalm-annotations branch August 31, 2023 00:11
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.

2 participants