Skip to content

Commit

Permalink
DateTime and DateTimeImmutable constructor does not always throw Exce…
Browse files Browse the repository at this point in the history
…ption
  • Loading branch information
ondrejmirtes committed May 5, 2021
1 parent 40d93a1 commit 181f75c
Show file tree
Hide file tree
Showing 8 changed files with 128 additions and 13 deletions.
2 changes: 0 additions & 2 deletions build/phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ parameters:
- ../tests/PHPStan/Command/IgnoredRegexValidatorTest.php
- ../src/Command/IgnoredRegexValidator.php
exceptions:
uncheckedExceptionRegexes:
- '~^Exception$~'
uncheckedExceptionClasses:
- 'PHPStan\ShouldNotHappenException'
- 'Symfony\Component\Console\Exception\InvalidArgumentException'
Expand Down
5 changes: 5 additions & 0 deletions conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,11 @@ services:
tags:
- phpstan.broker.dynamicFunctionReturnTypeExtension

-
class: PHPStan\Type\Php\DateTimeConstructorThrowTypeExtension
tags:
- phpstan.dynamicStaticMethodThrowTypeExtension

-
class: PHPStan\Type\Php\DsMapDynamicReturnTypeExtension
tags:
Expand Down
45 changes: 36 additions & 9 deletions src/Analyser/NodeScopeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -2458,15 +2458,9 @@ static function () use ($expr, $rightResult): MutatingScope {
$expr->args,
$constructorReflection->getVariants()
);
if ($constructorReflection->getThrowType() !== null) {
$throwType = $constructorReflection->getThrowType();
if (!$throwType instanceof VoidType) {
$throwPoints[] = ThrowPoint::createExplicit($scope, $throwType, $expr, true);
}
} elseif ($this->implicitThrows) {
if ($classReflection->getName() !== \Throwable::class && !$classReflection->isSubclassOf(\Throwable::class)) {
$throwPoints[] = ThrowPoint::createImplicit($scope, $expr);
}
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $classReflection, $expr, $expr->class, $expr->args, $scope);
if ($constructorThrowPoint !== null) {
$throwPoints[] = $constructorThrowPoint;
}
}
} else {
Expand Down Expand Up @@ -2720,6 +2714,39 @@ private function getMethodThrowPoint(MethodReflection $methodReflection, MethodC
return null;
}

/**
* @param Node\Arg[] $args
*/
private function getConstructorThrowPoint(MethodReflection $constructorReflection, ClassReflection $classReflection, New_ $new, Name $className, array $args, MutatingScope $scope): ?ThrowPoint
{
$methodCall = new StaticCall($className, $constructorReflection->getName(), $args);
foreach ($this->dynamicThrowTypeExtensionProvider->getDynamicStaticMethodThrowTypeExtensions() as $extension) {
if (!$extension->isStaticMethodSupported($constructorReflection)) {
continue;
}

$throwType = $extension->getThrowTypeFromStaticMethodCall($constructorReflection, $methodCall, $scope);
if ($throwType === null) {
return null;
}

return ThrowPoint::createExplicit($scope, $throwType, $new, false);
}

if ($constructorReflection->getThrowType() !== null) {
$throwType = $constructorReflection->getThrowType();
if (!$throwType instanceof VoidType) {
return ThrowPoint::createExplicit($scope, $throwType, $new, true);
}
} elseif ($this->implicitThrows) {
if ($classReflection->getName() !== \Throwable::class && !$classReflection->isSubclassOf(\Throwable::class)) {
return ThrowPoint::createImplicit($scope, $methodCall);
}
}

return null;
}

private function getStaticMethodThrowPoint(MethodReflection $methodReflection, StaticCall $methodCall, MutatingScope $scope): ?ThrowPoint
{
foreach ($this->dynamicThrowTypeExtensionProvider->getDynamicStaticMethodThrowTypeExtensions() as $extension) {
Expand Down
2 changes: 1 addition & 1 deletion src/Command/FixerApplication.php
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ private function downloadPhar(
);

/** @var array{url: string, version: string} $latestInfo */
$latestInfo = Json::decode((string) await($client->get('https://fixer-download-api.phpstan.com/latest'), $loop, 5.0)->getBody(), Json::FORCE_ARRAY);
$latestInfo = Json::decode((string) await($client->get('https://fixer-download-api.phpstan.com/latest'), $loop, 5.0)->getBody(), Json::FORCE_ARRAY); // @phpstan-ignore-line
if ($currentVersion !== null && $latestInfo['version'] === $currentVersion) {
$this->writeInfoFile($infoPath, $latestInfo['version']);
$output->writeln('<fg=green>You\'re running the latest PHPStan Pro!</>');
Expand Down
45 changes: 45 additions & 0 deletions src/Type/Php/DateTimeConstructorThrowTypeExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php declare(strict_types = 1);

namespace PHPStan\Type\Php;

use DateTime;
use DateTimeImmutable;
use PhpParser\Node\Expr\StaticCall;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Type\DynamicStaticMethodThrowTypeExtension;
use PHPStan\Type\Type;
use PHPStan\Type\TypeUtils;

class DateTimeConstructorThrowTypeExtension implements DynamicStaticMethodThrowTypeExtension

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet May 10, 2021

Contributor

This class is doing the same thing than this one https://github.com/pepakriz/phpstan-exception-rules/blob/master/src/Extension/DateTimeExtension.php

Is it planned to propose all the feature of phpstan-exception-rules (which has low maintenance) directly with phpstan ? Would it be useful to open a discussion/issue about this @ondrejmirtes (for instance to list the features you're interested by) ?

This comment has been minimized.

Copy link
@ondrejmirtes

ondrejmirtes May 10, 2021

Author Member

I think that pepakriz/phpstan-exception-rules should be refactored with the new capabilities of PHPStan core. I plan to add some exception-related rules to PHPStan core, but it won't be 1:1.

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet May 10, 2021

Contributor

The pepakriz/phpstan-exception-rules is not really active.
Instead of refactoring it, it could be easier to move some of the code to PHPStan.

I assume the extension https://github.com/pepakriz/phpstan-exception-rules/tree/master/src/Extension could be written with the new capabilities of PHPStan core, so could be moved here.

I can try some PR if you're interested in.

This comment has been minimized.

Copy link
@ondrejmirtes

ondrejmirtes May 10, 2021

Author Member

Yes, I'd like these extensions to have in the core. You can probably test them as test cases in CatchWithUnthrownExceptionRuleTest. Thank you.

As for the rest of the package, I want to have my take on it, and that's already in progress, so there isn't probably anything to take from that.

This comment has been minimized.

Copy link
@VincentLanglet

VincentLanglet May 10, 2021

Contributor

Yes, I'd like these extensions to have in the core. You can probably test them as test cases in CatchWithUnthrownExceptionRuleTest. Thank you.

I'll try to do the PR then.

As for the rest of the package, I want to have my take on it, and that's already in progress, so there isn't probably anything to take from that.

Is there somewhere a list of the feature you plan to implement for exceptions ?

I'm using a lot the pepakriz/phpstan-exception-rules.
If you're interested by a user-feedback, the feature I used are:

  • The ability to ignore all the checks for some exceptions (like LogicException)
  • Each exception should try/catch or added to the phpdoc with @throws.
  • Reporting unused @throws
  • The @throws tag of a method should be compatible with the phpdoc of the interface

With the fact that a lot of repository doesn't use correctly @throws, the feature to override the @throws tag is pretty useful (and I use it a lot)

methodThrowTypeDeclarations:
            Aws\S3\S3Client:
                getObject: [Aws\Exception\AwsException]
                putObject: [Aws\Exception\AwsException]

I assume you won't introduce something similar to the Phpstan core.
Does stub already (or will) support @throws tag in order to override them ?

This comment has been minimized.

Copy link
@ondrejmirtes

ondrejmirtes May 10, 2021

Author Member

Try out these settings:

exceptions:
uncheckedExceptionRegexes: []
uncheckedExceptionClasses: []
check:
missingCheckedExceptionInThrows: false
tooWideThrowType: false

Like this:

exceptions:
uncheckedExceptionClasses:
- 'PHPStan\ShouldNotHappenException'
- 'Symfony\Component\Console\Exception\InvalidArgumentException'
- 'PHPStan\BetterReflection\SourceLocator\Exception\InvalidFileLocation'
- 'PHPStan\BetterReflection\SourceLocator\Exception\InvalidArgumentException'
- 'Symfony\Component\Finder\Exception\DirectoryNotFoundException'
- 'InvalidArgumentException'
- 'PHPStan\DependencyInjection\ParameterNotFoundException'
- 'PHPStan\Analyser\UndefinedVariableException'
- 'RuntimeException'
- 'Nette\Neon\Exception'
- 'Nette\Utils\JsonException'
- 'PHPStan\File\CouldNotReadFileException'
- 'PHPStan\File\CouldNotWriteFileException'
- 'PHPStan\Parser\ParserErrorsException'
- 'ReflectionException'
- 'Nette\Utils\AssertionException'
- 'PHPStan\File\PathNotFoundException'
- 'PHPStan\Broker\ClassNotFoundException'
- 'PHPStan\Broker\FunctionNotFoundException'
- 'PHPStan\Broker\ConstantNotFoundException'
- 'PHPStan\Reflection\MissingMethodFromReflectionException'
- 'PHPStan\Reflection\MissingPropertyFromReflectionException'
- 'PHPStan\Reflection\MissingConstantFromReflectionException'
- 'PHPStan\Type\CircularTypeAliasDefinitionException'
- 'PHPStan\Broker\ClassAutoloadingException'
- 'LogicException'
- 'TypeError'
check:
missingCheckedExceptionInThrows: true
tooWideThrowType: true

Yes, overriding PHPDocs in stub files works for @throws too.

{

public function isStaticMethodSupported(MethodReflection $methodReflection): bool
{
return $methodReflection->getName() === '__construct' && in_array($methodReflection->getDeclaringClass()->getName(), [DateTime::class, DateTimeImmutable::class], true);
}

public function getThrowTypeFromStaticMethodCall(MethodReflection $methodReflection, StaticCall $methodCall, Scope $scope): ?Type
{
if (count($methodCall->args) === 0) {
return null;
}

$arg = $methodCall->args[0]->value;
$constantStrings = TypeUtils::getConstantStrings($scope->getType($arg));
if (count($constantStrings) === 0) {
return $methodReflection->getThrowType();
}

foreach ($constantStrings as $constantString) {
try {
new \DateTime($constantString->getValue());
} catch (\Exception $e) { // phpcs:ignore
return $methodReflection->getThrowType();
}
}

return null;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public function getTypeFromMethodCall(MethodReflection $methodReflection, Method

$argType = $scope->getType($methodCall->args[0]->value);

$xmlElement = new \SimpleXMLElement('<foo />');
$xmlElement = new \SimpleXMLElement('<foo />'); // @phpstan-ignore-line

foreach (TypeUtils::getConstantStrings($argType) as $constantString) {
$result = @$xmlElement->xpath($constantString->getValue());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ public function testRule(): void
'Dead catch - Throwable is never thrown in the try block.',
119,
],
[
'Dead catch - Exception is never thrown in the try block.',
171,
],
[
'Dead catch - Exception is never thrown in the try block.',
180,
],
]);
}

Expand Down
32 changes: 32 additions & 0 deletions tests/PHPStan/Rules/Exceptions/data/unthrown-exception.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,35 @@ public function doBar()
}

}

class TestDateTime
{

public function doFoo(): void
{
try {
new \DateTime();
} catch (\Exception $e) {

}
}

public function doBar(): void
{
try {
new \DateTime('now');
} catch (\Exception $e) {

}
}

public function doBaz(string $s): void
{
try {
new \DateTime($s);
} catch (\Exception $e) {

}
}

}

0 comments on commit 181f75c

Please sign in to comment.