Skip to content

Commit

Permalink
Merge pull request #66 from Spomky/features/error-non-exclusion
Browse files Browse the repository at this point in the history
Add non-ignorable error handling feature
  • Loading branch information
mremi authored Aug 14, 2024
2 parents bab2937 + c3d6975 commit e1c3e47
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 44 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
------

Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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()`.
9 changes: 9 additions & 0 deletions extension.neon
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ parametersSchema:
functions: schema(listOf(string()), nullable())
]))
use_from_tests: bool()
non_ignorable: bool()
])

parameters:
Expand Down Expand Up @@ -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
Expand All @@ -68,3 +72,8 @@ services:
- phpstan.rules.rule
arguments:
- '%banned_code.use_from_tests%'

-
class: Ekino\PHPStanBannedCode\Rules\BannedNodesErrorBuilder
arguments:
- '%banned_code.non_ignorable%'
43 changes: 43 additions & 0 deletions src/Rules/BannedNodesErrorBuilder.php
Original file line number Diff line number Diff line change
@@ -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 <[email protected]>
*/
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();
}
}
21 changes: 14 additions & 7 deletions src/Rules/BannedNodesRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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',
)];
}

/**
Expand Down
19 changes: 8 additions & 11 deletions src/Rules/BannedUseTestRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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',
);
}
}

Expand Down
65 changes: 42 additions & 23 deletions tests/Rules/BannedNodesRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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);
}

Expand All @@ -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);
Expand All @@ -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
Expand Down
23 changes: 20 additions & 3 deletions tests/Rules/BannedUseTestRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}

Expand All @@ -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));
}

/**
Expand Down Expand Up @@ -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);
}
}

0 comments on commit e1c3e47

Please sign in to comment.