LastConditionVisitor: condition followed by throw is marked as last

[JanTvrdik]

This improves LastConditionVisitor to consider as "last condition" any elseif that is followed by else with throw statement, or any last if/elseif condition that is followed throw statement.

This fixes issue, where PHPStan reports error when safety checks are used. Removing the else branch leads to less readable and more dangerous code, rewriting to match is not always possible / practical.

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) { // this is currently reported as error
  echo "blue\n";

} else {
  throw new LogicException('Unexpected color');
}

Removing else branch leads to code, that will silently fail when new color is added.

Besided enums, we are also encountering this pattern when trying to use sealed classes

[phpstan-bot]

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

[ondrejmirtes]

The problem here is that for this code:

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) { // this is currently reported as error
  echo "blue\n";

} else {
  throw new LogicException('Unexpected color');
}

PHPStan will not report an error even when Color enum has more than Red and Blue cases.

What I want developers to do to rewrite this to a match expression:

echo match ($color) {
	Color::Red => 'red',
	Color::Blue => 'blue',
};

Which solves all of the problems.

I know it's not obvious to go from if-elseif-else to match expression and that not everyone will figure it out, but that's maybe we can do instead - to help people figure out that match is better (with an error tip?)

[ondrejmirtes]

rewriting to match is not always possible / practical

Match expression leads to safety through static analysis. Why is it not always possible and practical?

[JanTvrdik]

PHPStan will not report an error even when Color enum has more than Red and Blue cases.

Which is still an improvement over current solution. Failing at runtime with exception is preferred over failing at runtime silently.

But of course, detecting the issue with PHPStan would be even better. The classic type safe approach that I proposed to support few weeks ago, was not accepted. And would not be compatible with this fix anyway.

New idea I have would be to have dedicated "dead code exception", which PHPStan would require to be thrown only from dead code.

So this would be fine.

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) {
  echo "blue\n";

} else {
  // no error, because this is dead code
  throw new SpecialDeadCodeOnlyException('Unexpected color');
}

But this would PHPStan report as error, because SpecialDeadCodeOnlyException is thrown from not-dead-code.

if ($color === Color::Red) {
  echo "red\n";

} else {
  // error: SpecialDeadCodeOnlyException can be only thrown from dead code
  throw new SpecialDeadCodeOnlyException('Unexpected color');
}

Why is it not always possible and practical?

Mostly because it does not support statements.

1. Extracting simple statements such as echo "red\n"; to method is not practical, you end up with a lot more code, leading to less readable code.
2. The same when the body contains multiple assignments. You would have to use tuples and/or create DTO. Again more code, more bugs, less readable.

[ondrejmirtes]

Mostly because it does not support statements.

Yeah this would be a nice addition to the PHP language, however you can already do this today:

match ($color) {
	Color::Red => (function () {
		echo 'red';
	})(),
	Color::Blue => (function () {
		echo 'blue';
	})(),
};

Someone might prefer IIFE and someone might prefer "dead code exception".

The easiest way to implement "dead code exception" rule is to pass $color into it so that you have some expression to compare for NeverType. Otherwise you have no way to ask for "is this scope dead?".

[ondrejmirtes]

So what I'd like to do:

1. Nudge the user into the right direction (rewrite to match) when they encounter this:

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) { // this is currently reported as error
  echo "blue\n";

} else {
  throw new LogicException('Unexpected color');
}

The code in this PR might be useful for this. We could tell the user to rewrite it as match, or to use something we invent in 2):

2. Come up with a useful standard way for PHPStan to assert "this code should be dead" that's also OK for production code. Custom exception + custom rule is a nice idea but there could be some kind of (probably) PHPDoc to achieve the same thing with less effort.

[JanTvrdik]

Someone might prefer IIFE and someone might prefer "dead code exception".

That's why both should work. I would be shocked if more people prefer IIFE over "dead code exception". And again it will work very poorly, when you have multiple/inconsistent variable assignments. You have tu use tuples/dto/variable passed by reference.

Compare

$blueFound = false;
$redFound = false;

foreach ($colors as $color) {
  if ($color === Color::Red) {
    $redFound = true;
  } elseif ($color === Color::Blue) {
    $blueFound = true;
  } else {
    throw new DeadCodeException();
  }
}

with

$blueFound = false;
$redFound = false;

foreach ($colors as $color) {
  match ($color) {
    Color::Red => (function () use (&$redFound): void {
      $redFound = true;
    })(),
    Color::Blue => (function () use (&$blueFound): void {
      $blueFound = true;
    })(),
  };
}

Otherwise you have no way to ask for "is this scope dead?".

I thought this was already implemented. Anyway, you iterate over declared variables in scope and if any of them has type never, then the scope is dead. In general assigning never to any variable should terminate the scope. In more general evaluating any expression to never should terminate the scope.

Nudge the user into the right direction (rewrite to match) when they encounter this:

I disagree with that being the right direction. It could be a valid code style choice, but PHPStan is not a code style checker.

[ondrejmirtes]

Anyway, you iterate over declared variables in scope and if any of them has type never, then the scope is dead.

A scope can also be dead without any variables involved, so I'd rather rely on this:

phpstan-src/src/Analyser/NodeScopeResolver.php

Lines 790 to 794 in 19c838b

	if (
	$elseIfConditionType->isTrue()->yes()
	) {
	$lastElseIfConditionIsTrue = true;
	}

But if we did something like this:

foreach ($colors as $color) {
  if ($color === Color::Red) {
    $redFound = true;
  } elseif ($color === Color::Blue) {
    $blueFound = true;
  } else {
    /** @phpstan-var-assert never $color */
    throw new DeadCodeException();
  }
}

Then the code would look at the type of $color and it wouldn't be a problem.

I disagree with that being the right direction. It could be a valid code style choice, but PHPStan is not a code style checker.

I get it, but the issue is here "how to correctly get rid of this 'always true' error by making code better" so we can tell the user what to do according to our opinion.

[JanTvrdik]

With the introduction of NonAcceptingNeverType, the code can now be improved to:

PLAYGROUND

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) { // this is currently reported as error
  echo "blue\n";

} else {
  throw new NeverException($color);
}

// ---

final class NeverException extends LogicException
{

    /**
     * @param never $value
     */
    public function __construct(mixed $value)
    {
        $message = 'Type of value should be never, but got ' . get_debug_type($value);
        parent::__construct($message);
    }

}

This allows PHPStan to report error when enum case is added that the code does not handle. This should address the concern that was blocking the merge.

[ondrejmirtes]

What would be great is to differentiate between:

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) {
  echo "blue\n";

} else {
  throw new NeverException($color); // safe code - no error should be reported with the last elseif
}

and:

if ($color === Color::Red) {
  echo "red\n";

} elseif ($color === Color::Blue) {
  echo "blue\n";

} else {
  throw new LogicException; // unsafe code - "never" is not enforced - last elseif should still be reported
}

WDYT?

[janedbal]

What would be great is to differentiate between

I'd say it would be great, but even current state greatly improves usability. I'd consider this a next step. Can we merge this as-is and possibly focus on this (imo hard) part in separate MR?

[ondrejmirtes]

I'd say it would be great, but even current state greatly improves usability

Yes, agreed :)

[ondrejmirtes]

Thank you!

[janedbal]

Thank you!