Correct support for PHP 7 (and above) `assert`

[nagmat84]

Bug report

PHPStan version: 1.7.12

Since PHP 7, assert is a language construct (see PHP Doc: assert - Expectations) and not an ordinary function call anymore. In particular, it gets optimized away, if assertions are disabled via php.ini. I often use assert(false, new \AssertionError('unexpected exception', $e->code, $e)) to get annoying and never thrown exceptions out of the way (detailed example below). PhpStan reports the following error

Call to function assert() with false and AssertionError will always evaluate to false

IMHO, this error message is a false positive, because since PHP 7 assert is not a (ordinary) function call and and does not "always" evaluate to false, but throw an exception. This error message might have been correct for PHP prior to version 7, but our project only targets PHP 8.

This message is frequently accompanied by the error message

Method some-method should return some-type but return statement is missing.

This error message is a logical consequence, if one assumes that the previous assert continues with the normal program flow.

Code snippet that reproduces the problem

This example is available here: https://phpstan.org/r/2ec8cd91-d089-4859-8b16-457cd36abe2f

I typically use this construction with 3rd-party library code which I cannot change and which throws \InvalidArgumentException if called with the wrong type of argument. This is 3rd-party code which does not use proper type hinting, but checks types at runtime. Let's assume that the 3rd-party code looks like this

/**
 * @param string $name 
 * @param int $age
 * @return void
 * @throws MeaningFullException
 * @throws \InvalidArgumentException thrown if $name is not a string or $age not an integer
 */
function thirdPartyCodeWhichCannotBeChanged($name, $age) {
  if (!is_string($name) || !is_int($age)) {
    throw new \InvalidArgumentException();
  }
  // do whatever the function is supposed to do
}

I don't want to deal with the unspecific exception \InvalidArgumentException on the higher level of the call stack nor do I want code inspection errors due to unhandled or uncaught exceptions. However, I can guarantee from the way how my own code calls the 3rd-party function that the exception is never thrown. Typically, my own code follows this pattern:

/**
 * @param string $firstName 
 * @param string $lastName 
 * @param \DateTimeImmutable $birthday
 * @return int
 * @throws MeaningFullException
 */
function myOwnFunction(string $firstName, string $lastName, \DateTimeImmutable $birthday): int {
  try {
    $name = $firstName . ' ' . $lastName;
    $age = (int) $birthday->diff(new \DateTime("now"))->format('y');
    thirdPartyCodeWhichCannotBeChanged($name, $age);

    return 42;
  } catch(\InvalidArgumentException $e) {
    assert(false, new \AssertionError('unexpected exception', $e->code, $e));
  }
}

Here, I use assert to "swallow" the superfluous exception. This yields three advantages:

• The superfluous exception is moved out of the way locally. In particular, I do not see another inspection error that \InvalidArgumentException is neither caught, documented as re-thrown nor handled by the upper layers.
• In development mode (i.e. if assert is not optimized away), \AssertionError is thrown and the backtrace points to the faulty line within my own function, i.e. the line with the assert. This gives me the chance to debug in case I did something wrong unintentionally (i.e. made a wrong assumption about the exception).
• In productive mode, the code is optimized away.

Expected output

I would have expected PHPStan to behave the same way as my IDE (PhpStorm) analyzes the code, which means not to report any error. IMHO PHPStan should know,

• that assert(false, new \AssertionError('unexpected exception', $e->code, $e)) throws an exception (if enabled) and hence should not report an issue for a missing return
• that this is nothing which is normally supposed to happen and hence not report an unhandled exception
• that assert(false, ...) is not an ordinary function call which returns false

Did PHPStan help you today? Did it make you happy in any way?

Currently, we are enabling PHPStan for Lychee as part of this PR LycheeOrg/Lychee#1336. While we are struggling with a lot of false positives due to the used framework, it already helped us to discover a lot of bugs which were dormant in less frequently used features of Lychee.

[mergeable]

This bug report is missing a link to reproduction at phpstan.org/try.

It will most likely be closed after manual review.

[nagmat84]

Please note, that I added a live example: https://phpstan.org/r/2ec8cd91-d089-4859-8b16-457cd36abe2f

[ondrejmirtes]

Hi, I don't understand your code sample:

  		try {
			$name = $firstName . ' ' . $lastName;
			$age = (int) $birthday->diff(new \DateTime("now"))->format('y');
			$this->thirdPartyCodeWhichCannotBeChanged($name, $age);

			return 42;
		} catch(\InvalidArgumentException $e) {
			assert(false, new \AssertionError('unexpected exception', $e->getCode(), $e));
		}

Because this:

		} catch(\InvalidArgumentException $e) {
			assert(false, new \AssertionError('unexpected exception', $e->getCode(), $e));
		}

is essentially this in your case:

		} catch(\InvalidArgumentException $e) {
			if (!$production) {
				throw new new \AssertionError('unexpected exception', $e->getCode(), $e);
			}
		}

Which means that if we're not in production and InvalidArgument is going to be thrown, the catch block isn't going to do anything and "return statement is missing" is the correct error to report.

If InvalidArgumentException is never going to be thrown, you don't need to write a catch block for it, which means the first example is equivalent to this:

		$name = $firstName . ' ' . $lastName;
		$age = (int) $birthday->diff(new \DateTime("now"))->format('y');
		$this->thirdPartyCodeWhichCannotBeChanged($name, $age);

		return 42;

You're saying you're trying to suppress some inspections with the esoteric assert(false, new \AssertionError('unexpected exception', $e->getCode(), $e)); line, but that just confuses PHPStan. It's better to disable such an inspection. For example PHPStan's optional exception checks have way to mark exceptions as checked and unchecked. Unchecked exception (InvalidArgumentException in your case) is not require to be caught. See https://phpstan.org/blog/bring-your-exceptions-under-control for more details.

[nagmat84]

You are right. I found a better solution for our code. See this comment LycheeOrg/Lychee#1336 (comment) for the long story.

[ondrejmirtes]

Good 😊

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.