Collection::isEmpty and isNotEmpty can not prove non-nullability

[spawnia]

This reverts parts of #51998 that are wrong.

Those annotations are wrong if the first element of a Collection is null. The following code currently passes PHPStan analysis, but errors when run:

<?php declare(strict_types=1);

namespace App\Console\Commands;

use Illuminate\Console\Command;
use Illuminate\Support\Collection;

final class CollectionError extends Command
{
    protected $signature = 'collection-error';

    public function handle(): void
    {
        $collection = new Collection([null, 2]);

        try {
            $this->empty($collection);
        } catch (\Throwable $e) {
            $this->error($e->getMessage());
        }

        try {
            $this->notEmpty($collection);
        } catch (\Throwable $e) {
            $this->error($e->getMessage());
        }
    }

    /** @param Collection<array-key, int|null> $collection */
    private function empty(Collection $collection): int
    {
        if ($collection->isEmpty()) {
            return 0;
        }

        return $collection->first();
    }

    /** @param Collection<array-key, int|null> $collection */
    private function notEmpty(Collection $collection): int
    {
        if ($collection->isNotEmpty()) {
            return $collection->first();
        }

        return 0;
    }
}

$ php artisan collection-error
App\Console\Commands\CollectionError::empty(): Return value must be of type int, null returned
App\Console\Commands\CollectionError::notEmpty(): Return value must be of type int, null returned

Without the annotations, PHPStan will rightfully complain that first() can return null.

[johanrosenson]

You are correct, but I would argue that the current annotations are better than no annotations.

Having null values in a collection could be seen as an edge case considering how Collections are mostly used.

The annotations should be improved instead of removed, maybe TFirstDefault can be used instead of null in the annotations i added?

The only reason I can see of having null values in a Collection is when you are using it as a key value storage/lookup, and in that case you would not be using isEmpty anyway, you would be relying on has instead.

Having phpstan be able to understand isEmpty/isNotEmpty is very important.

[spawnia]

The current annotations hinder the ability of PHPStan to discover potential bugs. The assumption that all users of Laravel generally do not have null in their Collections seems very wild to me.

There might be a better solution, but it has not been found yet. Until then, I propose to remove this code that is actively harmful and invites errors.

[johanrosenson]

The current annotations hinder the ability of PHPStan to discover potential bugs.

I respectfully disagree. Do you actually use Collections like in your example?

The assumption that all users of Laravel generally do not have null in their Collections seems very wild to me.

This is not my assumption though. My assertion is that in the use cases where you do have null in your Collection isEmpty/isNotEmpty is generally not very useful anyway, has will be the method you need in those cases.

Until then, I propose to remove this code that is actively harmful and invites errors.

Without the annotations phpstan gives false errors and forces you to litter your code with @phpstan-ignore to hide them, or rewrite your code to null check things you know are not null just to please phpstan.

If you have a Collection that looks like your example you will never do isEmpty and then just use the return of first() (or similar methods) — with or without the current annotations.

Generics for Collections need to be improved in general, the current annotations are step in the right direction, but maybe not the end.

I do agree however that the current annotations does lie a little when it says the return type can't be null, but I believe that this is better than how it was before, and the current annotations are wrong in way less occurrences than without the annotations (meaning the current version will be correct more often then the old version).

[spawnia]

I can see how it is generally good practice to apply something like filter() before checking isEmpty() or isNotEmpty(), and I am generally not passing around Collections that contain null. However, I would not expect every developer to do this - or even trust myself to do it correctly 100% of the time. That is why we have tools like PHPStan, to enforce correctness and to avoid silly errors.

What I would personally do is to avoid relying on indirect checks such as isEmpty() or isNotEmpty() before first(). Instead, I would call first(), store the result in a variable and check that directly.

-if ($items->isNotEmpty()) {
-    return $items->first();
+$firstItem = $items->first();
+if ($firstItem) {
+    return $firstItem;

Practicality and pragmatism are certainly important, but I am still convinced that it is preferrable to err on the side of caution when things are not certain. I would rather do an extra check, or perhaps write a seemingly redundant @phpstan-ignore in some cases, rather than have my code pass static analysis but then crash at runtime.

Parts of your original pull request are still left, and they definitely are an improvement. I do appreciate the effort to enable better static analysis with fewer false-positives, I just don't think it should come at the expense of safety and accuracy.

[johanrosenson]

What I would personally do is to avoid relying on indirect checks such as isEmpty() or isNotEmpty() before first(). Instead, I would call first(), store the result in a variable and check that directly.

If you have a Collection that you know may contain null (if it's used as a key value store for instance, but then you would never use first anyway, you would use has/get), then I 100% agree with this, but if you have a Collection containing models or similar (which is a extremely common usage) it's just cumbersome and unnecessary to assign the result of first() to a variable to check for null before using it when you have isEmpty()/isNotEmpty().

Parts of your original pull request are still left, and they definitely are an improvement. I do appreciate the effort to enable better static analysis with fewer false-positives, I just don't think it should come at the expense of safety and accuracy.

What's left doesn't really solve anything anymore though, now it's only possible to determine if first() is guaranteed to be null, so isEmpty() would be useless for determining that first() will be an instance of what you put into the Collection, whats left is @phpstan-ignore or $var = ->first() (which I argue is an unnecessary extra step if you know that your Collection only contains instances of models or similar, which must be agreed is a very common usage of Collection).

I would be more inclined to agree to this change in the future when (if) someone actually has a real world issue with the current annotations, for now I think they solve way more than they hinder, and removing them would be a mistake.

[spawnia]

I think i found a better solution and opened a new pull request for it: #52184.