danog/psalm
1
0
Fork 0
mirror of https://github.com/danog/psalm.git synced 2024-11-30 04:39:00 +01:00
Code Issues Projects Releases Wiki Activity

Allow property type invariance on templated properties

Browse Source 
This is a bit of a hack – the comparison should be similar to the ones done in MethodComparator, but this avoids false-positives for now
This commit is contained in:
Matt Brown 2021-02-07 01:45:48 -05:00
parent 049b2c3f7a
commit 4d76f7545c
3 changed files with 116 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
docs/running_psalm/issues/NonInvariantDocblockPropertyType.md
View File

@ -6,20 +6,54 @@ Emitted when a public or protected class property has a different docblock type
<?php
class A {
/** @var string */
/** @var null|string */
public $foo = 'hello';
}
class B extends A {
/** @var null|string */
class AChild extends A {
/** @var string */
public $foo;
}
```
## Why this is bad
For non-typed properties, it can cause a type system violation when code written against the parent class reads or writes a value on an object of the child class.
If the child class widens the type then reading the value may return unexpected value that client code cannot deal with. If the child class narrows the type then code setting the value may set
it to an invalid value.
If the child class widens the type then reading the value may return unexpected value that client code cannot deal with. If the child class narrows the type then code setting the value may set it to an invalid value:
```php
<?php
function takesA(A $a) {
$a->foo = null; // this is valid for A
}
$child = new AChild();
takesA($child);
echo strlen($child->foo); // this is valid for AChild
```
## Workarounds
You can either broaden the type or you could, in certain situations, use templates instead:
```php
<?php
/**
* @template T as string|null
*/
abstract class A {
/** @var T */
public $foo = 'hello';
}
/**
* @extends A<string>
*/
class AChild extends A {
/** @var string */
public $foo;
}
```

68
src/Psalm/Internal/Analyzer/ClassAnalyzer.php
View File

@ -664,43 +664,49 @@ class ClassAnalyzer extends ClassLikeAnalyzer
}
}
if ((($property_storage->signature_type
&& !$guide_property_storage->signature_type)
|| (!$property_storage->signature_type
&& $guide_property_storage->signature_type)
|| ($property_storage->signature_type
&& $guide_property_storage->signature_type
&& !$property_storage->type->equals($guide_property_storage->type)))
&& $property_storage->location
) {
if (IssueBuffer::accepts(
new NonInvariantPropertyType(
'Property ' . $fq_class_name . '::$' . $property_name
. ' has type ' . $property_storage->type->getId()
. ", not invariant with " . $guide_class_name . '::$'
. $property_name . ' of type '
. $guide_property_storage->type->getId(),
$property_storage->location
),
$property_storage->suppressed_issues
)) {
// fall through
}
}
if ($property_storage->type
&& $guide_property_storage->type
&& $property_storage->location
&& !$property_storage->type->equals($guide_property_storage->type)
&& !$guide_property_storage->type->hasTemplate()
) {
if ($property_storage->type->from_docblock
&& $guide_property_storage->type->from_docblock
) {
if (IssueBuffer::accepts(
new NonInvariantDocblockPropertyType(
'Property ' . $fq_class_name . '::$' . $property_name
. ' has type ' . $property_storage->type->getId()
. ", not invariant with " . $guide_class_name . '::$'
. $property_name . ' of type '
. $guide_property_storage->type->getId(),
$property_storage->location
),
$property_storage->suppressed_issues
)) {
// fall through
}
} else {
if (IssueBuffer::accepts(
new NonInvariantPropertyType(
'Property ' . $fq_class_name . '::$' . $property_name
. ' has type ' . $property_storage->type->getId()
. ", not invariant with " . $guide_class_name . '::$'
. $property_name . ' of type '
. $guide_property_storage->type->getId(),
$property_storage->location
),
$property_storage->suppressed_issues
)) {
// fall through
}
if (IssueBuffer::accepts(
new NonInvariantDocblockPropertyType(
'Property ' . $fq_class_name . '::$' . $property_name
. ' has type ' . $property_storage->type->getId()
. ", not invariant with " . $guide_class_name . '::$'
. $property_name . ' of type '
. $guide_property_storage->type->getId(),
$property_storage->location
),
$property_storage->suppressed_issues
)) {
// fall through
}
}
}
}

61
tests/PropertyTypeInvariance.php → tests/PropertyTypeInvarianceTest.php
View File

@ -1,7 +1,7 @@
<?php
namespace Psalm\Tests;
class PropertyTypeInvariance extends TestCase
class PropertyTypeInvarianceTest extends TestCase
{
use Traits\InvalidCodeAnalysisTestTrait;
use Traits\ValidCodeAnalysisTestTrait;
@ -12,35 +12,52 @@ class PropertyTypeInvariance extends TestCase
public function providerValidCodeParse(): iterable
{
return [
'validcode' =>
['<?php
'validcode' => [
'<?php
class ParentClass
{
/** @var null|string */
protected $mightExist;
class ParentClass
{
/** @var null|string */
protected $mightExist;
protected ?string $mightExistNative = null;
protected ?string $mightExistNative = null;
/** @var string */
protected $doesExist = "";
/** @var string */
protected $doesExist = "";
protected string $doesExistNative = "";
}
protected string $doesExistNative = "";
}
class ChildClass extends ParentClass
{
/** @var null|string */
protected $mightExist = "";
class ChildClass extends ParentClass
{
/** @var null|string */
protected $mightExist = "";
protected ?string $mightExistNative = null;
protected ?string $mightExistNative = null;
/** @var string */
protected $doesExist = "";
/** @var string */
protected $doesExist = "";
protected string $doesExistNative = "";
}'
],
'allowTemplatedInvariance' => [
'<?php
/**
* @template T as string|null
*/
abstract class A {
/** @var T */
public $foo;
}
protected string $doesExistNative = "";
}
'],
/**
* @extends A<string>
*/
class AChild extends A {
/** @var string */
public $foo = "foo";
}'
],
];
}