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 phpstan typing annotations #250

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

KmeCnin
Copy link

@KmeCnin KmeCnin commented Nov 24, 2023

Remaining functions not typed with PhpStan (seems not doable with current features):

  • compose
  • curry
  • reduce_right
  • suppress_error

@KmeCnin KmeCnin force-pushed the phpstan branch 7 times, most recently from 213a9b7 to b21f921 Compare November 24, 2023 23:32
@KmeCnin KmeCnin force-pushed the phpstan branch 2 times, most recently from 2ca165b to 98a23c8 Compare December 1, 2023 19:19
@lstrojny
Copy link
Owner

lstrojny commented Dec 1, 2023

Terrific work, thank you very much. Let me know when you thinking's ready for merge

src/Functional/Flatten.php Outdated Show resolved Hide resolved
src/Functional/Map.php Outdated Show resolved Hide resolved
@KmeCnin
Copy link
Author

KmeCnin commented Dec 8, 2023

@lstrojny my work on phpstan support relies on this commit that fixes/simplify some behaviors: 68ad0fd.
Do you think that you could first merge this commit before I'm proposing mine?

src/Functional/Concat.php Outdated Show resolved Hide resolved
src/Functional/InvokeFirst.php Outdated Show resolved Hide resolved
src/Functional/LessThan.php Show resolved Hide resolved
@lstrojny
Copy link
Owner

Do you think that you could first merge this commit before I'm proposing mine?

The index_of changes would need to be deprecated first. For now why not use a conditional type to express the current behavior?

@KmeCnin KmeCnin force-pushed the phpstan branch 3 times, most recently from 228b6f1 to ea147af Compare January 19, 2024 15:06
@KmeCnin
Copy link
Author

KmeCnin commented Jan 19, 2024

@lstrojny I guess this work is ready to be merge. I don't see further improvements at the moment.
Let me know if it is ok for you.

(I removed the other commit I was previously based on)

* @param iterable<K,V> $collection
* @param callable(V,K,iterable<K,V>):G $callback
*
* @return (G is numeric-string ? array<int,array<K,V>> : array<G,array<K,V>>)

Choose a reason for hiding this comment

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

Is there a way to also hint phpstan when G is surely a (non-numeric) string?

With current annotation I get false-positive in this case:

https://phpstan.org/r/53b4ebbd-2aa1-4d07-a0f2-37f96c117e0a

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is possible right now, see phpstan/phpstan#6847

Also the numeric-string isn’t correct, because this:

["1.2" => "foo"];

The key is a numeric string but the result will be a string array, not an integer array.

Copy link
Author

@KmeCnin KmeCnin Jan 26, 2024

Choose a reason for hiding this comment

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

You got a very good point and this is very sad since I'll have to remove this in some other functions also 😢

src/Functional/Average.php Show resolved Hide resolved
src/Functional/Concat.php Outdated Show resolved Hide resolved
Comment on lines +16 to +21
* @template T
*
* @param callable():T $callback
* @param T $result
*
* @return callable():T
Copy link
Owner

Choose a reason for hiding this comment

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

Would be great to indicate with types that the callback given is type-wise identical. But this depends on phpstan/phpstan#8964 and phpstan/phpstan#8214

src/Functional/CompareObjectHashOn.php Show resolved Hide resolved
Comment on lines 19 to 27
* @template V
* @template V2 of V
*
* @param iterable<V> $collection
* @param V2 $value
* @param bool $strict
*
* @return bool
*
Copy link
Owner

Choose a reason for hiding this comment

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

This needs to be a bit more complicated, to properly type the strictness.

We need a conditional type here where V2 is of V if strict is true but otherwise V2 is independent of V

Copy link
Author

Choose a reason for hiding this comment

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

Good catch.
Here is what I ended with: https://phpstan.org/r/c7d1770b-678b-446f-a285-095557fb6b3e

src/Functional/ErrorToException.php Outdated Show resolved Hide resolved
Comment on lines -29 to +34
if ($callback === null) {
$callback = '\Functional\id';
}

foreach ($collection as $index => $element) {
if (!$callback($element, $index, $collection)) {
if (!(null === $callback ? id($element) : $callback($element, $index, $collection))) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why that change?

Copy link
Author

Choose a reason for hiding this comment

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

Yes sorry I should have explain it but here is the error I get if I keep the previous code:

 ------ ----------------------------------------------------------------------------------- 
  Line   Every.php                                                                          
 ------ ----------------------------------------------------------------------------------- 
  38     Callable '\\Functional\\id'|(callable(V, K, iterable<K, V>): bool) invoked with 3  
         parameters, 1 required.                                                            
 ------ ----------------------------------------------------------------------------------- 

* @param iterable<K,V> $collection
* @param callable(V,K,iterable<K,V>):G $callback
*
* @return (G is numeric-string ? array<int,array<K,V>> : array<G,array<K,V>>)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is possible right now, see phpstan/phpstan#6847

Also the numeric-string isn’t correct, because this:

["1.2" => "foo"];

The key is a numeric string but the result will be a string array, not an integer array.

src/Functional/LessThan.php Show resolved Hide resolved
src/Functional/First.php Outdated Show resolved Hide resolved
@KmeCnin KmeCnin requested a review from lstrojny January 26, 2024 19:43
@KmeCnin
Copy link
Author

KmeCnin commented Jun 10, 2024

Hello @lstrojny! Do you think that we can merge this PR?

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