Update Command::withProgressBar phpdoc to account for arrow functions and non-void return types

[billypoke]

In #58565 the return type declaration of many functions was updated, including Command::withProgressBar. When passing an arrow function as the second argument ($callback), the return value of the callback function is not void and will trigger a static analysis warning, as arrow functions in PHP necessarily have a non-void return type.

This would pop up if models are being updated in a loop (each save() returns bool), or jobs being dispatched (each dispatch returns PendingDispatch or similar).

Adding mixed as a union type allows callbacks with non-void return types to be passed without failing static analysis. I don't think it's feasible to try and narrow the type any more, but I'm not a static analysis wizard 🧙

[billypoke]

Something's still not quite right here. I noticed in my other PR #58768 that return types for callables should be wrapped in parentheses. This introduces a new error that I'm still working on - specifically Type Item of parameter #1 $item of passed callable needs to be same or wider than parameter type Item|Symfony\Component\Console\Helper\ProgressBar of accepting callable.

https://phpstan.org/r/d6530ed2-e432-4555-9554-44caae902d5b

[shaedrich]

Something's still not quite right here.

Item doesn't extend \Symfony\Component\Console\Helper\ProgressBar. These two have no overlap.

[billypoke]

I understand that. There is still an issue here with the type declaration in the PHPDoc. If you implement a progress bar according to the documentation with the current phpdoc or with the updated one, you will see that error. This is not dependent on passing an arrow function vs. an anonymous function.

https://phpstan.org/r/f0fc0f19-4037-4b83-bde0-0cff86e397d9

How would you write valid code without throwing an error?

[shaedrich]

Here's the example with \Symfony\Component\Console\Helper\ProgressBar: https://phpstan.org/r/e22c42b2-b2ca-4da0-afd5-5cdc9043bdf8

And it works with void, which is not wrong or a bug, just an inconvenience, that sure can be widened.

And here's your arrow function implementation: https://phpstan.org/r/9fb36e9b-9834-4376-977b-ba56e2c27cb5

[billypoke]

\Symfony\Component\Console\Helper\ProgressBar is not under my control/not my code, that's the underlying ProgressBar instance. Placing my barebones Item class in Symfony's namespace is not a valid solution in my opinion. I'm going to be honest that reads like it was generated by an AI.

In my examples, it's not required that the parameter of the callback be a class, it could be an array or other primitive, such as int. The error remains.

I don't care about using or manipulating the bar, just iterating over an array/collection of items and invoking a callback function on each one as described in the documentation. Your examples don't do that. Please show me an example of how you would run a progress bar on anything and have it not throw this error with the PHPDoc as is.

To your credit, I really appreciate the additional types being added to the codebase, and I'm not actually convinced that this function can be correctly typed by phpstan with the constraints that generics have.

[shaedrich]

\Symfony\Component\Console\Helper\ProgressBar is not under my control/not my code, that's the underlying ProgressBar instance. Placing my barebones Item class in Symfony's namespace is not a valid solution in my opinion.

It doesn't matter. It's about the type, not the value. Fact is: You produced an invalid example that doesn't reflect the actual problem. What I did is building an example reflecting your problem and applying your fix to the second example, proving that your fix is working. You should be happy that it does.

In my examples, it's not required that the parameter of the callback be a class, it could be an array or other primitive, such as int. The error remains.

The param is a progress bar, not an array. Please check your types again.

To your credit, I really appreciate the additional types being added to the codebase

Thank you 🙂

I'm not actually convinced that this function can be correctly typed by phpstan with the constraints that generics have.

I don't think, the issue is with the generics here.

[billypoke]

The param can be a progress bar, but it can also be the iterable object, as documented in the LARAVEL USAGE DOCUMENTATION FOR WITHPROGRESSBAR, and in the phpdoc you wrote! The first param is either ProgressBar|TValue! What is TValue? It's the items in the iterable! Please actually read what withProgressBar does

This is literally the most basic, default usage of the function, passing an iterable to withProgressBar

https://phpstan.org/r/7a472fc6-7b59-451e-8ae1-9e39fde4614a

Tell me how the default usage causes an error please.

Look also at the actual function body of withProgressBar:

if (is_iterable($totalSteps)) {
    foreach ($totalSteps as $key => $value) {
        $callback($value, $bar, $key);

        $bar->advance();
    }
} else {
    $callback($bar);
}

What is passed to $callback if $totalSteps is iterable, i.e. an array? That's right it's $value, which has type TValue.

[shaedrich]

Yeah, it can be TValue, but the second parameter cannot. Either the first or second param always has to be a progress bar.

Yeah, okay, I mixed the progress bar and TValue—thanks for pointing that out 👍🏻
https://phpstan.org/r/726feb3a-e124-40de-8202-8449aa2b1e35

But there's still an error. We can make the type more precise by making the whole closure a union instead of having two options for every parameter and avoid the error:
https://phpstan.org/r/ee1a174e-15a1-4ab7-abd3-c90d1759a0b1

		/**
	     * Execute a given callback while advancing a progress bar.
	     *
	     * @template TKey of array-key
	     * @template TValue
	     *
	     * @param  iterable<TKey, TValue>|int  $totalSteps
-	     * @param  \Closure(\Symfony\Component\Console\Helper\ProgressBar|TValue, \Symfony\Component\Console\Helper\ProgressBar|null, TKey|null): (mixed|void)  $callback
+	     * @param  \Closure(\Symfony\Component\Console\Helper\ProgressBar): (mixed|void)|\Closure(TValue, \Symfony\Component\Console\Helper\ProgressBar, TKey): (mixed|void)  $callback
	     * @return mixed|void
	     */
		public function withProgressBar($totalSteps, Closure $callback)

Of course, conditional param types would be even better but this seems to be as far as we can get

[billypoke]

That should work. I added some more contrived examples to your latest - https://phpstan.org/r/19ae1d8c-f6c3-4df0-bbaf-d8e1f843d39e and they all pass.

I'll extend this PR to adopt the dual closure union.

[GrahamCampbell]

mixed cannot be included in a union type, because the type simplifies to just mixed once you allow everything.

[billypoke]

mixed cannot be included in a union type, because the type simplifies to just mixed once you allow everything.

Interesting, I wasn't aware. Would reducing the return type to only mixed then be an acceptable kludge since the return value of the callback isn't actually used anywhere in the function?

Or would adding another template value TReturnType and placing that in the signature as @param \Closure(\Symfony\Component\Console\Helper\ProgressBar): TReturnType and so on as a kind of no-op work? I don't know if that's valid phpstan syntax.

Here is a new suggested phpdoc:

/**
* Execute a given callback while advancing a progress bar.
*
* @template TKey of array-key
* @template TValue
* @template TReturnType
* @template TIterable of iterable<TKey, TValue>
*
* @param  TIterable|int  $totalSteps
* @param  \Closure(\Symfony\Component\Console\Helper\ProgressBar): TReturnType|\Closure(TValue, \Symfony\Component\Console\Helper\ProgressBar, TKey): TReturnType  $callback
* @return ($totalSteps is iterable ? TIterable : void)
*/

I added some more examples/tests, including using the return value in the calling function and phpstan correctly identifies that the output array is exactly the input array, or if a non-iterable is passed, that the return value is void - https://phpstan.org/r/8fb06957-9f7c-4488-8e23-3c185810191d

[shaedrich]

TReturnType doesn't really do anything because it wouldn't be reused. It would only make sense if it was returned from withProgressBar, which it is not.

[billypoke]

What benefit does having a concrete type there provide? The return value/type of the callback isn't used or checked anywhere in the function, so there are no constraints on what the return type should be.

That's why I called it a no-op in my last comment. It's there to satisfy that the callback should specify its return type, even if it isn't used or checked anywhere

[treyssatvincent]

In my opinion, having a PHPDoc triggering static analysis errors when using example from the documentation should be considered a bug.