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,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
 ------
 

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()`.

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%'

src/Rules/BannedNodesErrorBuilder.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the ekino/phpstan-banned-code project.
+ *
+ * (c) Ekino
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Ekino\PHPStanBannedCode\Rules;
+
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+
+/**
+ * @author Rémi Marseille <remi.marseille@ekino.com>
+ */
+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();
+    }
+}

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<string, mixed>
      */
-    private $bannedNodes;
+    private array $bannedNodes;
 
     /**
      * @var array<string>
      */
-    private $bannedFunctions;
+    private array $bannedFunctions;
 
     /**
      * @param array<array{type: string, functions?: array<string>}> $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',
+        )];
     }
 
     /**

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',
+                );
             }
         }
 

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,7 @@
 use PhpParser\Node\Name\FullyQualified;
 use PhpParser\Node\Scalar\LNumber;
 use PHPStan\Analyser\Scope;
+use PHPStan\Rules\NonIgnorableRuleError;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 
@@ -50,14 +52,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 +88,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 +125,10 @@ 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);
+        $this->assertStringStartsWith('ekinoBannedCode.', $errors[0]->getIdentifier());
+        $this->assertInstanceOf(NonIgnorableRuleError::class, $errors[0]);
     }
 
     protected function assertNodePasses(BannedNodesRule $rule, Node $node): void

tests/Rules/BannedUseTestRuleTest.php

@@ -13,12 +13,14 @@
 
 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\NonIgnorableRuleError;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 
@@ -42,7 +44,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 +65,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 +106,9 @@ 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);
+        $this->assertStringStartsWith('ekinoBannedCode.', $errors[0]->getIdentifier());
+        $this->assertInstanceOf(NonIgnorableRuleError::class, $errors[0]);
     }
 }