From bc35f888597e23acb04848b8375199942a0c0302 Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Fri, 23 Jun 2017 00:39:37 -0400 Subject: [PATCH] Fix issue with $this instanceof checks in traits --- .../Checker/Statements/Block/IfChecker.php | 14 +-- .../Checker/Statements/Block/WhileChecker.php | 4 +- .../Expression/AssignmentChecker.php | 2 +- .../Statements/Expression/CallChecker.php | 2 +- .../Checker/Statements/ExpressionChecker.php | 18 ++-- src/Psalm/Checker/TypeChecker.php | 18 ++-- src/Psalm/Context.php | 16 ++-- tests/TraitTest.php | 92 +++++++++++-------- tests/TypeReconciliationTest.php | 7 +- 9 files changed, 97 insertions(+), 76 deletions(-) diff --git a/src/Psalm/Checker/Statements/Block/IfChecker.php b/src/Psalm/Checker/Statements/Block/IfChecker.php index 229b48c81..c8cad87b9 100644 --- a/src/Psalm/Checker/Statements/Block/IfChecker.php +++ b/src/Psalm/Checker/Statements/Block/IfChecker.php @@ -118,7 +118,7 @@ class IfChecker $reconcilable_if_types, $if_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->cond, $context->include_location), $statements_checker->getSuppressedIssues() ); @@ -156,7 +156,7 @@ class IfChecker $if_scope->negated_types, $temp_else_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->cond, $context->include_location), $statements_checker->getSuppressedIssues() ); @@ -349,7 +349,7 @@ class IfChecker $if_scope->negated_types, $outer_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation( $statements_checker->getSource(), $stmt->cond, @@ -441,7 +441,7 @@ class IfChecker $if_scope->negated_types, $elseif_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation( $statements_checker->getSource(), $elseif->cond, @@ -516,7 +516,7 @@ class IfChecker $reconcilable_elseif_types, $elseif_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $elseif->cond, $outer_context->include_location), $statements_checker->getSuppressedIssues() ); @@ -634,7 +634,7 @@ class IfChecker $negated_elseif_types, $pre_conditional_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $elseif, $outer_context->include_location), $statements_checker->getSuppressedIssues() ); @@ -757,7 +757,7 @@ class IfChecker $else_types, $else_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $else, $outer_context->include_location), $statements_checker->getSuppressedIssues() ); diff --git a/src/Psalm/Checker/Statements/Block/WhileChecker.php b/src/Psalm/Checker/Statements/Block/WhileChecker.php index bafb9586d..729e06db6 100644 --- a/src/Psalm/Checker/Statements/Block/WhileChecker.php +++ b/src/Psalm/Checker/Statements/Block/WhileChecker.php @@ -58,7 +58,7 @@ class WhileChecker $reconcilable_while_types, $while_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->cond), $statements_checker->getSuppressedIssues() ); @@ -101,7 +101,7 @@ class WhileChecker $negated_while_types, $context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->cond), $statements_checker->getSuppressedIssues() ); diff --git a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php index b51c11ad7..cee3f7a4a 100644 --- a/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php +++ b/src/Psalm/Checker/Statements/Expression/AssignmentChecker.php @@ -125,7 +125,7 @@ class AssignmentChecker $array_var_id, $context->vars_in_scope[$array_var_id], $assign_value_type, - $statements_checker->getFileChecker() + $statements_checker ); } diff --git a/src/Psalm/Checker/Statements/Expression/CallChecker.php b/src/Psalm/Checker/Statements/Expression/CallChecker.php index bd974c8bb..a7bef56a2 100644 --- a/src/Psalm/Checker/Statements/Expression/CallChecker.php +++ b/src/Psalm/Checker/Statements/Expression/CallChecker.php @@ -396,7 +396,7 @@ class CallChecker $assert_type_assertions, $context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt), $statements_checker->getSuppressedIssues() ); diff --git a/src/Psalm/Checker/Statements/ExpressionChecker.php b/src/Psalm/Checker/Statements/ExpressionChecker.php index 7de187e58..05a923628 100644 --- a/src/Psalm/Checker/Statements/ExpressionChecker.php +++ b/src/Psalm/Checker/Statements/ExpressionChecker.php @@ -634,7 +634,7 @@ class ExpressionChecker $var_id, $existing_type, $by_ref_type, - $statements_checker->getFileChecker() + $statements_checker ); if ((string)$existing_type !== 'array') { @@ -790,7 +790,7 @@ class ExpressionChecker $left_type_assertions, $context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt), $statements_checker->getSuppressedIssues() ); @@ -862,7 +862,7 @@ class ExpressionChecker $negated_type_assertions, $context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt), $statements_checker->getSuppressedIssues() ); @@ -896,7 +896,7 @@ class ExpressionChecker '!empty', $context->vars_in_scope[$var_id], '', - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->left), $statements_checker->getSuppressedIssues() ); @@ -953,7 +953,7 @@ class ExpressionChecker $reconcilable_if_types, $t_if_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->left), $statements_checker->getSuppressedIssues() ); @@ -990,7 +990,7 @@ class ExpressionChecker $negated_if_types, $t_else_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->right), $statements_checker->getSuppressedIssues() ); @@ -1770,7 +1770,7 @@ class ExpressionChecker $reconcilable_if_types, $t_if_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->cond), $statements_checker->getSuppressedIssues() ); @@ -1806,7 +1806,7 @@ class ExpressionChecker $negated_if_types, $t_else_context->vars_in_scope, $changed_vars, - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt->else), $statements_checker->getSuppressedIssues() ); @@ -1847,7 +1847,7 @@ class ExpressionChecker '!empty', $stmt->cond->inferredType, '', - $statements_checker->getFileChecker(), + $statements_checker, new CodeLocation($statements_checker->getSource(), $stmt), $statements_checker->getSuppressedIssues() ); diff --git a/src/Psalm/Checker/TypeChecker.php b/src/Psalm/Checker/TypeChecker.php index 41bfbe7e9..d4ea122fb 100644 --- a/src/Psalm/Checker/TypeChecker.php +++ b/src/Psalm/Checker/TypeChecker.php @@ -34,7 +34,7 @@ class TypeChecker * @param array $new_types * @param array $existing_types * @param array $changed_types - * @param FileChecker $file_checker + * @param StatementsChecker $statements_checker * @param CodeLocation $code_location * @param array $suppressed_issues * @@ -44,7 +44,7 @@ class TypeChecker array $new_types, array $existing_types, array &$changed_types, - FileChecker $file_checker, + StatementsChecker $statements_checker, CodeLocation $code_location, array $suppressed_issues = [] ) { @@ -75,7 +75,7 @@ class TypeChecker $result_type = isset($existing_types[$key]) ? clone $existing_types[$key] - : self::getValueForKey($key, $existing_types, $file_checker); + : self::getValueForKey($key, $existing_types, $statements_checker->getFileChecker()); if ($result_type && empty($result_type->types)) { throw new \InvalidArgumentException('Union::$types cannot be empty after get value for ' . $key); @@ -90,7 +90,7 @@ class TypeChecker (string) $new_type_part, $result_type, $key, - $file_checker, + $statements_checker, $code_location, $suppressed_issues, $failed_reconciliation @@ -136,7 +136,7 @@ class TypeChecker * @param string $new_var_type * @param Type\Union|null $existing_var_type * @param string|null $key - * @param FileChecker $file_checker + * @param StatementsChecker $statements_checker * @param CodeLocation $code_location * @param array $suppressed_issues * @param bool $failed_reconciliation if the types cannot be reconciled, we need to know @@ -147,11 +147,13 @@ class TypeChecker $new_var_type, $existing_var_type, $key, - FileChecker $file_checker, + StatementsChecker $statements_checker, CodeLocation $code_location = null, array $suppressed_issues = [], &$failed_reconciliation = false ) { + $file_checker = $statements_checker->getFileChecker(); + if ($existing_var_type === null) { if ($new_var_type === '^isset') { return null; @@ -249,7 +251,9 @@ class TypeChecker $existing_var_type->removeType($negated_type); if (empty($existing_var_type->types)) { - if (!$existing_var_type->from_docblock) { + if (!$existing_var_type->from_docblock + && ($key !== '$this' || !($statements_checker->getSource()->getSource() instanceof TraitChecker)) + ) { if ($key && $code_location) { if (IssueBuffer::accepts( new FailedTypeResolution('Cannot resolve types for ' . $key, $code_location), diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 1663c3aaf..582f0b465 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -1,7 +1,7 @@ possibilities[$remove_var_id] === [$new_type_string] ) { $clauses_to_keep[] = $clause; - } elseif ($file_checker && + } elseif ($statements_checker && $new_type && !$new_type->isMixed() ) { @@ -313,7 +313,7 @@ class Context $type, clone $new_type, null, - $file_checker, + $statements_checker, null, [], $failed_reconciliation @@ -342,7 +342,7 @@ class Context * @param string $remove_var_id * @param \Psalm\Type\Union|null $existing_type * @param \Psalm\Type\Union|null $new_type - * @param FileChecker|null $file_checker + * @param ?StatementsChecker $statements_checker * * @return void */ @@ -350,7 +350,7 @@ class Context $remove_var_id, Union $existing_type = null, Union $new_type = null, - FileChecker $file_checker = null + StatementsChecker $statements_checker = null ) { if (!$existing_type && isset($this->vars_in_scope[$remove_var_id])) { $existing_type = $this->vars_in_scope[$remove_var_id]; @@ -363,7 +363,7 @@ class Context $this->removeVarFromConflictingClauses( $remove_var_id, $existing_type->isMixed() ? null : $new_type, - $file_checker + $statements_checker ); if ($existing_type->hasArray() || $existing_type->isMixed()) { diff --git a/tests/TraitTest.php b/tests/TraitTest.php index 433bc73cb..810a09c30 100644 --- a/tests/TraitTest.php +++ b/tests/TraitTest.php @@ -18,10 +18,10 @@ class TraitTest extends TestCase private function fooFoo() : void { } } - + class B { use T; - + public function doFoo() : void { $this->fooFoo(); } @@ -33,10 +33,10 @@ class TraitTest extends TestCase protected function fooFoo() : void { } } - + class B { use T; - + public function doFoo() : void { $this->fooFoo(); } @@ -48,10 +48,10 @@ class TraitTest extends TestCase public function fooFoo() : void { } } - + class B { use T; - + public function doFoo() : void { $this->fooFoo(); } @@ -63,10 +63,10 @@ class TraitTest extends TestCase /** @var string */ private $fooFoo = ""; } - + class B { use T; - + public function doFoo() : void { echo $this->fooFoo; } @@ -78,10 +78,10 @@ class TraitTest extends TestCase /** @var string */ protected $fooFoo = ""; } - + class B { use T; - + public function doFoo() : void { echo $this->fooFoo; } @@ -93,10 +93,10 @@ class TraitTest extends TestCase /** @var string */ public $fooFoo = ""; } - + class B { use T; - + public function doFoo() : void { echo $this->fooFoo; } @@ -108,11 +108,11 @@ class TraitTest extends TestCase protected function fooFoo() : void { } } - + class B { use T; } - + class C extends B { public function doFoo() : void { $this->fooFoo(); @@ -125,11 +125,11 @@ class TraitTest extends TestCase public function fooFoo() : void { } } - + class B { use T; } - + class C extends B { public function doFoo() : void { $this->fooFoo(); @@ -143,12 +143,12 @@ class TraitTest extends TestCase self::barBar(); } } - + class B { use T; - + public static function barBar() : void { - + } }', ], @@ -158,14 +158,14 @@ class TraitTest extends TestCase public function fooFoo() : void { } } - + class B { use T; - + public function fooFoo(string $a) : void { } } - + (new B)->fooFoo("hello");', ], 'redefinedTraitMethodWithAlias' => [ @@ -174,12 +174,12 @@ class TraitTest extends TestCase public function fooFoo() : void { } } - + class B { use T { fooFoo as barBar; } - + public function fooFoo() : void { $this->barBar(); } @@ -193,11 +193,11 @@ class TraitTest extends TestCase return $this; } } - + class A { use T; } - + $a = (new A)->g();', 'assertions' => [ ['A' => '$a'], @@ -211,18 +211,18 @@ class TraitTest extends TestCase return $this; } } - + class A { use T; } - + class B extends A { } - + class C { use T; } - + $a = (new B)->g();', 'assertions' => [ ['A' => '$a'], @@ -236,7 +236,7 @@ class TraitTest extends TestCase } class A { use T; - + /** @return void */ public function bar() { T::foo(); @@ -249,16 +249,28 @@ class TraitTest extends TestCase /** @return void */ abstract public function foo(); } - + abstract class A { use T; - + /** @return void */ public function bar() { $this->foo(); } }', ], + 'instanceOfTraitUser' => [ + 'fooFoo(); @@ -300,7 +312,7 @@ class TraitTest extends TestCase } class A { use T; - + public function assignToFoo() : void { $this->foo = 5; } @@ -315,7 +327,7 @@ class TraitTest extends TestCase } class A { use T; - + public function __construct() : void { $this->foo = 5; } @@ -330,11 +342,11 @@ class TraitTest extends TestCase } class A { use T; - + public function __construct() : void { $this->foo = 5; } - + public function makeNull() : void { $this->foo = null; } @@ -349,7 +361,7 @@ class TraitTest extends TestCase } class A { use T; - + public function __construct() : void { $this->foo = 5; } diff --git a/tests/TypeReconciliationTest.php b/tests/TypeReconciliationTest.php index 3a38a0855..a057c93ad 100644 --- a/tests/TypeReconciliationTest.php +++ b/tests/TypeReconciliationTest.php @@ -3,6 +3,7 @@ namespace Psalm\Tests; use Psalm\Checker\AlgebraChecker; use Psalm\Checker\FileChecker; +use Psalm\Checker\StatementsChecker; use Psalm\Checker\TypeChecker; use Psalm\Clause; use Psalm\Context; @@ -16,6 +17,9 @@ class TypeReconciliationTest extends TestCase /** @var FileChecker */ protected $file_checker; + /** @var StatementsChecker */ + protected $statements_checker; + /** * @return void */ @@ -25,6 +29,7 @@ class TypeReconciliationTest extends TestCase $this->file_checker = new FileChecker('somefile.php', $this->project_checker); $this->file_checker->context = new Context(); + $this->statements_checker = new StatementsChecker($this->file_checker); } /** @@ -42,7 +47,7 @@ class TypeReconciliationTest extends TestCase $type, Type::parseString($string), null, - $this->file_checker + $this->statements_checker ); $this->assertSame(