Update various typing information #196

drupol · 2021-09-13T15:47:35Z

This PR:

The Associate class is no more variadic - (Minor BC Break - only when using the Associate class on its own)
Fix Zip and Associate typing information
Update Unpack operation and its interface.
Has static analysis tests (psalm, phpstan)
Associate documentation/examples has been updated - added more examples.

drupol · 2021-09-13T16:02:54Z

src/Contract/Operation/Zipable.php

     *
-     * @return Collection<list<mixed>, list<mixed>>
+     * @param iterable<UKey, U> ...$iterables


I know that it's not possible to type this thing as it is a variadic parameter.

I don't know if it's a good idea, but I had the idea to do the typing of only one element, knowing that it would not be completely valid if there are more than one parameters given.

WDYT?

I get what you're looking to do here. The only issue is that when a user will try to use it with multiple iterables of different types they will get an error and so think that it's not supposed to be used like that.

I'm not sure how people normally use zip, maybe it wouldn't be an issue if most often it's used with a single type.

I leave this question open as I don't have any idea yet on what is best to do...

drupol · 2021-09-13T17:04:04Z

I deleted my previous comment and fixed the issue with PSalm and PHPStan, this is now ready for review.

AlexandruGG · 2021-09-14T06:40:19Z

src/Contract/Operation/Unpackable.php

- * @template T of array{0: NewTKey, 1: NewT}
- *
- * @template NewTKey
- * @template NewT
+ * @template T


I don't get this change. Was the existing annotation not working? Or is it not correct?

There's no static analysis check for unpack so this would be the time to add one if we're changing these

Despite the fact that this was technically valid, I changed that because:

All operations interfaces are using 2 templates (TKey and T), Unpackable was using 4 (TKey, T, NewTKey, NewT)

Incoming templates (TKey and T) should be opaque, at least that's how it works on every other operations. We should not describe the structure of these or else it will never end. If we start to do that, where should we put the limit or depth of details?

By removing that, we can remove a bunch of "unsupported" lines from phpstan-unsupported-baseline.neon

Regarding the SA for Unpack, I will add it today for sure!

Ok, I reverted the changes, I cannot get through it properly in PHPStan.

AlexandruGG · 2021-09-14T06:50:19Z

src/Operation/Normalize.php

-             * @return Generator<int, T>
+             * @return Generator<int, T, mixed, void>


does it add to the type information to add the other Generator generics, or were they already inferred as those types?

Hi Alex,

I don't understand your question, can you elaborate?

To give you a bit of context, this was spotted by PHPStan when using (see this tweet):

parameters: checkExplicitMixed: true

This is why I added some of these things here and there.

ok, sounds good! Should we add that in the PHPStan config in a separate PR?

No, not yet, I'll wait the next release of Phpstan before making those changes. I even think that it will be enabled by default in the new level 9 :)

tests/static-analysis/zip.php

phpstan-unsupported-baseline.neon

Found those small glitches using PHPStan upcoming features.

drupol · 2021-09-14T17:00:43Z

phpstan-unsupported-baseline.neon

@@ -20,7 +20,18 @@ parameters:
 			count: 1
 			path: src/Contract/Operation/Unpackable.php

+		-
+			message: "#^Parameter \\#1 \\$callable of class loophp\\\\collection\\\\Iterator\\\\ClosureIterator constructor expects callable\\(\\.\\.\\.mixed\\)\\: iterable\\<int, string\\>, Closure\\(mixed, bool\\)\\: Generator\\<int, string, mixed, void\\> given\\.$#"


This is the only part (The 2 new entries in this file) that I cannot fix. If you have any idea, just let me know.

it's a bit difficult to tell at a glance, but leave this will me I will try to see if I can figure out the problem and fix it

Ok, thanks! :)

@drupol so after testing I realised this happens when passing a callable which takes multiple parameters, both to the ClosureIterator and the Collection object itself. Why? I'm not entirely sure but see the latest commit and the other comment for the changes I did and let me know if you think that works

src/Iterator/TypedIterator.php

Co-authored-by: Alex Gidei <[email protected]>

AlexandruGG · 2021-09-15T20:03:47Z

src/Collection.php

-        return new self(
-            static fn (string $string, string $delimiter): Iterator => new StringIterator($string, $delimiter),
-            [$string, $delimiter]
-        );
+        return new self(static fn (): Iterator => new StringIterator($string, $delimiter));


does it matter if we pass the parameter to the closure or we just let it capture it from the environment? At least with the testing we have it doesn't seem to make any difference, but it fixes a PHPStan error

Sure technically this is valid, it's just confusing according to me, this is why I made the change.

I wish I could understand why PHPStan doesn't like it.

AlexandruGG · 2021-09-16T08:50:38Z

@drupol thank you for the work on this 👍

drupol added 2 commits September 13, 2021 17:45

refactor: Update Associate typing information, add tests.

3ae485a

refactor: Update Zip typing information, add tests.

e2ef673

drupol added the enhancement New feature or request label Sep 13, 2021

drupol assigned drupol and unassigned drupol Sep 13, 2021

drupol commented Sep 13, 2021

View reviewed changes

drupol self-assigned this Sep 13, 2021

drupol added 2 commits September 13, 2021 18:54

refactor: Associate operation is not more variadic.

673f3ee

docs: Update Associate documentation and examples.

b722eb9

drupol changed the title ~~[SA] Update Zip and Associate typing information~~ Update Zip and Associate typing information Sep 13, 2021

drupol marked this pull request as ready for review September 13, 2021 17:02

drupol requested a review from AlexandruGG September 13, 2021 17:12

drupol force-pushed the sa/fix-minor-operations-typing-information branch from 5fc373f to 5b3e278 Compare September 13, 2021 19:29

drupol marked this pull request as draft September 13, 2021 19:32

drupol marked this pull request as ready for review September 13, 2021 20:20

drupol changed the title ~~Update Zip and Associate typing information~~ Update various typing information Sep 13, 2021

drupol force-pushed the sa/fix-minor-operations-typing-information branch 2 times, most recently from b2be1a7 to d787b55 Compare September 14, 2021 06:25

AlexandruGG reviewed Sep 14, 2021

View reviewed changes

tests/static-analysis/zip.php Show resolved Hide resolved

AlexandruGG reviewed Sep 14, 2021

View reviewed changes

phpstan-unsupported-baseline.neon Outdated Show resolved Hide resolved

drupol marked this pull request as draft September 14, 2021 12:48

drupol added 3 commits September 14, 2021 17:51

Fix various minor SA typing information.

40ce120

Found those small glitches using PHPStan upcoming features.

refactor: Replace IteratorIterator with ClosureIterator.

2f81bce

Update PHPStan baseline - unable to get rid of the issue yet.

dafa9b6

drupol force-pushed the sa/fix-minor-operations-typing-information branch from 248fc9e to dafa9b6 Compare September 14, 2021 15:52

tests: SA tests of Zip operation - add more tests.

b4cdd4a

drupol commented Sep 14, 2021

View reviewed changes

drupol marked this pull request as ready for review September 14, 2021 17:04

drupol removed their assignment Sep 14, 2021

AlexandruGG reviewed Sep 14, 2021

View reviewed changes

src/Iterator/TypedIterator.php Outdated Show resolved Hide resolved

drupol and others added 2 commits September 14, 2021 23:52

Update src/Iterator/TypedIterator.php

b449188

Co-authored-by: Alex Gidei <[email protected]>

Fix PHPStan unsupported errors

0b8d1de

AlexandruGG reviewed Sep 15, 2021

View reviewed changes

drupol merged commit ba40bee into master Sep 16, 2021

drupol deleted the sa/fix-minor-operations-typing-information branch September 16, 2021 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update various typing information #196

Update various typing information #196

drupol commented Sep 13, 2021 •

edited

Loading

drupol Sep 13, 2021

AlexandruGG Sep 14, 2021

drupol Sep 14, 2021

drupol commented Sep 13, 2021 •

edited

Loading

AlexandruGG Sep 14, 2021

drupol Sep 14, 2021

drupol Sep 14, 2021

AlexandruGG Sep 14, 2021

drupol Sep 14, 2021

AlexandruGG Sep 14, 2021

drupol Sep 14, 2021

drupol Sep 14, 2021

AlexandruGG Sep 14, 2021

drupol Sep 15, 2021

AlexandruGG Sep 15, 2021

AlexandruGG Sep 15, 2021

drupol Sep 16, 2021

AlexandruGG commented Sep 16, 2021

		* @return Generator<int, T>
		* @return Generator<int, T, mixed, void>

Update various typing information #196

Update various typing information #196

Conversation

drupol commented Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drupol commented Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandruGG commented Sep 16, 2021

drupol commented Sep 13, 2021 •

edited

Loading

drupol commented Sep 13, 2021 •

edited

Loading