[11.x] feat: refine return type for throw_if and throw_unless to reflect actual behavior with "falsey" values

[crishoj]

In #53005, the return types for throw_if and throw_unless were narrowed to specify TValue or never depending on $condition being exactly true or not:

 * @return ($condition is true ? never : TValue)

This doesn't reflect the actual behavior of throw_if and throw_unless: Both check for "falseyness", and not strict equality to true.

Crucially, the new type annotation gets in the way when e.g. throw_unless is used to check for the presence of something, e.g.:

throw_unless($something, "Something required");

To keep phpstan happy, developers would have to rewrite this to e.g.:

throw_unless($something !== null, "Something required");

Luckily, phpstan offers the non-empty-mixed "advanced type", allowing us to precisely specify the behavior of throw_if and throw_unless:

 * @return ($condition is non-empty-mixed ? never : TValue)

[calebdw]

The entire point of the referenced PR was to narrow the type after the function call, this PR removed support for narrowing the types and had to rewrite the tests because they were failing.

This now fails:

    throw_unless(is_int($foo));
    assertType('int', $foo);

[crishoj]

@calebdw Apologies, the loss of type narrowing was unintentional.

Trying to achieve both type-narrowing and compatibility with falsey values, I got this annotation for throw_if that works with your original type-narrowing tests:

     * @return ($condition is true ? never : ($condition is non-empty-mixed ? never : TValue))

But I'm struggling to find a similar annotation for throw_unless that addresses both needs...

[calebdw]

I would assume something like this would work:

@return ($condition is true ? TValue  :  ($condition is non-empty-mixed ? TValue : never))

[crishoj]

That version gives:

 ------ --------------------------------------
  Line   Support/Helpers.php
 ------ --------------------------------------
  64     Expected type int, actual: float|int
  66     Expected type 0, actual: float|int
 ------ --------------------------------------

This annotation seems to work:

     * @return ($condition is false ? never : ($condition is non-empty-mixed ? TValue : never))

If you agree, I will open a new pull request with changed annotations and your original type-narrowing tests.

[calebdw]

Ah, yeah that makes sense.

I'm fine with these changes if the tests I originally had continue to pass.

Also, you added a bunch of assertions on the return type, but you can also add some tests to see if your falsy stuff is narrowed:

/** @var Model|null $model */
throw_unless($model);
assertType('Model', $model);

[crishoj]

@calebdw Good call.

I can't get type-narrowing to work for throw_unless() and an argument of type Carbon|null:

function testThrowUnlessWithFalseyValue(Carbon\Carbon|null $date): void
{
    throw_unless($date);
    assertType('Carbon', $date); // → ⚠️ Expected type Carbon, actual: Carbon\Carbon|null
}

@calebdw Can you tell what I'm missing?
11.x...crishoj:framework:restore-type-narrowing

[calebdw]

Maybe try using empty?

* @return ($condition is false ? never : ($condition is empty ? never : TValue))

However, I'm not the most concerned with supporting this case---as long as the strict true/false allows narrowing types then this could be solved with any number of expressions:

function test(Carbon\Carbon|null $date): void
{
    throw_if($date === null);
    throw_if(is_null($date));
    throw_unless($date !== null);
    throw_unless(! is_null($date));
    throw_unless($date instanceof Carbon);
    // etc.
    assertType('Carbon', $date);
}

[jackwh]

Since this was merged PhpStorm (storm, not stan) has started reporting every statement following throw_if / throw_unless as being unreachable. It seems to only be using the never return type, even though the documentation popup shows it knows about the rest.

As far as I'm aware I have everything set up correctly. Phpstan is enabled and configured in PhpStorm, it's not detecting any errors, I'm up to date on everything... It just seems like, for some reason, PhpStorm isn't accepting the conditional return types. Supposedly it can, but I don't know if it's a bug, or something I'm doing wrong.

Anyone else, or is it something at my end? Thanks! 🙏

[calebdw]

It might be having issues with the non-empty-mixed type? I'm not sure, I use Neovim (btw 🙃) and it works fine. I would try opening an issue with the PHPStorm folks since this seems like an issue on their end

[jackwh]

It looks like it's a PhpStorm bug after all 🤦‍♂️

WI-78351 Type not correctly inferred from phpstan/psalm conditional generic return type