diff --git a/README.md b/README.md index 898a13b3..b9446499 100644 --- a/README.md +++ b/README.md @@ -2850,6 +2850,41 @@ public function handle(): void
+### NoNullableServiceInConstructorRule + +A constructor service dependency must not be nullable - a service is always provided by the container, so `?SomeService` only hides that it is really required. Nullable is allowed on an abstract class (a child fills the dependency) and on values that are not services: scalars, arrays, exceptions (`?Throwable $previous`) and date value objects. + +```yaml +rules: + - Symplify\PHPStanRules\Rules\Symfony\NoNullableServiceInConstructorRule +``` + +```php +public function __construct( + private readonly ?SomeService $someService, +) { +} +``` + +:x: + +
+ +```php +public function __construct( + private readonly SomeService $someService, +) { +} +``` + +:+1: + +
+ +--- + +
+ ## 4. PHPUnit-specific Rules ### NoAssertFuncCallInTestsRule diff --git a/config/symfony-rules.neon b/config/symfony-rules.neon index 0be3320d..7e1ce0b8 100644 --- a/config/symfony-rules.neon +++ b/config/symfony-rules.neon @@ -36,3 +36,4 @@ rules: # constructor injection - Symplify\PHPStanRules\Rules\Symfony\PreferInterfaceInConstructorRule + - Symplify\PHPStanRules\Rules\Symfony\NoNullableServiceInConstructorRule diff --git a/src/Enum/RuleIdentifier.php b/src/Enum/RuleIdentifier.php index 2ac9c714..a991a630 100644 --- a/src/Enum/RuleIdentifier.php +++ b/src/Enum/RuleIdentifier.php @@ -95,4 +95,6 @@ final class RuleIdentifier public const string REQUIRE_ARRAY_SHAPE_RETURN = 'symplify.requireArrayShapeReturn'; public const string NO_SERVICE_JUGGLING = 'symplify.noServiceJuggling'; + + public const string NO_NULLABLE_SERVICE_IN_CONSTRUCTOR = 'symplify.noNullableServiceInConstructor'; } diff --git a/src/Rules/Symfony/NoNullableServiceInConstructorRule.php b/src/Rules/Symfony/NoNullableServiceInConstructorRule.php new file mode 100644 index 00000000..084c879e --- /dev/null +++ b/src/Rules/Symfony/NoNullableServiceInConstructorRule.php @@ -0,0 +1,183 @@ + + */ +final readonly class NoNullableServiceInConstructorRule implements Rule +{ + public const string ERROR_MESSAGE = 'Constructor service "%s" of type "%s" is nullable. A service is always provided, make it non-nullable'; + + public function __construct( + private ReflectionProvider $reflectionProvider, + ) { + } + + public function getNodeType(): string + { + return ClassMethod::class; + } + + /** + * @param ClassMethod $node + * + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if ($node->name->toLowerString() !== '__construct') { + return []; + } + + // an anonymous class is a local one-off, not a container service + $classReflection = $scope->getClassReflection(); + if (! $classReflection instanceof ClassReflection || $classReflection->isAnonymous()) { + return []; + } + + // an abstract base class may leave a dependency optional for a child to provide + if ($classReflection->isAbstract()) { + return []; + } + + $paramTypes = $this->resolveParamClassTypes($node, $scope); + + $ruleErrors = []; + + foreach ($node->params as $param) { + $serviceName = $this->matchNullableServiceName($param->type); + if (! $serviceName instanceof Name) { + continue; + } + + $serviceType = $scope->resolveName($serviceName); + if ($this->isValueObjectType($serviceType)) { + continue; + } + + // a sibling param of the same type already provides it non-nullable, so this one is a real optional extra + if (count(array_keys($paramTypes, $serviceType, true)) > 1) { + continue; + } + + $parameterName = $param->var instanceof Variable && is_string($param->var->name) + ? '$' . $param->var->name + : ''; + + $ruleErrors[] = RuleErrorBuilder::message( + sprintf(self::ERROR_MESSAGE, $parameterName, $serviceType) + ) + ->identifier(RuleIdentifier::NO_NULLABLE_SERVICE_IN_CONSTRUCTOR) + ->line($param->getStartLine()) + ->build(); + } + + return $ruleErrors; + } + + /** + * Resolves every constructor param to its class-type name (nullable or not), for duplicate-type detection. + * + * @return string[] + */ + private function resolveParamClassTypes(ClassMethod $classMethod, Scope $scope): array + { + $paramTypes = []; + + foreach ($classMethod->params as $param) { + $className = $this->matchClassName($param->type); + if ($className instanceof Name) { + $paramTypes[] = $scope->resolveName($className); + } + } + + return $paramTypes; + } + + /** + * Returns the class-type name node of any type, nullable or not, null when the type is not a class type. + */ + private function matchClassName(Identifier|Name|ComplexType|null $type): ?Name + { + if ($type instanceof Name) { + return $type; + } + + return $this->matchNullableServiceName($type); + } + + /** + * Returns the class-type name node when the type is a nullable class type, null otherwise. + */ + private function matchNullableServiceName(Identifier|Name|ComplexType|null $type): ?Name + { + if ($type instanceof NullableType) { + return $type->type instanceof Name ? $type->type : null; + } + + if ($type instanceof UnionType) { + $className = null; + $hasNull = false; + + foreach ($type->types as $unionedType) { + if ($unionedType instanceof Identifier && $unionedType->toLowerString() === 'null') { + $hasNull = true; + + continue; + } + + if ($unionedType instanceof Name) { + $className = $unionedType; + } + } + + return $hasNull ? $className : null; + } + + return null; + } + + /** + * A nullable class type that is not really a service: an exception ("$previous") or a date value object. + */ + private function isValueObjectType(string $className): bool + { + if (! $this->reflectionProvider->hasClass($className)) { + return false; + } + + $classReflection = $this->reflectionProvider->getClass($className); + + return $classReflection->is(Throwable::class) || $classReflection->is(DateTimeInterface::class); + } +} diff --git a/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Fixture/ReportNullableService.php b/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Fixture/ReportNullableService.php new file mode 100644 index 00000000..08e70a2b --- /dev/null +++ b/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Fixture/ReportNullableService.php @@ -0,0 +1,17 @@ + + */ +final class NoNullableServiceInConstructorRuleTest extends RuleTestCase +{ + /** + * @param list $expectedErrorsWithLines + */ + #[DataProvider('provideData')] + public function testRule(string $filePath, array $expectedErrorsWithLines): void + { + $this->analyse([$filePath], $expectedErrorsWithLines); + } + + /** + * @return Iterator, mixed>> + */ + public static function provideData(): Iterator + { + $someServiceError = sprintf( + NoNullableServiceInConstructorRule::ERROR_MESSAGE, + '$someService', + SomeService::class + ); + $anotherServiceError = sprintf( + NoNullableServiceInConstructorRule::ERROR_MESSAGE, + '$anotherService', + AnotherService::class + ); + + yield [__DIR__ . '/Fixture/ReportNullableService.php', [[$someServiceError, 13], [$anotherServiceError, 14]]]; + + yield [__DIR__ . '/Fixture/SkipNullableScalar.php', []]; + yield [__DIR__ . '/Fixture/SkipNullableException.php', []]; + yield [__DIR__ . '/Fixture/SkipNullableDateTime.php', []]; + yield [__DIR__ . '/Fixture/SkipAbstractClass.php', []]; + yield [__DIR__ . '/Fixture/SkipDuplicateType.php', []]; + yield [__DIR__ . '/Fixture/SkipAnonymousClass.php', []]; + } + + protected function getRule(): Rule + { + return new NoNullableServiceInConstructorRule($this->createReflectionProvider()); + } +} diff --git a/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Source/AnotherService.php b/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Source/AnotherService.php new file mode 100644 index 00000000..21357f38 --- /dev/null +++ b/tests/Rules/Symfony/NoNullableServiceInConstructorRule/Source/AnotherService.php @@ -0,0 +1,9 @@ +