From d8654b8389257c347ad4405d155d3c95bcaef795 Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Fri, 30 Jun 2017 01:24:45 -0400 Subject: [PATCH] Slow down Psalm by checking parent initialisations too --- src/Psalm/Checker/ClassLikeChecker.php | 42 +++++++++------- src/Psalm/Checker/FileChecker.php | 33 ++++++------ .../Expression/AssignmentChecker.php | 3 +- .../Statements/Expression/CallChecker.php | 43 ++++++++++++++-- src/Psalm/Checker/TraitChecker.php | 1 + src/Psalm/Storage/ClassLikeStorage.php | 5 ++ src/Psalm/Type.php | 2 +- tests/ClassScopeTest.php | 2 +- tests/PropertyTypeTest.php | 50 +++++++++++++++++++ 9 files changed, 143 insertions(+), 38 deletions(-) diff --git a/src/Psalm/Checker/ClassLikeChecker.php b/src/Psalm/Checker/ClassLikeChecker.php index cd794e717..85a444823 100644 --- a/src/Psalm/Checker/ClassLikeChecker.php +++ b/src/Psalm/Checker/ClassLikeChecker.php @@ -563,10 +563,6 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc } foreach ($storage->appearing_property_ids as $property_name => $appearing_property_id) { - if (explode('::', $appearing_property_id)[0] !== $fq_class_name) { - continue; - } - $property_class_name = self::getDeclaringClassForProperty($appearing_property_id); $property_class_storage = self::$storage[strtolower((string)$property_class_name)]; $property_class_name = self::getDeclaringClassForProperty($appearing_property_id); @@ -662,10 +658,6 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc $uninitialized_properties = []; foreach ($storage->appearing_property_ids as $property_name => $appearing_property_id) { - if (explode('::', $appearing_property_id)[0] !== $fq_class_name) { - continue; - } - $property_class_name = self::getDeclaringClassForProperty($appearing_property_id); $property_class_storage = self::$storage[strtolower((string)$property_class_name)]; $property_class_name = self::getDeclaringClassForProperty($appearing_property_id); @@ -684,7 +676,10 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc } } - if ($uninitialized_properties && !($this instanceof TraitChecker)) { + if ($uninitialized_properties + && !($this instanceof TraitChecker) + && !$storage->abstract + ) { if ($constructor_checker) { $method_context = clone $class_context; $method_context->collect_initializations = true; @@ -842,14 +837,6 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc $method_name, Context $context ) { - foreach (self::$storage[strtolower($this->fq_class_name)]->properties as $property_name => $property_storage) { - if (!isset($context->vars_in_scope['$this->' . $property_name]) && - $property_storage->type !== false - ) { - $context->vars_in_scope['$this->' . $property_name] = clone $property_storage->type; - } - } - foreach ($this->class->stmts as $stmt) { if ($stmt instanceof PhpParser\Node\Stmt\ClassMethod && strtolower($stmt->name) === strtolower($method_name) @@ -1696,16 +1683,37 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc // register where they appear (can never be in a trait) foreach ($parent_storage->appearing_property_ids as $property_name => $appearing_property_id) { + if (!$parent_storage->is_trait + && isset($parent_storage->properties[$property_name]) + && $parent_storage->properties[$property_name]->visibility === self::VISIBILITY_PRIVATE + ) { + continue; + } + $storage->appearing_property_ids[$property_name] = $appearing_property_id; } // register where they're declared foreach ($parent_storage->declaring_property_ids as $property_name => $declaring_property_id) { + if (!$parent_storage->is_trait + && isset($parent_storage->properties[$property_name]) + && $parent_storage->properties[$property_name]->visibility === self::VISIBILITY_PRIVATE + ) { + continue; + } + $storage->declaring_property_ids[$property_name] = $declaring_property_id; } // register where they're declared foreach ($parent_storage->inheritable_property_ids as $property_name => $inheritable_property_id) { + if (!$parent_storage->is_trait + && isset($parent_storage->properties[$property_name]) + && $parent_storage->properties[$property_name]->visibility === self::VISIBILITY_PRIVATE + ) { + continue; + } + $storage->inheritable_property_ids[$property_name] = $inheritable_property_id; } } diff --git a/src/Psalm/Checker/FileChecker.php b/src/Psalm/Checker/FileChecker.php index 0898eb222..11b7f5aa0 100644 --- a/src/Psalm/Checker/FileChecker.php +++ b/src/Psalm/Checker/FileChecker.php @@ -352,8 +352,25 @@ class FileChecker extends SourceChecker implements StatementsSource public function getMethodMutations($method_id, Context &$this_context) { list($fq_class_name, $method_name) = explode('::', $method_id); - $call_context = new Context((string)array_values($this_context->vars_in_scope['$this']->types)[0]); + + $class_checker_to_examine = null; + + foreach ($this->class_checkers_to_analyze as $class_checker) { + if (strtolower($class_checker->getFQCLN()) === strtolower($fq_class_name)) { + $class_checker_to_examine = $class_checker; + break; + } + } + + if (!$class_checker_to_examine) { + $this->project_checker->getMethodMutations($method_id, $this_context); + + return; + } + + $call_context = new Context($this_context->self); $call_context->collect_mutations = true; + $call_context->include_location = $this_context->include_location; foreach ($this_context->vars_possibly_in_scope as $var => $type) { if (strpos($var, '$this->') === 0) { @@ -369,19 +386,7 @@ class FileChecker extends SourceChecker implements StatementsSource $call_context->vars_in_scope['$this'] = $this_context->vars_in_scope['$this']; - $checked = false; - - foreach ($this->class_checkers_to_analyze as $class_checker) { - if (strtolower($class_checker->getFQCLN()) === strtolower($fq_class_name)) { - $class_checker->getMethodMutations($method_name, $call_context); - $checked = true; - break; - } - } - - if (!$checked) { - throw new \UnexpectedValueException('Method ' . $method_id . ' could not be checked'); - } + $class_checker_to_examine->getMethodMutations($method_name, $call_context); foreach ($call_context->vars_possibly_in_scope as $var => $_) { $this_context->vars_possibly_in_scope[$var] = true; diff --git a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php index 3bb4ea333..d58cc99db 100644 --- a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php +++ b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php @@ -770,7 +770,8 @@ class AssignmentChecker $assignment_value_type . '\'', new CodeLocation( $statements_checker->getSource(), - $assignment_value ?: $stmt + $assignment_value ?: $stmt, + $context->include_location ) ), $statements_checker->getSuppressedIssues() diff --git a/src/Psalm/Checker/Statements/Expression/CallChecker.php b/src/Psalm/Checker/Statements/Expression/CallChecker.php index a7bef56a2..39cbe63af 100644 --- a/src/Psalm/Checker/Statements/Expression/CallChecker.php +++ b/src/Psalm/Checker/Statements/Expression/CallChecker.php @@ -1123,12 +1123,47 @@ class CallChecker return; } - if (is_string($stmt->name)) { - if ($context->collect_mutations) { - $method_id = $fq_class_name . '::' . strtolower($stmt->name); + $class_storage = ClassLikeChecker::$storage[strtolower($fq_class_name)]; - $file_checker->project_checker->getMethodMutations($method_id, $context); + if (is_string($stmt->name) && $class_storage->user_defined) { + $method_id = $fq_class_name . '::' . strtolower($stmt->name); + + $old_context_include_location = $context->include_location; + $old_self = $context->self; + $context->include_location = new CodeLocation($statements_checker->getSource(), $stmt); + $context->self = $fq_class_name; + + if ($context->collect_mutations) { + $file_checker->getMethodMutations($method_id, $context); + } elseif ($context->collect_initializations) { + $local_vars_in_scope = []; + $local_vars_possibly_in_scope = []; + + foreach ($context->vars_in_scope as $var => $type) { + if (strpos($var, '$this->') !== 0 && $var !== '$this') { + $local_vars_in_scope[$var] = $context->vars_in_scope[$var]; + } + } + + foreach ($context->vars_possibly_in_scope as $var => $type) { + if (strpos($var, '$this->') !== 0 && $var !== '$this') { + $local_vars_possibly_in_scope[$var] = $context->vars_possibly_in_scope[$var]; + } + } + + $file_checker->getMethodMutations($method_id, $context); + + foreach ($local_vars_in_scope as $var => $type) { + $context->vars_in_scope[$var] = $type; + } + + foreach ($local_vars_possibly_in_scope as $var => $type) { + $context->vars_possibly_in_scope[$var] = $type; + } } + + $context->include_location = $old_context_include_location; + $context->self = $old_self; } } else { $namespace = $statements_checker->getNamespace() diff --git a/src/Psalm/Checker/TraitChecker.php b/src/Psalm/Checker/TraitChecker.php index 78f6977c4..b990950ab 100644 --- a/src/Psalm/Checker/TraitChecker.php +++ b/src/Psalm/Checker/TraitChecker.php @@ -38,6 +38,7 @@ class TraitChecker extends ClassLikeChecker if (!isset(self::$storage[$fq_class_name_lower])) { self::$storage[$fq_class_name_lower] = $storage = new ClassLikeStorage(); $storage->name = $fq_class_name; + $storage->is_trait = true; $storage->location = new CodeLocation($this->source, $class, null, true); self::$file_classes[$this->source->getFilePath()][] = $fq_class_name; diff --git a/src/Psalm/Storage/ClassLikeStorage.php b/src/Psalm/Storage/ClassLikeStorage.php index 5196a5cba..3dc84497d 100644 --- a/src/Psalm/Storage/ClassLikeStorage.php +++ b/src/Psalm/Storage/ClassLikeStorage.php @@ -95,6 +95,11 @@ class ClassLikeStorage */ public $used_traits = []; + /** + * @var bool + */ + public $is_trait = false; + /** * @var array */ diff --git a/src/Psalm/Type.php b/src/Psalm/Type.php index 5bdb605f2..f8e1e37ba 100644 --- a/src/Psalm/Type.php +++ b/src/Psalm/Type.php @@ -595,7 +595,7 @@ abstract class Type $type_key = $type->getKey(); if ($type instanceof TArray || $type instanceof TGenericObject) { - for ($i = 0; $i < count($type->type_params); $i++) { + for ($i = 0; $i < count($type->type_params); ++$i) { $type_param = $type->type_params[$i]; if (isset($combination->type_params[$type_key][$i])) { diff --git a/tests/ClassScopeTest.php b/tests/ClassScopeTest.php index 039519c5b..0468cfd64 100644 --- a/tests/ClassScopeTest.php +++ b/tests/ClassScopeTest.php @@ -230,7 +230,7 @@ class ClassScopeTest extends TestCase echo $this->fooFoo; } }', - 'error_message' => 'InaccessibleProperty', + 'error_message' => 'UndefinedThisPropertyFetch', ], 'inaccessibleStaticPrivateProperty' => [ ' [ + ' [ + 'foo = ""; + } + } + + class B extends A { + public function __construct() { + parent::__construct(); + } + }', + ], + 'SKIPPED-abstractClassConstructorAndImplicitChildConstructor' => [ + 'foo = ""; + } + } + + class B extends A {}', + ], ]; } @@ -714,6 +751,19 @@ class PropertyTypeTest extends TestCase }', 'error_message' => 'UndefinedClass', ], + 'abstractClassWithNoConstructorButChild' => [ + ' 'PropertyNotSetInConstructor', + ], ]; } }