Code Analysis Fixes - PHPStan

[AlexandruGG]

This PR aims to fix current issues reported by PHPStan.

• fixing a few issues without major impact
• adding a baseline file for remaining issues
• making static analysis (Psalm, PHPStan) blocking in GrumPHP

[AlexandruGG]

@drupol I noticed the Psalm level set up in the config is 7, which is very lenient: https://psalm.dev/docs/running_psalm/error_levels/. A bit confusingly, the levels go the other way in PHPStan's case, where higher is stricter 😄 .

Was Psalm added later to project? Or how come it's level is not very strict compared to PHPStan?

[AlexandruGG]

@drupol I decided to err on the side of caution with this and so only fixed some of the things what were not very problematic. I tried diving into the issues in Collection.php, however fixing the types there leads to a lot more issues because PHPStan is then able to better infer the types in other places 😄 .

I added a baseline file so now these are clearly visible and can be tackled separately.

How do we make PHPStan+Psalm blocking in GrumPHP? Do we do it via a parameter in this project which overwrites the drup-conventions parameter? Or do we change the one in that repository? (not sure how it affects other projects of yours)

[AlexandruGG]

@drupol I noticed the Psalm level set up in the config is 7, which is very lenient: https://psalm.dev/docs/running_psalm/error_levels/. A bit confusingly, the levels go the other way in PHPStan's case, where higher is stricter 😄 .

Was Psalm added later to project? Or how come it's level is not very strict compared to PHPStan?

Trying the library in a project that uses a stricter Psalm level results in errors 😢

ERROR: InvalidArgument - tests/Util/Command/CheckCrapScore.php:120:57 - Argument 1 of loophp\collection\Collection::map expects callable(int, array-key, Iterator<array-key, mixed>):V:loophp\collection\Contract\Operation\Mapable as mixed, pure-Closure(int):int provided (see https://psalm.dev/004)

$collection = Collection::fromIterable([1, 2, 3])->map(static fn(int $value): int => $value * 2);

I scratched my head with this one a bit but still couldn't see what the issue was 😅

[drupol]

I added a baseline file so now these are clearly visible and can be tackled separately.

Excellent, I really like that!

How do we make PHPStan+Psalm blocking in GrumPHP? Do we do it via a parameter in this project which overwrites the drup-conventions parameter? Or do we change the one in that repository? (not sure how it affects other projects of yours)

Like this:

diff --git a/grumphp.yml b/grumphp.yml
index a5dba304..0bb8a40e 100644
--- a/grumphp.yml
+++ b/grumphp.yml
@@ -5,6 +5,7 @@ parameters:
   tasks.phpcsfixer.config: .php-cs-fixer.dist.php
   tasks.license.holder: Pol Dellaiera
   tasks.license.date_from: 2019
+  tasks.phpstan.blocking: true
   extra_tasks:
     phpspec:
       verbose: true

[drupol]

Trying the library in a project that uses a stricter Psalm level results in errors cry

Yeah I'm aware of this :(

ERROR: InvalidArgument - tests/Util/Command/CheckCrapScore.php:120:57 - Argument 1 of loophp\collection\Collection::map expects callable(int, array-key, Iterator<array-key, mixed>):V:loophp\collection\Contract\Operation\Mapable as mixed, pure-Closure(int):int provided (see https://psalm.dev/004)

$collection = Collection::fromIterable([1, 2, 3])->map(static fn(int $value): int => $value * 2);

I scratched my head with this one a bit but still couldn't see what the issue was sweat_smile

The callable takes 3 arguments.

• The value
• The key
• The iterator.

Try with something like this:

static fn(int $value, $key, Iterator $iterator): int => $value * 2

[AlexandruGG]

Trying the library in a project that uses a stricter Psalm level results in errors cry

Yeah I'm aware of this :(

ERROR: InvalidArgument - tests/Util/Command/CheckCrapScore.php:120:57 - Argument 1 of loophp\collection\Collection::map expects callable(int, array-key, Iterator<array-key, mixed>):V:loophp\collection\Contract\Operation\Mapable as mixed, pure-Closure(int):int provided (see https://psalm.dev/004)
$collection = Collection::fromIterable([1, 2, 3])->map(static fn(int $value): int => $value * 2);
I scratched my head with this one a bit but still couldn't see what the issue was sweat_smile

The callable takes 3 arguments.

• The value
• The key
• The iterator.

Try with something like this:

static fn(int $value, $key, Iterator $iterator): int => $value * 2

Unfortunately I don't think that's the issue as the map should be able to be used without those too. Adding them still errors:

ERROR: InvalidArgument - tests/Util/Command/CheckCrapScore.php:121:57 - Argument 1 of loophp\collection\Collection::map expects callable(int, int, Iterator<TKey:loophp\collection\Collection as array-key, T:loophp\collection\Collection as mixed>):V:loophp\collection\Contract\Operation\Mapable as mixed, pure-Closure(int, int, Iterator<TKey:loophp\collection\Collection as array-key, T:loophp\collection\Collection as mixed>):int provided (see https://psalm.dev/004)

$collection = Collection::fromIterable([1, 2, 3])->map(static fn(int $value, int $key, Iterator $iterator): int => $value * 2);

I think it has to do with the map return type instead, that part in the error message looks like it's different.

[drupol]

This is something that we need to sort out. I know that annotations are not well done, but this is a huge work. I haven't been able to find the right balance.

[AlexandruGG]

This is something that we need to sort out. I know that annotations are not well done, but this is a huge work. I haven't been able to find the right balance.

I know, it'll be hard to fix them with the code already in place because there are interdependencies.
I want to try the suggestion I made here in a future PR and adjust the Psalm level so those errors become visible.

What would be the best place to put that code? I don't think it quite fits in docs, src, spec, or tests (also the last 2 are excluded from analysis). Should we create a new folder usage?

[AlexandruGG]

This is something that we need to sort out. I know that annotations are not well done, but this is a huge work. I haven't been able to find the right balance.

I know, it'll be hard to fix them with the code already in place because there are interdependencies.
I want to try the suggestion I made here in a future PR and adjust the Psalm level so those errors become visible.

What would be the best place to put that code? I don't think it quite fits in docs, src, spec, or tests (also the last 2 are excluded from analysis). Should we create a new folder usage?

Update on this, I think a folder called static-analysis would be more descriptive. A colleague alerted me to the fact that this pattern is commonly used:

• https://github.com/moneyphp/money/tree/master/static-analysis
• https://github.com/Ocramius/ProxyManager/tree/2.12.x/tests/static-analysis

[drupol]

Yeah I did that for this: https://github.com/loophp/combinator

[AlexandruGG]

Yeah I did that for this: https://github.com/loophp/combinator

Ah nice! Yeah this library needs that too. Don't keep the best stuff for your other projects 😜 😄

[drupol]

I have a local branch of loophp/collection completely rewritten with loophp/fpt :)

But this is for later, much later!

[AlexandruGG]

Let me know if there's anything else to address on this, it's ready from my side :)

[drupol]

You're on a roll ! Thanks again !!!