Skip to content

Commit

Permalink
Fix #4537 - use more rigorous inerhitance for return and param types
Browse files Browse the repository at this point in the history
  • Loading branch information
@muglug
muglug committed Nov 12, 2020
1 parent 929efcc commit 3dd185e
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 89 deletions.
82 changes: 70 additions & 12 deletions src/Psalm/Internal/Codebase/Methods.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,36 +724,94 @@ public function getMethodReturnType(
return $return_type_candidate;
}

$class_storage = $this->classlike_storage_provider->get($appearing_fq_class_name);

$storage = $this->getStorage($declaring_method_id);

if ($storage->return_type) {
$self_class = $appearing_fq_class_storage->name;
$candidate_type = $storage->return_type;

return clone $storage->return_type;
if ($candidate_type && $candidate_type->isVoid()) {
return $candidate_type;
}

$class_storage = $this->classlike_storage_provider->get($appearing_fq_class_name);

if (!isset($class_storage->overridden_method_ids[$appearing_method_name])) {
return null;
}

$candidate_type = null;

if (isset($class_storage->documenting_method_ids[$appearing_method_name])) {
if (isset($class_storage->documenting_method_ids[$appearing_method_name]) && $source_analyzer) {
$overridden_method_id = $class_storage->documenting_method_ids[$appearing_method_name];

// special override to allow inference of Iterator types
if ($overridden_method_id->fq_class_name === 'Iterator'
&& $storage->return_type
&& $storage->return_type === $storage->signature_return_type
) {
return clone $storage->return_type;
}

$overridden_storage = $this->getStorage($overridden_method_id);

if ($overridden_storage->return_type) {
if ($overridden_storage->return_type->isNull()) {
return Type::getVoid();
}

if (!$candidate_type) {
$self_class = $overridden_method_id->fq_class_name;

return clone $overridden_storage->return_type;
}

$old_contained_by_new = UnionTypeComparator::isContainedBy(
$source_analyzer->getCodebase(),
$candidate_type,
$overridden_storage->return_type
);

$new_contained_by_old = UnionTypeComparator::isContainedBy(
$source_analyzer->getCodebase(),
$overridden_storage->return_type,
$candidate_type
);

if (!$old_contained_by_new && !$new_contained_by_old) {
$attempted_intersection = Type::intersectUnionTypes(
$overridden_storage->return_type,
$candidate_type,
$source_analyzer->getCodebase()
);

if ($attempted_intersection) {
$self_class = $overridden_method_id->fq_class_name;

return $attempted_intersection;
}

$self_class = $appearing_fq_class_storage->name;

return clone $candidate_type;
}

if ($old_contained_by_new) {
$self_class = $appearing_fq_class_storage->name;

return clone $candidate_type;
}

$self_class = $overridden_method_id->fq_class_name;

return clone $overridden_storage->return_type;
}
}

if ($candidate_type) {
$self_class = $appearing_fq_class_storage->name;

return clone $candidate_type;
}

if (!isset($class_storage->overridden_method_ids[$appearing_method_name])) {
return null;
}

$candidate_type = null;

foreach ($class_storage->overridden_method_ids[$appearing_method_name] as $overridden_method_id) {
$overridden_storage = $this->getStorage($overridden_method_id);

Expand Down
78 changes: 35 additions & 43 deletions src/Psalm/Internal/Codebase/Populator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,16 +309,42 @@ private function populateOverriddenMethods(

$declaring_method_storage = $declaring_class_storage->methods[$declaring_method_name];

if ($declaring_method_storage->has_docblock_param_types
if (($declaring_method_storage->has_docblock_param_types
|| $declaring_method_storage->return_type
!== $declaring_method_storage->signature_return_type)
&& !$method_storage->has_docblock_param_types
&& (!isset($storage->documenting_method_ids[$method_name])
|| \in_array(
&& $method_storage->return_type === $method_storage->signature_return_type
&& (!$declaring_method_storage->signature_return_type
|| ($method_storage->signature_return_type
&& $method_storage->signature_return_type->getId()
=== $declaring_method_storage->signature_return_type->getId()))
&& $method_storage->inherited_return_type !== null
) {
if (!isset($storage->documenting_method_ids[$method_name])) {
$storage->documenting_method_ids[$method_name] = $declaring_method_id;
$method_storage->inherited_return_type = true;
} else {
if (\in_array(
$storage->documenting_method_ids[$method_name]->fq_class_name,
$declaring_class_storage->parent_interfaces
)
)
) {
$storage->documenting_method_ids[$method_name] = $declaring_method_id;
) || $storage->documenting_method_ids[$method_name]->fq_class_name
=== $declaring_class_storage->name
) {
$storage->documenting_method_ids[$method_name] = $declaring_method_id;
$method_storage->inherited_return_type = true;
} else {
$documenting_class_storage = $declaring_class_storages
[$storage->documenting_method_ids[$method_name]->fq_class_name];

if (!\in_array(
$declaring_class,
$documenting_class_storage->parent_interfaces
)) {
unset($storage->documenting_method_ids[$method_name]);
$method_storage->inherited_return_type = null;
}
}
}
}

// tell the declaring class it's overridden downstream
Expand All @@ -336,40 +362,6 @@ private function populateOverriddenMethods(
) {
$method_storage->throws += $declaring_method_storage->throws;
}

if ((count($overridden_method_ids) === 1
|| $candidate_overridden_ids)
&& $method_storage->signature_return_type
&& !$method_storage->signature_return_type->isVoid()
&& ($method_storage->return_type === $method_storage->signature_return_type
|| $method_storage->inherited_return_type)
) {
if (isset($declaring_class_storage->methods[$method_name])) {
$declaring_method_storage = $declaring_class_storage->methods[$method_name];

if ($declaring_method_storage->return_type
&& $declaring_method_storage->return_type
!== $declaring_method_storage->signature_return_type
) {
if ($declaring_method_storage->signature_return_type
&& UnionTypeComparator::isSimplyContainedBy(
$method_storage->signature_return_type,
$declaring_method_storage->signature_return_type
)
) {
$method_storage->return_type = clone $declaring_method_storage->return_type;
$method_storage->inherited_return_type = true;
} elseif (UnionTypeComparator::isSimplyContainedBy(
$declaring_method_storage->return_type,
$method_storage->signature_return_type
)) {
$method_storage->return_type = clone $declaring_method_storage->return_type;
$method_storage->inherited_return_type = true;
$method_storage->return_type->from_docblock = false;
}
}
}
}
}
}
}
Expand Down Expand Up @@ -853,8 +845,8 @@ function ($constant) {
$method_storage->signature_return_type
)
) {
$method_storage->return_type = $interface_method_storage->return_type;
$method_storage->inherited_return_type = true;
//$method_storage->return_type = $interface_method_storage->return_type;
//$method_storage->inherited_return_type = true;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Storage/MethodStorage.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class MethodStorage extends FunctionLikeStorage
public $inheritdoc = false;

/**
* @var bool
* @var ?bool
*/
public $inherited_return_type = false;

Expand Down
46 changes: 46 additions & 0 deletions tests/DocblockInheritanceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,52 @@ public function aa() : array {
}
}'
],
'inheritCorrectReturnTypeOnInterface' => [
'<?php
interface A {
/**
* @return A
*/
public function map(): A;
}
interface B extends A {
/**
* @return B
*/
public function map(): A;
}
function takesB(B $f) : B {
return $f->map();
}'
],
'inheritCorrectReturnTypeOnClass' => [
'<?php
interface A {
/**
* @return A
*/
public function map(): A;
}
interface B extends A {
/**
* @return B
*/
public function map(): A;
}
class F implements B {
public function map(): A {
return new F();
}
}
function takesF(F $f) : B {
return $f->map();
}'
],
];
}

Expand Down
16 changes: 8 additions & 8 deletions tests/InterfaceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ function foo(A $i): B {
'<?php
class SomeIterator extends IteratorIterator {}',
],
'suppressMismatch' => [
'SKIPPED-suppressMismatch' => [
'<?php
interface I {
/**
Expand Down Expand Up @@ -888,17 +888,17 @@ class A implements Container {}',
'inheritMultipleInterfacesWithConflictingDocblocks' => [
'<?php
interface I1 {
/** @return string */
public function foo();
/** @return string */
public function foo();
}
interface I2 {
/** @return int */
public function foo();
/** @return int */
public function foo();
}
class A implements I1, I2 {
public function foo() {
return "hello";
}
public function foo() {
return "hello";
}
}',
'error_message' => 'InvalidReturnType',
],
Expand Down
44 changes: 22 additions & 22 deletions tests/MethodSignatureTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,30 +457,30 @@ public function testIterable(?iterable $i) : array {
'allowVoidToNullConversion' => [
'<?php
class A {
/** @return ?string */
public function foo() {
return rand(0, 1) ? "hello" : null;
}
/** @return ?string */
public function foo() {
return rand(0, 1) ? "hello" : null;
}
}
class B extends A {
public function foo(): void {
return;
}
public function foo(): void {
return;
}
}
class C extends A {
/** @return void */
public function foo() {
return;
}
/** @return void */
public function foo() {
return;
}
}
class D extends A {
/** @return null */
public function foo() {
return null;
}
/** @return null */
public function foo() {
return null;
}
}',
],
'allowNoChildClassPropertyWhenMixed' => [
Expand Down Expand Up @@ -955,18 +955,18 @@ public function foo(A $a): void {
}',
'error_message' => 'MoreSpecificImplementedParamType',
],
'disallowVoidToNullConversionSignature' => [
'preventVoidToNullConversionSignature' => [
'<?php
class A {
public function foo(): ?string {
return rand(0, 1) ? "hello" : null;
}
public function foo(): ?string {
return rand(0, 1) ? "hello" : null;
}
}
class B extends A {
public function foo(): void {
return;
}
public function foo(): void {
return;
}
}',
'error_message' => 'MethodSignatureMismatch',
],
Expand Down
3 changes: 0 additions & 3 deletions tests/Template/ClassTemplateExtendsTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3339,9 +3339,6 @@ public function boo($x);
* @implements X<T>
*/
class A implements X {
/**
* @return T
*/
public function boo($x) {
return $x[0];
}
Expand Down

0 comments on commit 3dd185e

Please sign in to comment.