-
Notifications
You must be signed in to change notification settings - Fork 0
Added new proposed rules #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
606ac75
8850574
8f8cfa3
9041841
e2e0a92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| /vendor | ||
| /phpunit.xml | ||
| /.php_cs.cache | ||
| composer.lock | ||
| .php-cs-fixer.cache | ||
| .phpunit.result.cache |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| parameters: | ||
| ignoreErrors: | ||
| - | ||
| message: "#^Property Ibexa\\\\Tests\\\\PHPStan\\\\Rules\\\\Fixtures\\\\RequireInterfaceInDependenciesFixture\\:\\:\\$classWithoutInterface is never read, only written\\.$#" | ||
| count: 1 | ||
| path: tests/rules/Fixtures/RequireInterfaceInDependenciesFixture.php | ||
|
|
||
| - | ||
| message: "#^Property Ibexa\\\\Tests\\\\PHPStan\\\\Rules\\\\Fixtures\\\\RequireInterfaceInDependenciesFixture\\:\\:\\$concreteClass is never read, only written\\.$#" | ||
| count: 1 | ||
| path: tests/rules/Fixtures/RequireInterfaceInDependenciesFixture.php | ||
|
|
||
| - | ||
| message: "#^Property Ibexa\\\\Tests\\\\PHPStan\\\\Rules\\\\Fixtures\\\\RequireInterfaceInDependenciesFixture\\:\\:\\$testInterface is never read, only written\\.$#" | ||
| count: 1 | ||
| path: tests/rules/Fixtures/RequireInterfaceInDependenciesFixture.php |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| includes: | ||
| - phpstan-baseline.neon | ||
|
|
||
| parameters: | ||
| level: 8 | ||
| paths: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\PHPStan\Rules; | ||
|
|
||
| use PhpParser\Node; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Reflection\ReflectionProvider; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
|
|
||
| /** | ||
| * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Stmt\Class_> | ||
| */ | ||
| final class FinalClassRule implements Rule | ||
| { | ||
| private ReflectionProvider $reflectionProvider; | ||
|
|
||
| public function __construct( | ||
| ReflectionProvider $reflectionProvider | ||
| ) { | ||
| $this->reflectionProvider = $reflectionProvider; | ||
| } | ||
|
|
||
| public function getNodeType(): string | ||
| { | ||
| return Node\Stmt\Class_::class; | ||
| } | ||
|
|
||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| // Skip anonymous classes | ||
| if (!isset($node->namespacedName)) { | ||
| return []; | ||
| } | ||
|
|
||
| $className = $node->namespacedName->toString(); | ||
|
|
||
| if (!$this->reflectionProvider->hasClass($className)) { | ||
| return []; | ||
| } | ||
|
|
||
| $reflection = $this->reflectionProvider->getClass($className); | ||
|
|
||
| // Skip if already final | ||
| if ($reflection->isFinal()) { | ||
| return []; | ||
| } | ||
|
|
||
| // Skip if abstract (abstract classes shouldn't be final) | ||
| if ($reflection->isAbstract()) { | ||
| return []; | ||
| } | ||
|
|
||
| // Skip interfaces and traits | ||
| if ($reflection->isInterface() || $reflection->isTrait()) { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Class %s is not final. All non-abstract classes should be final.', | ||
| $reflection->getName() | ||
| ) | ||
| ) | ||
| ->identifier('class.notFinal') | ||
| ->tip('Add "final" keyword to the class declaration.') | ||
| ->build(), | ||
| ]; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\PHPStan\Rules; | ||
|
|
||
| use PhpParser\Node; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
|
|
||
| /** | ||
| * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Stmt\ClassLike> | ||
| */ | ||
| final class NamingConventionRule implements Rule | ||
konradoboza marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| public function getNodeType(): string | ||
| { | ||
| return Node\Stmt\ClassLike::class; | ||
| } | ||
|
|
||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| if (!$this->isRelevantNode($node)) { | ||
| return []; | ||
| } | ||
|
|
||
| if ($node->name === null) { | ||
| return []; | ||
| } | ||
|
|
||
| $className = $node->name->toString(); | ||
| $errors = []; | ||
|
|
||
| if ($node instanceof Node\Stmt\Interface_ && substr($className, -9) !== 'Interface') { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Interface "%s" should have "Interface" suffix', | ||
| $className | ||
| ) | ||
| )->build(); | ||
| } | ||
|
|
||
| if ($node instanceof Node\Stmt\Trait_ && substr($className, -5) !== 'Trait') { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Trait "%s" should have "Trait" suffix', | ||
| $className | ||
| ) | ||
| )->build(); | ||
| } | ||
|
|
||
| if ($node instanceof Node\Stmt\Class_ && $node->isAbstract() && strpos($className, 'Abstract') !== 0) { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Abstract class "%s" should have "Abstract" prefix', | ||
| $className | ||
| ) | ||
| )->build(); | ||
| } | ||
|
|
||
| return $errors; | ||
| } | ||
|
|
||
| private function isRelevantNode(Node $node): bool | ||
| { | ||
| return $node instanceof Node\Stmt\Interface_ | ||
| || $node instanceof Node\Stmt\Trait_ | ||
| || ($node instanceof Node\Stmt\Class_ && $node->isAbstract()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\PHPStan\Rules; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\Error; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
|
|
||
| /** | ||
| * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Stmt\ClassMethod> | ||
| */ | ||
| final class RequireInterfaceInDependenciesRule implements Rule | ||
| { | ||
| public function getNodeType(): string | ||
| { | ||
| return Node\Stmt\ClassMethod::class; | ||
| } | ||
|
|
||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| $errors = []; | ||
|
|
||
| if (!$node->params) { | ||
| return []; | ||
| } | ||
|
|
||
| foreach ($node->params as $param) { | ||
| if (!$param->type instanceof Node\Name) { | ||
| continue; | ||
| } | ||
| if ($param->var instanceof Error) { | ||
| continue; | ||
| } | ||
| $typeName = $param->type->toString(); | ||
|
|
||
| // Skip built-in types and primitives | ||
| if ($this->isBuiltInType($typeName)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!interface_exists($typeName) && class_exists($typeName)) { | ||
| // Check if this class implements any interface | ||
|
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are valid cases where a class implements an interface, but is not itself logically related to said class. For example, I think this rule should be configurable, and instead we should use a list of interfaces that we want to keep track of. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
totally disagree. This would create maintanance hell. As with Finals, its not to be 100% right, I totally expect that some things will land in ignore or baseline and we were discussing that. It's just those are 1% (IDZD) I dont think we need to care about. And there is this tiny case, when given concrete class comes from third party and does not have any interface at all. And just add it to ignore or use phpstan ignore anotation and everything is clear during review. |
||
| $interfaces = class_implements($typeName); | ||
|
|
||
| if (!empty($interfaces)) { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Parameter $%s uses concrete class %s instead of an interface. Available interfaces: %s', | ||
| is_string($param->var->name) ? $param->var->name : $param->var->name->getType(), | ||
| $typeName, | ||
| implode(', ', $interfaces) | ||
| ) | ||
| )->build(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $errors; | ||
| } | ||
|
|
||
| private function isBuiltInType(string $type): bool | ||
| { | ||
| $builtInTypes = [ | ||
| 'string', 'int', 'float', 'bool', 'array', 'object', | ||
| 'callable', 'iterable', 'mixed', 'void', 'never', | ||
| ]; | ||
|
|
||
| return in_array(strtolower($type), $builtInTypes); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules; | ||
|
|
||
| use Ibexa\PHPStan\Rules\FinalClassRule; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Testing\RuleTestCase; | ||
|
|
||
| /** | ||
| * @extends \PHPStan\Testing\RuleTestCase<\Ibexa\PHPStan\Rules\FinalClassRule> | ||
| */ | ||
| final class FinalClassRuleTest extends RuleTestCase | ||
| { | ||
| protected function getRule(): Rule | ||
| { | ||
| return new FinalClassRule($this->createReflectionProvider()); | ||
| } | ||
|
|
||
| public function testRule(): void | ||
| { | ||
| $this->analyse( | ||
| [ | ||
| __DIR__ . '/Fixtures/FinalClass/NonFinalClass.php', | ||
| ], | ||
| [ | ||
| [ | ||
| 'Class Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass\NonFinalClass is not final. All non-abstract classes should be final.', | ||
| 11, | ||
| 'Add "final" keyword to the class declaration.', | ||
| ], | ||
| ] | ||
| ); | ||
| } | ||
|
|
||
| public function testNoErrorsOnFinalAndAbstractClassesAndInterfaces(): void | ||
| { | ||
| $this->analyse( | ||
| [ | ||
| __DIR__ . '/Fixtures/FinalClass/FinalClass.php', | ||
| __DIR__ . '/Fixtures/FinalClass/AbstractClass.php', | ||
| __DIR__ . '/Fixtures/FinalClass/SomeInterface.php', | ||
| __DIR__ . '/Fixtures/FinalClass/SomeTrait.php', | ||
| ], | ||
| [] | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass; | ||
|
|
||
| abstract class AbstractClass | ||
| { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass; | ||
|
|
||
| final class FinalClass | ||
| { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass; | ||
|
|
||
| class NonFinalClass | ||
| { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass; | ||
|
|
||
| interface SomeInterface | ||
| { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Tests\PHPStan\Rules\Fixtures\FinalClass; | ||
|
|
||
| trait SomeTrait | ||
| { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can make this assumption and expect to never create classes that allow extending, but are themselves not abstract/final.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we were disscussing exactly that case on daily - its much lower effort to think why given class is NOT FINAL and describe that case with ignore annotation / baseline entry than to check if everything is final when writing / reviewing and thinking then if something is mistake or not.
99% we are either missing final or interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I am up for this change as it applies well to our codebase. What bothers me though is partners'/customers' code - I wonder if it's fair to report all non-abstract classes as not final. This is basically our way of doing things and I am not sure if it isn't to strict to enforce it outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If thats the only issue then then can always filter whole rule out in their config with ignoreErrors.