From 94428057634acc527efdc43043b84835e1f82eb6 Mon Sep 17 00:00:00 2001 From: Brown Date: Wed, 6 Mar 2019 11:12:36 -0500 Subject: [PATCH] Mutation checks should not care about return type --- .../Analyzer/FunctionLikeAnalyzer.php | 32 +-- .../Analyzer/Statements/ReturnAnalyzer.php | 5 +- tests/MethodMutationTest.php | 221 +++++++++++------- 3 files changed, 150 insertions(+), 108 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index 2d87dc0bc..3cfdde3d8 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -62,7 +62,7 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements protected $source; /** - * @var array> + * @var ?array */ protected $return_vars_in_scope = []; @@ -72,7 +72,7 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements protected $possible_param_types = []; /** - * @var array> + * @var ?array */ protected $return_vars_possibly_in_scope = []; @@ -582,7 +582,7 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements return false; } - if ($context->collect_initializations) { + if ($context->collect_initializations || $context->collect_mutations) { $statements_analyzer->addSuppressedIssues([ 'DocblockTypeContradiction', 'InvalidReturnStatement', @@ -854,17 +854,17 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements } if ($add_mutations) { - if (isset($this->return_vars_in_scope[''])) { + if ($this->return_vars_in_scope !== null) { $context->vars_in_scope = TypeAnalyzer::combineKeyedTypes( $context->vars_in_scope, - $this->return_vars_in_scope[''] + $this->return_vars_in_scope ); } - if (isset($this->return_vars_possibly_in_scope[''])) { + if ($this->return_vars_possibly_in_scope !== null) { $context->vars_possibly_in_scope = array_merge( $context->vars_possibly_in_scope, - $this->return_vars_possibly_in_scope[''] + $this->return_vars_possibly_in_scope ); } @@ -967,24 +967,24 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements * * @return void */ - public function addReturnTypes($return_type, Context $context) + public function addReturnTypes(Context $context) { - if (isset($this->return_vars_in_scope[$return_type])) { - $this->return_vars_in_scope[$return_type] = TypeAnalyzer::combineKeyedTypes( + if ($this->return_vars_in_scope !== null) { + $this->return_vars_in_scope = TypeAnalyzer::combineKeyedTypes( $context->vars_in_scope, - $this->return_vars_in_scope[$return_type] + $this->return_vars_in_scope ); } else { - $this->return_vars_in_scope[$return_type] = $context->vars_in_scope; + $this->return_vars_in_scope = $context->vars_in_scope; } - if (isset($this->return_vars_possibly_in_scope[$return_type])) { - $this->return_vars_possibly_in_scope[$return_type] = array_merge( + if ($this->return_vars_possibly_in_scope !== null) { + $this->return_vars_possibly_in_scope = array_merge( $context->vars_possibly_in_scope, - $this->return_vars_possibly_in_scope[$return_type] + $this->return_vars_possibly_in_scope ); } else { - $this->return_vars_possibly_in_scope[$return_type] = $context->vars_possibly_in_scope; + $this->return_vars_possibly_in_scope = $context->vars_possibly_in_scope; } } diff --git a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php index e1ab15f28..7d38c7769 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php @@ -119,10 +119,7 @@ class ReturnAnalyzer if ($source instanceof FunctionLikeAnalyzer && !($source->getSource() instanceof TraitAnalyzer) ) { - $source->addReturnTypes( - $stmt->expr ? (string) $stmt->inferredType : '', - $context - ); + $source->addReturnTypes($context); $source->examineParamTypes($statements_analyzer, $context, $codebase, $stmt); diff --git a/tests/MethodMutationTest.php b/tests/MethodMutationTest.php index 50db896fa..97dd36f6f 100644 --- a/tests/MethodMutationTest.php +++ b/tests/MethodMutationTest.php @@ -14,79 +14,79 @@ class MethodMutationTest extends TestCase $this->addFile( 'somefile.php', 'name = $name; - } + /** + * @param string $name + */ + protected function __construct($name) { + $this->name = $name; + } - /** @return User|null */ - public static function loadUser(int $id) { - if ($id === 3) { - $user = new User("bob"); - return $user; + /** @return User|null */ + public static function loadUser(int $id) { + if ($id === 3) { + $user = new User("bob"); + return $user; + } + + return null; + } } - return null; - } - } - - class UserViewData { - /** @var string|null */ - public $name; - } - - class Response { - public function __construct (UserViewData $viewdata) {} - } - - class UnauthorizedException extends Exception { } - - class Controller { - /** @var UserViewData */ - public $user_viewdata; - - /** @var string|null */ - public $title; - - public function __construct() { - $this->user_viewdata = new UserViewData(); - } - - public function setUser(): void - { - $user_id = (int)$_GET["id"]; - - if (!$user_id) { - throw new UnauthorizedException("No user id supplied"); + class UserViewData { + /** @var string|null */ + public $name; } - $user = User::loadUser($user_id); - - if (!$user) { - throw new UnauthorizedException("User not found"); + class Response { + public function __construct (UserViewData $viewdata) {} } - $this->user_viewdata->name = $user->name; - } - } + class UnauthorizedException extends Exception { } - class FooController extends Controller { - public function barBar(): Response { - $this->setUser(); + class Controller { + /** @var UserViewData */ + public $user_viewdata; - if (rand(0, 1)) { - $this->title = "hello"; + /** @var string|null */ + public $title; + + public function __construct() { + $this->user_viewdata = new UserViewData(); + } + + public function setUser(): void + { + $user_id = (int)$_GET["id"]; + + if (!$user_id) { + throw new UnauthorizedException("No user id supplied"); + } + + $user = User::loadUser($user_id); + + if (!$user) { + throw new UnauthorizedException("User not found"); + } + + $this->user_viewdata->name = $user->name; + } } - return new Response($this->user_viewdata); - } - }' + class FooController extends Controller { + public function barBar(): Response { + $this->setUser(); + + if (rand(0, 1)) { + $this->title = "hello"; + } + + return new Response($this->user_viewdata); + } + }' ); new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); @@ -106,6 +106,51 @@ class MethodMutationTest extends TestCase $this->assertTrue($method_context->vars_possibly_in_scope['$this->title']); } + /** + * @return void + */ + public function testNotSettingUser() + { + $this->addFile( + 'somefile.php', + 'user) { + return []; + } + + return ["hello"]; + } + + public function barBar(): void { + $this->user = rand(0, 1) ? new User() : null; + + $this->doThingWithUser(); + } + }' + ); + + new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); + $this->project_analyzer->getCodebase()->scanFiles(); + $method_context = new Context(); + $method_context->collect_mutations = true; + $this->project_analyzer->getMethodMutations( + 'FooController::barBar', + $method_context, + 'somefile.php', + 'somefile.php' + ); + + $this->assertSame('null|User', (string)$method_context->vars_in_scope['$this->user']); + } + /** * @return void */ @@ -114,22 +159,22 @@ class MethodMutationTest extends TestCase $this->addFile( 'somefile.php', 'foo = new Foo(); - } - } + public function __construct() { + $this->foo = new Foo(); + } + } - class FooController extends Controller { - public function __construct() { - parent::__construct(); - } - }' + class FooController extends Controller { + public function __construct() { + parent::__construct(); + } + }' ); new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); @@ -154,24 +199,24 @@ class MethodMutationTest extends TestCase $this->addFile( 'somefile.php', 'foo = new Foo(); - } - } + trait T { + private function setFoo(): void { + $this->foo = new Foo(); + } + } - class FooController { - use T; + class FooController { + use T; - /** @var Foo|null */ - public $foo; + /** @var Foo|null */ + public $foo; - public function __construct() { - $this->setFoo(); - } - }' + public function __construct() { + $this->setFoo(); + } + }' ); new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php');