From 5e520e6bff0e6d132d07c1be144f0486ce10da89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Mon, 28 Aug 2023 01:12:28 +0200 Subject: [PATCH 1/3] bugfix: properly inherit assertions from parents or implemented interfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/Psalm/Codebase.php | 2 + .../ExistingAtomicMethodCallAnalyzer.php | 104 +++++++++++++++++- tests/AssertAnnotationTest.php | 84 ++++++++++++++ 3 files changed, 188 insertions(+), 2 deletions(-) diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index c56145a17c6..d2b5d54ceae 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -684,6 +684,8 @@ public function classOrInterfaceExists( /** * Check whether a class/interface exists + * + * @psalm-assert-if-true class-string|interface-string|enum-string $fq_class_name */ public function classOrInterfaceOrEnumExists( string $fq_class_name, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php index 980043f1ce7..15afcdc7b93 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -34,6 +34,8 @@ use Psalm\IssueBuffer; use Psalm\Node\Expr\VirtualFuncCall; use Psalm\Plugin\EventHandler\Event\AfterMethodCallAnalysisEvent; +use Psalm\Storage\Assertion; +use Psalm\Storage\ClassLikeStorage; use Psalm\Storage\Possibilities; use Psalm\Type; use Psalm\Type\Atomic; @@ -44,10 +46,13 @@ use function array_filter; use function array_map; +use function array_merge; +use function array_values; use function count; use function explode; use function in_array; use function is_string; +use function reset; use function strpos; use function strtolower; @@ -415,11 +420,45 @@ public static function analyze( } } - if ($method_storage->assertions) { + $assertions = $method_storage->assertions; + if ($method_storage->inheritdoc) { + $inherited_classes_and_interfaces = array_values(array_filter(array_merge( + $class_storage->parent_classes, + $class_storage->class_implements, + ), fn(string $classOrInterface) => $codebase->classOrInterfaceOrEnumExists($classOrInterface))); + + foreach ($inherited_classes_and_interfaces as $potential_assertion_providing_class) { + $potential_assertion_providing_classlike_storage = $codebase->classlike_storage_provider->get( + $potential_assertion_providing_class, + ); + if (!isset($potential_assertion_providing_classlike_storage->methods[$method_name_lc])) { + continue; + } + + $potential_assertion_providing_method_storage = $potential_assertion_providing_classlike_storage + ->methods[$method_name_lc]; + + /** + * Since the inheritance does not provide its own assertions, we have to detect those + * from inherited classes + */ + $assertions += array_map( + static fn(Possibilities $possibilities) => self::modifyAssertionsForInheritance( + $possibilities, + $codebase, + $class_storage, + $inherited_classes_and_interfaces, + ), + $potential_assertion_providing_method_storage->assertions, + ); + } + } + + if ($assertions) { self::applyAssertionsToContext( $stmt_name, ExpressionIdentifier::getExtendedVarId($stmt->var, null, $statements_analyzer), - $method_storage->assertions, + $assertions, $args, $template_result, $context, @@ -690,4 +729,65 @@ private static function getMagicGetterOrSetterProperty( return null; } + + /** + * In case the called class is either implementing or extending a class/interface which does also has the + * template we are searching for, we assume that the called method has the same assertions. + * + * @param list $potential_assertion_providing_classes + */ + private static function modifyAssertionsForInheritance( + Possibilities $possibilities, + Codebase $codebase, + ClassLikeStorage $called_class, + array $potential_assertion_providing_classes + ): Possibilities { + $replacement = new Possibilities($possibilities->var_id, []); + $extended_params = $called_class->template_extended_params; + foreach ($possibilities->rule as $assertion) { + if (!$assertion instanceof Assertion\IsType + || !$assertion->type instanceof TTemplateParam) { + $replacement->rule[] = $assertion; + continue; + } + + /** Called class does not extend the template parameter */ + $extended_templates = $called_class->template_extended_params; + if (!isset($extended_templates[$assertion->type->defining_class][$assertion->type->param_name])) { + $replacement->rule[] = $assertion; + continue; + } + + foreach ($potential_assertion_providing_classes as $potential_assertion_providing_class) { + if (!isset($extended_params[$potential_assertion_providing_class][$assertion->type->param_name])) { + continue; + } + + if (!$codebase->classlike_storage_provider->has($potential_assertion_providing_class)) { + continue; + } + + $potential_assertion_providing_classlike_storage = $codebase->classlike_storage_provider->get( + $potential_assertion_providing_class, + ); + if (!isset( + $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], + )) { + continue; + } + + $replacement->rule[] = new Assertion\IsType(new TTemplateParam( + $assertion->type->param_name, + reset($potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name]), + $potential_assertion_providing_class, + )); + + continue 2; + } + + $replacement->rule[] = $assertion; + } + + return $replacement; + } } diff --git a/tests/AssertAnnotationTest.php b/tests/AssertAnnotationTest.php index 16cdf6d5244..7bba9297f2c 100644 --- a/tests/AssertAnnotationTest.php +++ b/tests/AssertAnnotationTest.php @@ -2883,6 +2883,90 @@ public static function doAssert($value): void '$iterable===' => 'non-empty-list', ], ], + 'assertFromInheritedDocBlock' => [ + 'code' => ' + */ + abstract class AbstractPluginManager implements PluginManagerInterface + { + /** @param InstanceType $value */ + public function __construct(private readonly mixed $value) + {} + + /** {@inheritDoc} */ + public function get(): mixed + { + return $this->value; + } + } + + /** + * @template InstanceType of object + * @template-extends AbstractPluginManager + */ + abstract class AbstractSingleInstancePluginManager extends AbstractPluginManager + { + /** + * An object type that the created instance must be instanced of + * + * @var class-string + */ + protected string $instanceOf; + + /** {@inheritDoc} */ + public function get(): object + { + return parent::get(); + } + + + /** {@inheritDoc} */ + public function validate(mixed $value): void + { + } + } + } + + namespace Namespace2 { + use Namespace1\AbstractSingleInstancePluginManager; + use stdClass; + + /** @template-extends AbstractSingleInstancePluginManager */ + final class Qoo extends AbstractSingleInstancePluginManager + { + /** @var class-string */ + protected string $instanceOf = stdClass::class; + } + } + + namespace { + $baz = new \Namespace2\Qoo(new stdClass); + + /** @var mixed $object */ + $object = null; + $baz->validate($object); + } + ', + 'assertions' => [ + '$object===' => 'stdClass', + ], + 'ignored_issues' => [], + 'php_version' => '8.1', + ], ]; } From 2a0ce2feccefbefea122b40400d404a6b942cdc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Mon, 28 Aug 2023 01:15:49 +0200 Subject: [PATCH 2/3] qa: add newlines to prevent exceeding 120 character line-length limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../Call/Method/ExistingAtomicMethodCallAnalyzer.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php index 15afcdc7b93..eafd740584f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -778,7 +778,9 @@ private static function modifyAssertionsForInheritance( $replacement->rule[] = new Assertion\IsType(new TTemplateParam( $assertion->type->param_name, - reset($potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name]), + reset( + $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], + ), $potential_assertion_providing_class, )); From 66c01813c1630e49a642560f82159b6a0be11e1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Mon, 28 Aug 2023 11:09:23 +0200 Subject: [PATCH 3/3] refactor: move assertion detection based on inherited classes/interfaces into internal resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This also enables `ExistingAtomicStaticCallAnalyzer` to detect those inherited assertions. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- .../ExistingAtomicMethodCallAnalyzer.php | 104 +------------ .../ExistingAtomicStaticCallAnalyzer.php | 11 +- .../AssertionsFromInheritanceResolver.php | 140 ++++++++++++++++++ 3 files changed, 152 insertions(+), 103 deletions(-) create mode 100644 src/Psalm/Internal/Codebase/AssertionsFromInheritanceResolver.php diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php index eafd740584f..35fda473155 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -15,6 +15,7 @@ use Psalm\Internal\Analyzer\Statements\Expression\ExpressionIdentifier; use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Internal\Analyzer\TraitAnalyzer; +use Psalm\Internal\Codebase\AssertionsFromInheritanceResolver; use Psalm\Internal\Codebase\InternalCallMapHandler; use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\MethodIdentifier; @@ -34,8 +35,6 @@ use Psalm\IssueBuffer; use Psalm\Node\Expr\VirtualFuncCall; use Psalm\Plugin\EventHandler\Event\AfterMethodCallAnalysisEvent; -use Psalm\Storage\Assertion; -use Psalm\Storage\ClassLikeStorage; use Psalm\Storage\Possibilities; use Psalm\Type; use Psalm\Type\Atomic; @@ -46,13 +45,10 @@ use function array_filter; use function array_map; -use function array_merge; -use function array_values; use function count; use function explode; use function in_array; use function is_string; -use function reset; use function strpos; use function strtolower; @@ -420,39 +416,8 @@ public static function analyze( } } - $assertions = $method_storage->assertions; - if ($method_storage->inheritdoc) { - $inherited_classes_and_interfaces = array_values(array_filter(array_merge( - $class_storage->parent_classes, - $class_storage->class_implements, - ), fn(string $classOrInterface) => $codebase->classOrInterfaceOrEnumExists($classOrInterface))); - - foreach ($inherited_classes_and_interfaces as $potential_assertion_providing_class) { - $potential_assertion_providing_classlike_storage = $codebase->classlike_storage_provider->get( - $potential_assertion_providing_class, - ); - if (!isset($potential_assertion_providing_classlike_storage->methods[$method_name_lc])) { - continue; - } - - $potential_assertion_providing_method_storage = $potential_assertion_providing_classlike_storage - ->methods[$method_name_lc]; - - /** - * Since the inheritance does not provide its own assertions, we have to detect those - * from inherited classes - */ - $assertions += array_map( - static fn(Possibilities $possibilities) => self::modifyAssertionsForInheritance( - $possibilities, - $codebase, - $class_storage, - $inherited_classes_and_interfaces, - ), - $potential_assertion_providing_method_storage->assertions, - ); - } - } + $assertionsResolver = new AssertionsFromInheritanceResolver($codebase); + $assertions = $assertionsResolver->resolve($method_storage, $class_storage); if ($assertions) { self::applyAssertionsToContext( @@ -729,67 +694,4 @@ private static function getMagicGetterOrSetterProperty( return null; } - - /** - * In case the called class is either implementing or extending a class/interface which does also has the - * template we are searching for, we assume that the called method has the same assertions. - * - * @param list $potential_assertion_providing_classes - */ - private static function modifyAssertionsForInheritance( - Possibilities $possibilities, - Codebase $codebase, - ClassLikeStorage $called_class, - array $potential_assertion_providing_classes - ): Possibilities { - $replacement = new Possibilities($possibilities->var_id, []); - $extended_params = $called_class->template_extended_params; - foreach ($possibilities->rule as $assertion) { - if (!$assertion instanceof Assertion\IsType - || !$assertion->type instanceof TTemplateParam) { - $replacement->rule[] = $assertion; - continue; - } - - /** Called class does not extend the template parameter */ - $extended_templates = $called_class->template_extended_params; - if (!isset($extended_templates[$assertion->type->defining_class][$assertion->type->param_name])) { - $replacement->rule[] = $assertion; - continue; - } - - foreach ($potential_assertion_providing_classes as $potential_assertion_providing_class) { - if (!isset($extended_params[$potential_assertion_providing_class][$assertion->type->param_name])) { - continue; - } - - if (!$codebase->classlike_storage_provider->has($potential_assertion_providing_class)) { - continue; - } - - $potential_assertion_providing_classlike_storage = $codebase->classlike_storage_provider->get( - $potential_assertion_providing_class, - ); - if (!isset( - $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], - )) { - continue; - } - - $replacement->rule[] = new Assertion\IsType(new TTemplateParam( - $assertion->type->param_name, - reset( - $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], - ), - $potential_assertion_providing_class, - )); - - continue 2; - } - - $replacement->rule[] = $assertion; - } - - return $replacement; - } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticMethod/ExistingAtomicStaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticMethod/ExistingAtomicStaticCallAnalyzer.php index 607f521881a..a8c916534ea 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticMethod/ExistingAtomicStaticCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticMethod/ExistingAtomicStaticCallAnalyzer.php @@ -15,6 +15,7 @@ use Psalm\Internal\Analyzer\Statements\Expression\Call\StaticCallAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\CallAnalyzer; use Psalm\Internal\Analyzer\StatementsAnalyzer; +use Psalm\Internal\Codebase\AssertionsFromInheritanceResolver; use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\Comparator\UnionTypeComparator; @@ -317,11 +318,17 @@ public static function analyze( } } - if ($method_storage->assertions) { + $assertionsResolver = new AssertionsFromInheritanceResolver($codebase); + $assertions = $assertionsResolver->resolve( + $method_storage, + $class_storage, + ); + + if ($assertions) { CallAnalyzer::applyAssertionsToContext( $stmt_name, null, - $method_storage->assertions, + $assertions, $stmt->getArgs(), $template_result, $context, diff --git a/src/Psalm/Internal/Codebase/AssertionsFromInheritanceResolver.php b/src/Psalm/Internal/Codebase/AssertionsFromInheritanceResolver.php new file mode 100644 index 00000000000..489c88f3644 --- /dev/null +++ b/src/Psalm/Internal/Codebase/AssertionsFromInheritanceResolver.php @@ -0,0 +1,140 @@ +codebase = $codebase; + } + + /** + * @return array + */ + public function resolve( + MethodStorage $method_storage, + ClassLikeStorage $called_class + ): array { + $method_name_lc = strtolower($method_storage->cased_name ?? ''); + + $assertions = $method_storage->assertions; + $inherited_classes_and_interfaces = array_values(array_filter(array_merge( + $called_class->parent_classes, + $called_class->class_implements, + ), fn(string $classOrInterface) => $this->codebase->classOrInterfaceOrEnumExists($classOrInterface))); + + foreach ($inherited_classes_and_interfaces as $potential_assertion_providing_class) { + $potential_assertion_providing_classlike_storage = $this->codebase->classlike_storage_provider->get( + $potential_assertion_providing_class, + ); + if (!isset($potential_assertion_providing_classlike_storage->methods[$method_name_lc])) { + continue; + } + + $potential_assertion_providing_method_storage = $potential_assertion_providing_classlike_storage + ->methods[$method_name_lc]; + + /** + * Since the inheritance does not provide its own assertions, we have to detect those + * from inherited classes + */ + $assertions += array_map( + fn(Possibilities $possibilities) => $this->modifyAssertionsForInheritance( + $possibilities, + $this->codebase, + $called_class, + $inherited_classes_and_interfaces, + ), + $potential_assertion_providing_method_storage->assertions, + ); + } + + return $assertions; + } + + /** + * In case the called class is either implementing or extending a class/interface which does also has the + * template we are searching for, we assume that the called method has the same assertions. + * + * @param list $potential_assertion_providing_classes + */ + private function modifyAssertionsForInheritance( + Possibilities $possibilities, + Codebase $codebase, + ClassLikeStorage $called_class, + array $potential_assertion_providing_classes + ): Possibilities { + $replacement = new Possibilities($possibilities->var_id, []); + $extended_params = $called_class->template_extended_params; + foreach ($possibilities->rule as $assertion) { + if (!$assertion instanceof IsType + || !$assertion->type instanceof TTemplateParam) { + $replacement->rule[] = $assertion; + continue; + } + + /** Called class does not extend the template parameter */ + $extended_templates = $called_class->template_extended_params; + if (!isset($extended_templates[$assertion->type->defining_class][$assertion->type->param_name])) { + $replacement->rule[] = $assertion; + continue; + } + + foreach ($potential_assertion_providing_classes as $potential_assertion_providing_class) { + if (!isset($extended_params[$potential_assertion_providing_class][$assertion->type->param_name])) { + continue; + } + + if (!$codebase->classlike_storage_provider->has($potential_assertion_providing_class)) { + continue; + } + + $potential_assertion_providing_classlike_storage = $codebase->classlike_storage_provider->get( + $potential_assertion_providing_class, + ); + if (!isset( + $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], + )) { + continue; + } + + $replacement->rule[] = new IsType(new TTemplateParam( + $assertion->type->param_name, + reset( + $potential_assertion_providing_classlike_storage->template_types[$assertion->type->param_name], + ), + $potential_assertion_providing_class, + )); + + continue 2; + } + + $replacement->rule[] = $assertion; + } + + return $replacement; + } +}