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

Static Analysis Checks: Common Operations (II) #109

Merged
merged 5 commits into from Jun 16, 2021

Conversation

AlexandruGG
Copy link
Collaborator

This PR adds more static analysis checks for commonly-used methods, and fixes a few type hints in the process.

  • filter
  • first
  • head
  • last

@@ -711,20 +711,22 @@ Signature: ``Collection::falsy();``
filter
~~~~~~

Filter collection items based on one or more callbacks.
Filter collection items based on one or more callbacks. Multiple callbacks will be treated as a logical ``AND``.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel like it's a bit inconsistent with the others that filter works like this. I was expecting multiple callbacks to behave like logical OR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drupol what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this is one of the gotcha(s) I couldn't fix in a lazy way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, no worries I've updated the docs and example 😁

@AlexandruGG
Copy link
Collaborator Author

Weird, looks like unit tests on windows on php 8 sometimes get stuck taking forever. Saw it happen once previously but I cannot cancel the job 🤷‍♂️. 10 min and still going

@AlexandruGG AlexandruGG marked this pull request as ready for review June 15, 2021 20:36
Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hello,
Most of the things are great, I just added a minor remark.
Let me know what you think.


// Point free style.
return $head;
return Head::of();
Copy link
Collaborator

Choose a reason for hiding this comment

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

By removing this, you get 2 new minor errors in PSalm:

 ~/d/g/collection   feature/more-static-analysis-checks $…  ./vendor/bin/psalm --show-info=true src/Operation/First.php                                            
Scanning files...
Analyzing files...

░

INFO: MixedReturnTypeCoercion - src/Operation/First.php:23:16 - The declared return type 'Closure(Iterator<TKey:loophp\collection\Operation\First as mixed, T:loophp\collection\Operation\First as mixed>):Generator<TKey:loophp\collection\Operation\First as mixed, T:loophp\collection\Operation\First as mixed, mixed, mixed>' for loophp\collection\Operation\First::__invoke is more specific than the inferred return type 'Closure' (see https://psalm.dev/197)
     * @return Closure(Iterator<TKey, T>): Generator<TKey, T>


INFO: MixedReturnTypeCoercion - src/Operation/First.php:27:16 - The type 'Closure' is more general than the declared return type 'Closure(Iterator<TKey:loophp\collection\Operation\First as mixed, T:loophp\collection\Operation\First as mixed>):Generator<TKey:loophp\collection\Operation\First as mixed, T:loophp\collection\Operation\First as mixed, mixed, mixed>' for loophp\collection\Operation\First::__invoke (see https://psalm.dev/197)
        return Head::of();


------------------------------
No errors found!
------------------------------
2 other issues found.
------------------------------

Checks took 0.54 seconds and used 235.298MB of memory
Psalm was able to infer types for 100% of the codebase
 ~/d/g/collection   feature/more-static-analysis-checks $

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oooh I didn't notice those, I guess because of the info level. I'll see if I can fix those, if not I'll revert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drupol I see now why it happens, it's because AbstractOperation::of has the simple return type Closure. I think this makes sense as they are not all the same.

I don't think it should be a problem because it's just an info thing, but I've reverted this change to keep the same style as others

@drupol drupol merged commit ee3bf3b into loophp:master Jun 16, 2021
@AlexandruGG
Copy link
Collaborator Author

thanks!

@AlexandruGG AlexandruGG deleted the feature/more-static-analysis-checks branch June 16, 2021 19:22
@drupol
Copy link
Collaborator

drupol commented Jun 16, 2021

Thanks you!!!

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.

None yet

2 participants