Update PhpStorm stubs

[VincentLanglet]

Hi @ondrejmirtes,

I tried to work on the update #3334

First I thought some DynamicMethodThrowTypeExtension was needed but it seems like it is correctly implemented since your PR aa7989f

But the issue is the fact that currently CallToMethodStatementWithoutSideEffectsRule
Does not seems to use those dynamicExtension but rely on $method->getThrowType() instead
which seems to only rely on the phpdoc of the method.

An example can be found with https://phpstan.org/r/b071a436-7779-47de-816e-d520f4d70bdd, even if the dynamicThrowTypeExtension exists (and say there is no exception with 2 params https://github.com/phpstan/phpstan-src/blob/1.12.x/src/Type/Php/DsMapDynamicMethodThrowTypeExtension.php).

I see 3 solutions:

• This is done by design, so I ignore the "error" like in this PR
• There is an available method to use or something easy to implement instead of $method->getThrowType(), and I'll be happy to update the PR with it. (but which one ?)
• It's more complex and we ignore the error until then

I'll be happy to try working on this, but so far I have trouble to see if I need to play with the ThrowPoint things or just duplicate the use of DynamicMethodThrowTypeExtension....

[ondrejmirtes]

The right solution is to take dynamic throw type extensions into account in *WithoutSideEffectsRule rules.

But we need to introduce a new virtual node that would wrap Node\Stmt\Expression node and also provide things the StatementResult provides. We could then refactor these rules to use this virtual node.

[ondrejmirtes]

Oh, we already have this virtual node: NoopExpressionNode

Try rewriting the rules by using it. The main rule that uses it is BetterNoopRule and it actually contains skips for situations that are handled by *WithoutSideEffectsRule rules. So it should be possible to rewrite *WithoutSideEffectsRule rules to use NoopExpressionNode too.

[VincentLanglet]

Oh, we already have this virtual node: NoopExpressionNode

Try rewriting the rules by using it. The main rule that uses it is BetterNoopRule and it actually contains skips for situations that are handled by *WithoutSideEffectsRule rules. So it should be possible to rewrite *WithoutSideEffectsRule rules to use NoopExpressionNode too.

Am I wrong or currently NoopExpressionNode doesn't have access to things the StatementResult provides ?

[ondrejmirtes]

Doesn't matter, this node is invoked only for expressions with zero throw points.

[ondrejmirtes]

Please also rewrite the 3 other rules as well (static call, function, constructor).

[VincentLanglet]

Please also rewrite the 3 other rules as well (static call, function, constructor).

Sure I will, but since tests are still failing, it means that PHPStan still believe there is throwPoints

[VincentLanglet]

I had to add back the check on NeverType (ff5c552) to solve two tests.

The last test was failing because PHPStan consider there is a ImpurePoint (methodCall: call to unknown method) in

$dt::createFromFormat('Y-m-d', '2019-07-24');

Which seems true, I can produce an exception with

class Foo extends \DateTimeImmutable {
	public static function createFromFormat($format, $time, ?DateTimeZone $object = NULL): DateTimeImmutable|false {
		throw new \Exception('Fail');
	}
}

$foo = new Foo();

var_dump($foo::createFromFormat('Y-m-d', '2019-07-24'));

[VincentLanglet]

Thanks a lot for your help @ondrejmirtes, it's now ready for a new review

[ondrejmirtes]

Awesome, thank you!

[VincentLanglet]

phpstan/phpstan#11503 (comment) is showing there is a (small ?) impact of the current refacto.

A reproducer would be https://phpstan.org/r/37abe2c8-b4bb-4ba6-bb7d-0eedcf703d0c

There is an impure point instantiation of class DateTimeZone, which comes from

phpstan-src/src/Analyser/NodeScopeResolver.php

Lines 3270 to 3281 in 2e27573

	if ($constructorReflection !== null) {
	if (!$constructorReflection->hasSideEffects()->no()) {
	$certain = $constructorReflection->isPure()->no();
	$impurePoints[] = new ImpurePoint(
	$scope,
	$expr,
	'new',
	sprintf('instantiation of class %s', $constructorReflection->getDeclaringClass()->getDisplayName()),
	$certain,
	);
	}
	} elseif ($classReflection === null) {

There is a DateTimeZoneConstructorThrowTypeExtension

phpstan-src/src/Type/Php/DateTimeZoneConstructorThrowTypeExtension.php

Line 17 in 2e27573

final class DateTimeZoneConstructorThrowTypeExtension implements DynamicStaticMethodThrowTypeExtension

it seems used for DeadCatch detection but not NoSideEffect detection, is there something missing or is this behavior wanted ?

Edit: I don't see DateTime::__construct(), DateTimeImmutable::__construct(), DateTimeZone::__construct(), and so on in the functionMetadata, I feel like it's related but dunno if it's done on purpose.

[ondrejmirtes]

Does new DateTimeImmutable('xyz') have the same problem or not? If not, how is that achieved? We could do the same thing for new DateTimeZone.

If it has the same problem then we should fix both.

[VincentLanglet]

Does new DateTimeImmutable('xyz') have the same problem or not?

Not sure to understand the question but I'll do my best to answer it:

new \DateTimeZone('UTC')

has

• 1 impure point instantiation of class DateTimeZone
• 0 throw point

new \DateTimeImmutable()

has

• 1 impure point instantiation of class DateTimeImmutable
• 0 throw point

new \DateTimeImmutable('xyz')

has

• 1 impure point instantiation of class DateTimeImmutable
• 1 throw point => this is expected

Which gives https://phpstan.org/r/dfe3ff19-92e9-4d6b-be05-4673be64ffec

I expect 0 impure point everywhere

[ondrejmirtes]

I say we're fine here, nothing to fix.

[VincentLanglet]

I say we're fine here, nothing to fix.

Would you mind develop a bit ?
Why a single line with

new \DateTimeImmutable();

without assigning the variable doesn't require to be reported ? (and is considering has having side effects)

I was planning on adding

'DateInterval::__construct' => ['hasSideEffects' => false],
'DatePeriod::__construct' => ['hasSideEffects' => false],
'DateTime::__construct' => ['hasSideEffects' => false],
'DateTimeImmutable::__construct' => ['hasSideEffects' => false],
'DateTimeZone::__construct' => ['hasSideEffects' => false],

in the functionMetadata.php file. (which seems to solve the issue)

[ondrejmirtes]

new \DateTimeImmutable(); is not pure, it depends on system clock and always creates a different time object.

[ondrejmirtes]

So the purity depends on how the arguments look like. And we don't have an extension point for that now.

[VincentLanglet]

Oh, I see.

I thought PHPStan had a level between purity and side effect, in a way that

• Pure imply no side effects
• But no side effects could be done in a impure function

Like https://phpstan.org/r/60731ffd-1e61-4cc0-bfdd-ec8529197b52

But seems like it's not.

So indeed, we're "fine" until such feature would be implemented.