Skip to content

Commit 1dd6b84

Browse files
JakeQZJakeQZ
authored
[TASK] Add PHPStan extension to allow assertions (#1594)
Suppress warnings for `assert()`s that, according to the DocBlock, can never fail, but in reality could, due to programmer error, since the DocBlock types are not enforced by PHP. This allows us to restore `treatPhpDocTypesAsCertain:true` for stricter static analysis, whilst also using assertions for runtime checks in a development environment, without cluttering up the code with `@phpstan-ignore` comments. The change parallels MyIntervals/emogrifier#1624. It may be possible in future to move the extension to a separate package.
1 parent f32498a commit 1dd6b84

10 files changed

Lines changed: 242 additions & 1 deletion

‎.gitattributes‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,5 @@
88
/CONTRIBUTING.md export-ignore
99
/bin/ export-ignore
1010
/docs/ export-ignore
11+
/src/PhpStan/ export-ignore
1112
/tests/ export-ignore

‎Build/phpstan/phpstan.neon‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ parameters:
1111
- %currentWorkingDirectory%/src/
1212
- %currentWorkingDirectory%/tests/
1313

14-
treatPhpDocTypesAsCertain: false
14+
excludePaths:
15+
- %currentWorkingDirectory%/tests/fixtures/phpstan/
1516

1617
type_perfect:
1718
no_mixed_property: true
@@ -24,3 +25,9 @@ parameters:
2425
-
2526
message: '#^Call to static method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) .* will always evaluate to#'
2627
path: '%currentWorkingDirectory%/tests/'
28+
29+
services:
30+
-
31+
class: \Sabberworm\CSS\PhpStan\IgnoreBooleanAlways
32+
tags:
33+
- phpstan.ignoreErrorExtension

‎src/PhpStan/IgnoreBooleanAlways.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sabberworm\CSS\PhpStan;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Expr\FuncCall;
9+
use PhpParser\Node\Name;
10+
use PHPStan\Analyser\Error;
11+
use PHPStan\Analyser\IgnoreErrorExtension;
12+
use PHPStan\Analyser\Scope;
13+
14+
/**
15+
* Ignore PHPStan warnings where the DocBlocks indicate that a conditional expression would always be true (or false),
16+
* but a programming mistake elsewhere could lead to that not being the case, for the following:
17+
* - `assert($object instanceof Class);`.
18+
*
19+
* @internal
20+
*/
21+
final class IgnoreBooleanAlways implements IgnoreErrorExtension
22+
{
23+
public function shouldIgnore(Error $error, Node $node, Scope $scope): bool
24+
{
25+
$shouldIgnore = false;
26+
27+
switch ($error->getIdentifier()) {
28+
case 'function.alreadyNarrowedType':
29+
// For an `assert()` that the DocBlocks say cannot fail.
30+
if ($node instanceof FuncCall) {
31+
$nameNode = $node->name;
32+
if ($nameNode instanceof Name && $nameNode->name === 'assert') {
33+
$shouldIgnore = true;
34+
}
35+
}
36+
break;
37+
case 'instanceof.alwaysTrue':
38+
// For `instanceof` within an `assert()` that the DocBlocks say cannot fail.
39+
$functionCallStack = $scope->getFunctionCallStack();
40+
if (isset($functionCallStack[0]) && $functionCallStack[0]->getName() === 'assert') {
41+
$shouldIgnore = true;
42+
}
43+
break;
44+
}
45+
46+
return $shouldIgnore;
47+
}
48+
}

‎tests/Unit/PhpStan/IgnoreBooleanAlwaysImpossibleAssertTest.php‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sabberworm\CSS\Tests\Unit\PhpStan;
6+
7+
use PHPStan\Analyser\IgnoreErrorExtension;
8+
use PHPStan\Testing\RuleTestCase;
9+
use PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRule;
10+
use PHPStan\Rules\Rule;
11+
12+
/**
13+
* This covers `function.alreadyNarrowedType` error handling in the `IgnoreBooleanAlways` PHPStan extension class.
14+
* `RuleTestCase` allows only one `Rule` class, so separate `TestCase`s are needed for errors generated by other rules.
15+
* Note that the base class covers testing the suppression of the warning, and is included via `extends`.
16+
* This class covers the non-suppression of warnings in other contexts, which requires detailing the expected warnings.
17+
*
18+
* @covers \Sabberworm\CSS\PhpStan\IgnoreBooleanAlways
19+
*/
20+
final class IgnoreBooleanAlwaysImpossibleAssertTest extends IgnoreBooleanAlwaysTestBase
21+
{
22+
/**
23+
* @return ImpossibleCheckTypeFunctionCallRule
24+
*/
25+
protected function getRule(): Rule
26+
{
27+
// If the class is renamed or removed in the next major release of PHPStan, we'll deal with it then.
28+
// @phpstan-ignore phpstanApi.classConstant
29+
return self::getContainer()->getByType(ImpossibleCheckTypeFunctionCallRule::class);
30+
}
31+
32+
/**
33+
* @test
34+
*/
35+
public function warningIsRetainedForPointlessAssert(): void
36+
{
37+
// Skip the test in PHP/PHPStan configurations that don't have the required components.
38+
// It is good enough to test for those that do.
39+
if (!\interface_exists(IgnoreErrorExtension::class)) {
40+
self::markTestSkipped('This is testing the testers, and only needs to run whenever possible.');
41+
}
42+
43+
// Second argument is array of expected warnings.
44+
$this->analyse(
45+
[self::FIXTURES_DIR . 'alwaystrue-pointlessassert.php'],
46+
[
47+
[
48+
'Call to function is_int() with int will always evaluate to true.',
49+
7,
50+
],
51+
]
52+
);
53+
}
54+
}

‎tests/Unit/PhpStan/IgnoreBooleanAlwaysImpossibleInstanceofTest.php‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sabberworm\CSS\Tests\Unit\PhpStan;
6+
7+
use PHPStan\Analyser\IgnoreErrorExtension;
8+
use PHPStan\Testing\RuleTestCase;
9+
use PHPStan\Rules\Classes\ImpossibleInstanceOfRule;
10+
use PHPStan\Rules\Rule;
11+
12+
/**
13+
* This covers `instanceof.alwaysTrue` error handling in the `IgnoreBooleanAlways` PHPStan extension class.
14+
* `RuleTestCase` allows only one `Rule` class, so separate `TestCase`s are needed for errors generated by other rules.
15+
* Note that the base class covers testing the suppression of the warning, and is included via `extends`.
16+
* This class covers the non-suppression of warnings in other contexts, which requires detailing the expected warnings.
17+
*
18+
* @covers \Sabberworm\CSS\PhpStan\IgnoreBooleanAlways
19+
*/
20+
final class IgnoreBooleanAlwaysImpossibleInstanceofTest extends IgnoreBooleanAlwaysTestBase
21+
{
22+
/**
23+
* @return ImpossibleInstanceOfRule
24+
*/
25+
protected function getRule(): Rule
26+
{
27+
// If the class is renamed or removed in the next major release of PHPStan, we'll deal with it then.
28+
// @phpstan-ignore phpstanApi.classConstant
29+
return self::getContainer()->getByType(ImpossibleInstanceOfRule::class);
30+
}
31+
32+
/**
33+
* @test
34+
*/
35+
public function warningIsRetainedForInstanceofNotInAssert(): void
36+
{
37+
// Skip the test in PHP/PHPStan configurations that don't have the required components.
38+
// It is good enough to test for those that do.
39+
if (!\interface_exists(IgnoreErrorExtension::class)) {
40+
self::markTestSkipped('This is testing the testers, and only needs to run whenever possible.');
41+
}
42+
43+
// Second argument is array of expected warnings.
44+
$this->analyse(
45+
[self::FIXTURES_DIR . 'alwaystrue-instanceof-notinassert.php'],
46+
[
47+
[
48+
'Instanceof between Exception and Exception will always evaluate to true.',
49+
6,
50+
],
51+
]
52+
);
53+
}
54+
}

‎tests/Unit/PhpStan/IgnoreBooleanAlwaysTestBase.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sabberworm\CSS\Tests\Unit\PhpStan;
6+
7+
use PHPStan\Analyser\IgnoreErrorExtension;
8+
use PHPStan\Testing\RuleTestCase;
9+
use PHPStan\Rules\Rule;
10+
11+
/**
12+
* Only one `Rule` can seemingly be covered by classes extending `RuleTestCase`.
13+
* This provides some common functionality and settings for `TestCase`s covering `IgnoreBooleanAlways`,
14+
* which involves more than one `Rule`.
15+
*
16+
* @extends RuleTestCase<Rule>
17+
*/
18+
abstract class IgnoreBooleanAlwaysTestBase extends RuleTestCase
19+
{
20+
/**
21+
* @var non-empty-string
22+
*/
23+
protected const FIXTURES_DIR = __DIR__ . '/../../fixtures/phpstan/';
24+
25+
/**
26+
* @test
27+
*/
28+
public function warningIsIgnoredInAssertInstanceOf(): void
29+
{
30+
// Skip the test in PHP/PHPStan configurations that don't have the required components.
31+
// It is good enough to test for those that do.
32+
if (!\interface_exists(IgnoreErrorExtension::class)) {
33+
self::markTestSkipped('This is testing the testers, and only needs to run whenever possible.');
34+
}
35+
36+
// Second argument is array of expected warnings.
37+
$this->analyse([self::FIXTURES_DIR . 'alwaystrue-instanceof-inassert.php'], []);
38+
}
39+
40+
/**
41+
* @return non-empty-array<string>
42+
*/
43+
public static function getAdditionalConfigFiles(): array
44+
{
45+
return \array_merge(
46+
parent::getAdditionalConfigFiles(),
47+
[self::FIXTURES_DIR . 'ignorebooleanalways.neon']
48+
);
49+
}
50+
}

‎tests/fixtures/phpstan/alwaystrue-instanceof-inassert.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
$e = new \Exception();
6+
\assert($e instanceof \Exception);

‎tests/fixtures/phpstan/alwaystrue-instanceof-notinassert.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
$e = new \Exception();
6+
if ($e instanceof \Exception) {
7+
$theTest = 'passed';
8+
}

‎tests/fixtures/phpstan/alwaystrue-pointlessassert.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
function pointless(int $value): void
6+
{
7+
\assert(\is_int($value));
8+
}

‎tests/fixtures/phpstan/ignorebooleanalways.neon‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
services:
2+
-
3+
class: \Sabberworm\CSS\PhpStan\IgnoreBooleanAlways
4+
tags:
5+
- phpstan.ignoreErrorExtension

0 commit comments

Comments
 (0)