Rework constructor params checker

erickskrauch · Oct 2, 2023 · 2db421d · 2db421d
1 parent 855910b
commit 2db421d
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 94 deletions.
diff --git a/extension.neon b/extension.neon
@@ -45,6 +45,8 @@ parametersSchema:
 
 services:
   - class: ErickSkrauch\PHPStan\Yii2\Rule\YiiConfigHelper
+    arguments:
+      reportMaybes: %reportMaybes%
 
   - class: ErickSkrauch\PHPStan\Yii2\Reflection\ApplicationPropertiesClassReflectionExtension
     tags: [phpstan.broker.propertiesClassReflectionExtension]

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -18,3 +18,4 @@ parameters:
     - '#Calling PHPStan\\Rules\\RuleLevelHelper::acceptsWithReason\(\) is not covered.+#'
     - '#Calling PHPStan\\Reflection\\Annotations\\AnnotationsPropertiesClassReflectionExtension\:\:(has|get)Property\(\) is not covered.+#'
     - '#Creating new PHPStan\\Reflection\\Dummy\\DummyPropertyReflection is not covered.+#'
+    - '#Creating new PHPStan\\Node\\Expr\\TypeExpr is not covered.+#'
diff --git a/src/Helper/RuleHelper.php b/src/Helper/RuleHelper.php
diff --git a/src/Rule/YiiConfigHelper.php b/src/Rule/YiiConfigHelper.php
@@ -3,35 +3,31 @@
 
 namespace ErickSkrauch\PHPStan\Yii2\Rule;
 
-use ErickSkrauch\PHPStan\Yii2\Helper\RuleHelper;
-use PhpParser\Node;
-use PhpParser\Node\Expr;
 use PHPStan\Analyser\Scope;
 use PHPStan\Internal\SprintfHelper;
 use PHPStan\Node\Expr\TypeExpr;
-use PHPStan\Rules\Classes\InstantiationRule;
+use PHPStan\Reflection\ParametersAcceptorSelector;
 use PHPStan\Rules\RuleErrorBuilder;
 use PHPStan\Rules\RuleLevelHelper;
 use PHPStan\Type\Constant\ConstantArrayType;
 use PHPStan\Type\Constant\ConstantIntegerType;
 use PHPStan\Type\Constant\ConstantStringType;
 use PHPStan\Type\ErrorType;
 use PHPStan\Type\Type;
-use PHPStan\Type\TypeCombinator;
 use PHPStan\Type\VerbosityLevel;
 
 final class YiiConfigHelper {
 
     private RuleLevelHelper $ruleLevelHelper;
 
-    private InstantiationRule $instantiationRule;
+    private bool $reportMaybes;
 
     public function __construct(
         RuleLevelHelper $ruleLevelHelper,
-        InstantiationRule $instantiationRule
+        bool $reportMaybes
     ) {
         $this->ruleLevelHelper = $ruleLevelHelper;
-        $this->instantiationRule = $instantiationRule;
+        $this->reportMaybes = $reportMaybes;
     }
 
     /**
@@ -100,7 +96,7 @@ public function validateArray(Type $object, ConstantArrayType $config, Scope $sc
 
             $typeResult = $this->ruleLevelHelper->findTypeToCheck(
                 $scope,
-                new TypeExpr($object), // @phpstan-ignore-line @ondrejmirtes said that I can use that method
+                new TypeExpr($object),
                 sprintf('Access to property $%s on an unknown class %%s.', SprintfHelper::escapeFormatString($propertyName)), // @phpstan-ignore-line @ondrejmirtes said that I can use that method
                 static fn(Type $type): bool => $type->canAccessProperties()->yes() && $type->hasProperty($propertyName)->yes(),
             );
@@ -168,50 +164,114 @@ public function validateArray(Type $object, ConstantArrayType $config, Scope $sc
      * @phpstan-return list<\PHPStan\Rules\IdentifierRuleError>
      */
     public function validateConstructorArgs(Type $object, ConstantArrayType $config, Scope $scope): array {
+        /** @var \PHPStan\Type\Type|null $firstParamKeyType */
+        $firstParamKeyType = null;
         $errors = [];
-        /** @var \PhpParser\Node\Arg[] $args */
-        $args = [];
-        /** @var Type|null $firstKeyType */
-        $firstKeyType = null;
+
+        /** @var ConstantIntegerType|ConstantStringType $key */
         foreach ($config->getKeyTypes() as $i => $key) {
-            /** @var Type $value */
-            $value = $config->getValueTypes()[$i];
+            /** @var \PHPStan\Type\Type $paramValue */
+            $paramValue = $config->getValueTypes()[$i];
+            $paramName = $key->getValue();
+            $paramStrToReport = is_int($paramName) ? ('#' . ($paramName + 1)) : ('$' . $paramName);
 
-            if ($firstKeyType === null) {
-                $firstKeyType = $key;
-            } elseif (!$firstKeyType instanceof $key) {
+            if ($firstParamKeyType === null) {
+                $firstParamKeyType = $key;
+            } elseif (!$firstParamKeyType instanceof $key) {
                 $errors[] = RuleErrorBuilder::message("Parameters indexed by name and by position in the same array aren't allowed.")
                     ->identifier('yiiConfig.constructorParamsMix')
                     ->build();
                 continue;
             }
 
-            if ($key instanceof ConstantIntegerType) {
-                // @phpstan-ignore-next-line I know about backward compatibility promise
-                $args[] = new Node\Arg(new TypeExpr($value));
-            } else {
-                // @phpstan-ignore-next-line I know about backward compatibility promise
-                $args[] = new Node\Arg(new TypeExpr($value), false, false, [], new Node\Identifier($key->getValue()));
+            /** @var list<\PHPStan\Rules\IdentifierRuleError> $paramSearchErrors */
+            $paramSearchErrors = [];
+            /** @var \PHPStan\Reflection\ParameterReflection[] $foundParams */
+            $foundParams = [];
+            /** @var \PHPStan\Reflection\ClassReflection $classReflection */
+            foreach ($object->getObjectClassReflections() as $classReflection) {
+                $constructorParams = ParametersAcceptorSelector::selectSingle($classReflection->getConstructor()->getVariants())->getParameters();
+                $param = null;
+
+                // TODO: prevent direct pass of 'config' param to constructor args (\yii\base\Configurable)
+
+                if (is_int($paramName)) {
+                    $param = $constructorParams[$paramName] ?? null;
+                } else {
+                    foreach ($constructorParams as $constructorParam) {
+                        if ($constructorParam->getName() === $paramName) {
+                            $param = $constructorParam;
+                            break;
+                        }
+                    }
+                }
+
+                if ($param === null) {
+                    $paramSearchErrors[] = RuleErrorBuilder::message(sprintf(
+                        'Unknown parameter %s in call to %s constructor.',
+                        $paramStrToReport,
+                        $classReflection->getName(),
+                    ))
+                        ->identifier('argument.unknown')
+                        ->build();
+                    continue;
+                }
+
+                $foundParams[$classReflection->getName()] = $param;
             }
-        }
 
-        if (!empty($errors)) {
-            return $errors;
-        }
+            if (empty($foundParams)) {
+                $paramSearchErrors[] = RuleErrorBuilder::message(sprintf(
+                    'Unknown parameter %s in call to %s constructor.',
+                    $paramStrToReport,
+                    $object->describe(VerbosityLevel::typeOnly()),
+                ))
+                    ->identifier('argument.unknown')
+                    ->build();
+                continue;
+            }
 
-        $classNamesTypes = [];
-        foreach ($object->getObjectClassNames() as $className) {
-            $classNamesTypes[] = new ConstantStringType($className, true);
-        }
+            if ($this->reportMaybes && !empty($paramSearchErrors)) {
+                $errors = array_merge($errors, $paramSearchErrors);
+                continue;
+            }
 
-        // @phpstan-ignore-next-line I know about backward compatibility promise
-        $newNode = new Expr\New_(new TypeExpr(TypeCombinator::union(...$classNamesTypes)), $args);
+            /** @var list<\PHPStan\Rules\IdentifierRuleError> $typeCheckErrors */
+            $typeCheckErrors = [];
+            $accepted = false;
+            foreach ($foundParams as $className => $constructorParam) {
+                // TODO: expose param name
+                $paramType = $constructorParam->getType();
+                $result = $this->ruleLevelHelper->acceptsWithReason($paramType, $paramValue, $scope->isDeclareStrictTypes());
+                if (!$result->result) {
+                    $level = VerbosityLevel::getRecommendedLevelByType($paramType, $paramValue);
+                    $typeCheckErrors[] = RuleErrorBuilder::message(sprintf(
+                        'Parameter %s of class %s constructor expects %s, %s given.',
+                        $paramStrToReport,
+                        $className,
+                        $paramType->describe($level),
+                        $paramValue->describe($level),
+                    ))
+                        ->identifier('argument.type')
+                        ->acceptsReasonsTip($result->reasons)
+                        ->build();
+                }
 
-        // @phpstan-ignore-next-line I know about backward compatibility promise
-        $errors = $this->instantiationRule->processNode($newNode, $scope);
+                $accepted |= $result->result;
+            }
+
+            if (!$accepted) {
+                // TODO: bad decision, need to create more specific error
+                $errors[] = $typeCheckErrors[0];
+                continue;
+            }
 
-        // @phpstan-ignore-next-line it does return the correct type, but some internal PHPStan's magic fails here
-        return array_map([RuleHelper::class, 'removeLine'], $errors);
+            if ($this->reportMaybes && !empty($typeCheckErrors)) {
+                $errors = array_merge($errors, $typeCheckErrors);
+            }
+        }
+
+        return $errors;
     }
 
 }
diff --git a/tests/Rule/CreateObjectRuleTest.php b/tests/Rule/CreateObjectRuleTest.php
@@ -21,8 +21,8 @@ public function testRule(): void {
         $this->analyse([__DIR__ . '/_data/create_object_array_valid.php'], []);
         $this->analyse([__DIR__ . '/_data/create_object_array_union_type_valid.php'], []);
         $this->analyse([__DIR__ . '/_data/create_object_classname_invalid.php'], [
-            ['Parameter #1 $stringArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects string, int given.', 6],
-            ['Parameter #2 $intArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects int, string given.', 6],
+            ['Parameter #1 of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects string, int given.', 6],
+            ['Parameter #2 of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects int, string given.', 6],
             ['Parameter $stringArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects string, int given.', 7],
             ['Parameter $intArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects int, string given.', 7],
         ]);
@@ -46,15 +46,15 @@ public function testRule(): void {
             ['Property ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent::$readonlyFunctionProp is not writable.', 6],
         ]);
         $this->analyse([__DIR__ . '/_data/create_object_array_with_indexed_args.php'], [
-            ['Parameter #1 $stringArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects string, int given.', 6],
-            ['Parameter #2 $intArg of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects int, string given.', 6],
+            ['Parameter #1 of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects string, int given.', 6],
+            ['Parameter #2 of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\MyComponent constructor expects int, string given.', 6],
         ]);
         $this->analyse([__DIR__ . '/_data/create_object_array_with_missing_class.php'], [
             ['Configuration params array must have "class" or "__class" key', 4],
         ]);
         $this->analyse([__DIR__ . '/_data/create_object_array_union_type_invalid.php'], [
             ["The config for ErickSkrauch\PHPStan\Yii2\Tests\Yii\Article|ErickSkrauch\PHPStan\Yii2\Tests\Yii\Comment is wrong: the property field doesn't exists", 10],
-            ['Parameter #1 $dateTime of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\BarComponent constructor expects DateTimeInterface, string given.', 17],
+            ['Parameter #1 of class ErickSkrauch\PHPStan\Yii2\Tests\Yii\BarComponent constructor expects DateTimeInterface, string given.', 17],
         ]);
     }