From c3d69757882c37defc4368572783bcf3c3c6d478 Mon Sep 17 00:00:00 2001 From: Florent Morselli Date: Tue, 13 Aug 2024 18:00:47 +0200 Subject: [PATCH] Add non-ignorable error handling to banned nodes rules Extend the `BannedNodesRule` and `BannedUseTestRule` classes to support non-ignorable error handling. Introduced the `BannedNodesErrorBuilder` class to streamline error message construction and updated the configuration and tests accordingly. --- CHANGELOG.md | 6 +++ README.md | 3 ++ extension.neon | 9 ++++ src/Rules/BannedNodesErrorBuilder.php | 43 ++++++++++++++++++ src/Rules/BannedNodesRule.php | 21 ++++++--- src/Rules/BannedUseTestRule.php | 19 ++++---- tests/Rules/BannedNodesRuleTest.php | 65 +++++++++++++++++---------- tests/Rules/BannedUseTestRuleTest.php | 23 ++++++++-- 8 files changed, 145 insertions(+), 44 deletions(-) create mode 100644 src/Rules/BannedNodesErrorBuilder.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 30f5283..1b18258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ master * todo... +v2.1.0 +------ + +* Added option to prevent errors to be ignored +* Added error identifiers starting with `ekinoBannedCode.` + v2.0.0 ------ diff --git a/README.md b/README.md index 03642a6..013d7a3 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,9 @@ parameters: # enable detection of `use Tests\Foo\Bar` in a non-test file use_from_tests: true + + # errors emitted by the extension are non-ignorable by default, so they cannot accidentally be put into the baseline. + non_ignorable: false # default is true ``` `type` is the returned value of a node, see the method `getType()`. diff --git a/extension.neon b/extension.neon index 1ebe93b..02a1707 100644 --- a/extension.neon +++ b/extension.neon @@ -5,6 +5,7 @@ parametersSchema: functions: schema(listOf(string()), nullable()) ])) use_from_tests: bool() + non_ignorable: bool() ]) parameters: @@ -54,6 +55,9 @@ parameters: # enable detection of `use Tests\Foo\Bar` in a non-test file use_from_tests: true + # when true, errors cannot be excluded + non_ignorable: true + services: - class: Ekino\PHPStanBannedCode\Rules\BannedNodesRule @@ -68,3 +72,8 @@ services: - phpstan.rules.rule arguments: - '%banned_code.use_from_tests%' + + - + class: Ekino\PHPStanBannedCode\Rules\BannedNodesErrorBuilder + arguments: + - '%banned_code.non_ignorable%' diff --git a/src/Rules/BannedNodesErrorBuilder.php b/src/Rules/BannedNodesErrorBuilder.php new file mode 100644 index 0000000..a0892c9 --- /dev/null +++ b/src/Rules/BannedNodesErrorBuilder.php @@ -0,0 +1,43 @@ + + */ +class BannedNodesErrorBuilder +{ + public const ERROR_IDENTIFIER_PREFIX = 'ekinoBannedCode'; + + public function __construct(private readonly bool $nonIgnorable) + { + } + + public function buildError( + string $errorMessage, + string $errorSuffix + ): RuleError { + $errBuilder = RuleErrorBuilder::message($errorMessage) + ->identifier(\sprintf('%s.%s', self::ERROR_IDENTIFIER_PREFIX, $errorSuffix)); + + if ($this->nonIgnorable) { + $errBuilder->nonIgnorable(); + } + + return $errBuilder->build(); + } +} diff --git a/src/Rules/BannedNodesRule.php b/src/Rules/BannedNodesRule.php index 47aeea3..092d9c0 100644 --- a/src/Rules/BannedNodesRule.php +++ b/src/Rules/BannedNodesRule.php @@ -15,7 +15,6 @@ use PhpParser\Node; use PhpParser\Node\Expr\FuncCall; -use PhpParser\Node\Expr\Variable; use PhpParser\Node\Name; use PHPStan\Analyser\Scope; use PHPStan\Rules\Rule; @@ -28,18 +27,20 @@ class BannedNodesRule implements Rule /** * @var array */ - private $bannedNodes; + private array $bannedNodes; /** * @var array */ - private $bannedFunctions; + private array $bannedFunctions; /** * @param array}> $bannedNodes */ - public function __construct(array $bannedNodes) - { + public function __construct( + array $bannedNodes, + private readonly BannedNodesErrorBuilder $errorBuilder + ) { $this->bannedNodes = array_column($bannedNodes, null, 'type'); $this->bannedFunctions = $this->normalizeFunctionNames($this->bannedNodes); } @@ -71,13 +72,19 @@ public function processNode(Node $node, Scope $scope): array $function = $node->name->toString(); if (\in_array($function, $this->bannedFunctions)) { - return [\sprintf('Should not use function "%s", please change the code.', $function)]; + return [$this->errorBuilder->buildError( + \sprintf('Should not use function "%s", please change the code.', $function), + 'function', + )]; } return []; } - return [\sprintf('Should not use node with type "%s", please change the code.', $type)]; + return [$this->errorBuilder->buildError( + \sprintf('Should not use node with type "%s", please change the code.', $type), + 'expression', + )]; } /** diff --git a/src/Rules/BannedUseTestRule.php b/src/Rules/BannedUseTestRule.php index 865da36..82abb66 100644 --- a/src/Rules/BannedUseTestRule.php +++ b/src/Rules/BannedUseTestRule.php @@ -23,17 +23,11 @@ */ class BannedUseTestRule implements Rule { - /** - * @var bool - */ - private $enabled; - - /** - * @param bool $enabled - */ - public function __construct(bool $enabled = true) + public function __construct( + private readonly bool $enabled, + private readonly BannedNodesErrorBuilder $errorBuilder + ) { - $this->enabled = $enabled; } /** @@ -69,7 +63,10 @@ public function processNode(Node $node, Scope $scope): array foreach ($node->uses as $use) { if (preg_match('#^Tests#', $use->name->toString())) { - $errors[] = \sprintf('Should not use %s in the non-test file %s', $use->name->toString(), $scope->getFile()); + $errors[] = $this->errorBuilder->buildError( + \sprintf('Should not use %s in the non-test file %s', $use->name->toString(), $scope->getFile()), + 'test', + ); } } diff --git a/tests/Rules/BannedNodesRuleTest.php b/tests/Rules/BannedNodesRuleTest.php index 90dd794..621472e 100644 --- a/tests/Rules/BannedNodesRuleTest.php +++ b/tests/Rules/BannedNodesRuleTest.php @@ -13,6 +13,7 @@ namespace Tests\Ekino\PHPStanBannedCode\Rules; +use Ekino\PHPStanBannedCode\Rules\BannedNodesErrorBuilder; use Ekino\PHPStanBannedCode\Rules\BannedNodesRule; use PhpParser\Node; use PhpParser\Node\Expr; @@ -27,6 +28,9 @@ use PhpParser\Node\Name\FullyQualified; use PhpParser\Node\Scalar\LNumber; use PHPStan\Analyser\Scope; +use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Rules\NonIgnorableRuleError; +use PHPStan\Rules\RuleError; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -50,14 +54,17 @@ class BannedNodesRuleTest extends TestCase */ protected function setUp(): void { - $this->rule = new BannedNodesRule([ - ['type' => 'Stmt_Echo'], - ['type' => 'Expr_Eval'], - ['type' => 'Expr_Exit'], - ['type' => 'Expr_FuncCall', 'functions' => ['debug_backtrace', 'dump', 'Safe\namespaced']], - ['type' => 'Expr_Print'], - ['type' => 'Expr_ShellExec'], - ]); + $this->rule = new BannedNodesRule( + [ + ['type' => 'Stmt_Echo'], + ['type' => 'Expr_Eval'], + ['type' => 'Expr_Exit'], + ['type' => 'Expr_FuncCall', 'functions' => ['debug_backtrace', 'dump', 'Safe\namespaced']], + ['type' => 'Expr_Print'], + ['type' => 'Expr_ShellExec'], + ], + new BannedNodesErrorBuilder(true) + ); $this->scope = $this->createMock(Scope::class); } @@ -83,25 +90,31 @@ public function testProcessNodeWithUnhandledType(Expr $node): void public function testProcessNodeWithBannedFunctions(): void { - $ruleWithoutLeadingSlashes = new BannedNodesRule([ + $ruleWithoutLeadingSlashes = new BannedNodesRule( [ - 'type' => 'Expr_FuncCall', - 'functions' => [ - 'root', - 'Safe\namespaced', - ] + [ + 'type' => 'Expr_FuncCall', + 'functions' => [ + 'root', + 'Safe\namespaced', + ] + ], ], - ]); + new BannedNodesErrorBuilder(true) + ); - $ruleWithLeadingSlashes = new BannedNodesRule([ + $ruleWithLeadingSlashes = new BannedNodesRule( [ - 'type' => 'Expr_FuncCall', - 'functions' => [ - '\root', - '\Safe\namespaced', - ] + [ + 'type' => 'Expr_FuncCall', + 'functions' => [ + '\root', + '\Safe\namespaced', + ] + ], ], - ]); + new BannedNodesErrorBuilder(true) + ); $rootFunction = new FuncCall(new Name('root')); $this->assertNodeTriggersError($ruleWithoutLeadingSlashes, $rootFunction); @@ -114,7 +127,13 @@ public function testProcessNodeWithBannedFunctions(): void protected function assertNodeTriggersError(BannedNodesRule $rule, Node $node): void { - $this->assertCount(1, $rule->processNode($node, $this->scope)); + $errors = $rule->processNode($node, $this->scope); + $this->assertCount(1, $errors); + $error = $errors[0]; + $this->assertInstanceOf(RuleError::class, $error); + $this->assertInstanceOf(IdentifierRuleError::class, $error); + $this->assertStringStartsWith('ekinoBannedCode.', $error->getIdentifier()); + $this->assertInstanceOf(NonIgnorableRuleError::class, $error); } protected function assertNodePasses(BannedNodesRule $rule, Node $node): void diff --git a/tests/Rules/BannedUseTestRuleTest.php b/tests/Rules/BannedUseTestRuleTest.php index a291cb9..8027392 100644 --- a/tests/Rules/BannedUseTestRuleTest.php +++ b/tests/Rules/BannedUseTestRuleTest.php @@ -13,12 +13,16 @@ namespace Tests\Ekino\PHPStanBannedCode\Rules; +use Ekino\PHPStanBannedCode\Rules\BannedNodesErrorBuilder; use Ekino\PHPStanBannedCode\Rules\BannedUseTestRule; use PhpParser\Node; use PhpParser\Node\Name; use PhpParser\Node\Stmt\Use_; use PhpParser\Node\Stmt\UseUse; use PHPStan\Analyser\Scope; +use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Rules\NonIgnorableRuleError; +use PHPStan\Rules\RuleError; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -42,7 +46,10 @@ class BannedUseTestRuleTest extends TestCase */ protected function setUp(): void { - $this->rule = new BannedUseTestRule(); + $this->rule = new BannedUseTestRule( + true, + new BannedNodesErrorBuilder(true) + ); $this->scope = $this->createMock(Scope::class); } @@ -60,8 +67,12 @@ public function testGetNodeType(): void public function testProcessNodeIfDisabled(): void { $this->scope->expects($this->never())->method('getNamespace'); + $testRule = new BannedUseTestRule( + false, + new BannedNodesErrorBuilder(true) + ); - $this->assertCount(0, (new BannedUseTestRule(false))->processNode($this->createMock(Use_::class), $this->scope)); + $this->assertCount(0, ($testRule)->processNode($this->createMock(Use_::class), $this->scope)); } /** @@ -97,6 +108,12 @@ public function testProcessNodeWithErrors(): void new UseUse(new Name('Tests\\Foo\\Bar')), ]); - $this->assertCount(1, $this->rule->processNode($node, $this->scope)); + $errors = $this->rule->processNode($node, $this->scope); + $this->assertCount(1, $errors); + $error = $errors[0]; + $this->assertInstanceOf(RuleError::class, $error); + $this->assertInstanceOf(IdentifierRuleError::class, $error); + $this->assertStringStartsWith('ekinoBannedCode.', $error->getIdentifier()); + $this->assertInstanceOf(NonIgnorableRuleError::class, $error); } }