Open 00dani opened 2 years ago
Here is an other example: https://phpstan.org/r/064e69da-8b94-461b-872c-a2322d69e2a6
In a situation like this, I believe that PHPStan should effectively unify the types of
$this->attribute
and$value
, in the logic programming sense. The inferred types of both variables ought to be narrowed by the assignment expression, preserving type compatibility for both the outer class type and its type parameters. In other words, this ought to happen:
Agreed.
So we would have these rules:
(Rules 1-3 are the current behavior)
*NEVER*
type could denote that, or an other specific typeFoo<int>
accepts Foo<unresolved>
)Rules 4 and 5 prevent aliasing the same value from different variables of different types, which closes the hole. I think that its important that all type parameters are resolved in 4 and 5, otherwise it's still possible to create aliases with different types.
Examples for rule 4 and 5:
/** @var Collection<int> */
private Collection $ints;
/** @var Collection<int> */
private Collection $strings;
public function __construct() {
$ints = new Collection(); // $ints is a Collection<unresolved>
$this->ints = $ints; // $ints is now a Collection<int>
$this->strings = $ints; // error: Collection<int> is not accepted by Collection<string>
}
/** @param Collection<int> $ints */
function takeInts(Collection $ints): void {}
/** @param Collection<string> $strings */
function takeStrings(Collection $strings): void {}
$ints = new Collection(); // $ints is a Collection<unresolved>
takeInts($ints); // $ints is now a Collection<int>
takeStrings($ints); // error
/** @var Collection<int>|null */
$ints = null;
$tmp = new Collection(); // $tmp is a Collection<unresolved>
$ints = $tmp; // $ints is now a Collection<int>
$ints = new Collection(); // $tmp is a Collection<unresolved>
function () use ($ints): void { // $ints is now a Collection<ints>
takeInts($ints);
}
$ints = new Collection(); // $tmp is a Collection<unresolved>
fn () => // $ints is now a Collection<ints>
takeInts($ints);
$ints = new Collection(); // $ints is a Collection<unresolved>
$ints->add(1); // $ints is now a Collection<int>
$ints->add("a"); // error
$ints = new Collection(); // $ints is a Collection<unresolved>
takeInts(
$foo ? $ints : new Collection()
) // $ints is now a Collection<int>
In my experience, most use cases fall into the argument narrowing use case.
As discussed with @ondrejmirtes here, most programming languages with generic support can proper infer cases like the following:
/**
* @template T
*/
final class Collection
{
/**
* @param T $value
*/
public function add(mixed $value): void
{
}
}
/**
* @template T
*/
class Example
{
public function fill(): void
{
$this->addAll(new Collection());
}
/**
* @param Collection<T> $collection
*/
public function addAll(Collection $collection): void
{
}
}
Unfortunately, casting is the only workaround available, but it introduces many unnecessary variable assignments and, consequently, a cognitive burden.
This example shows how painful this limitation is currently:
<?php declare(strict_types = 1);
class Key {
}
final class Example
{
/** @var \WeakMap<Key, int> */
public \WeakMap $map;
/**
* @param \WeakMap<Key, int> $map
*/
public function __construct(\WeakMap $map = new \WeakMap()) {
$this->map = $map;
}
}
Property Example::$map (WeakMap<Key, int>) does not accept WeakMap<Key, int>|WeakMap<object, mixed>.
To make it work today, you need this:
<?php declare(strict_types = 1);
class Key {
}
final class Example
{
/** @var \WeakMap<Key, int> */
public \WeakMap $map;
/**
* @param \WeakMap<Key, int> $map
*/
public function __construct(?\WeakMap $map = null) {
if ($map === null) {
/** @var \WeakMap<Key, int> $map **/
$map = new \WeakMap();
}
$this->map = $map;
}
}
As expected, due to the same issue, this also doesn't work:
<?php declare(strict_types = 1);
class Key {
}
final class Example
{
/** @var \WeakMap<Key, int> */
public \WeakMap $map;
/**
* @param \WeakMap<Key, int> $map
*/
public function __construct(?\WeakMap $map = null) {
$this->map = $map ?? new \WeakMap();
}
}
Hi! Could this issue be the cause of the 2 errors here? https://phpstan.org/r/814c4824-d65a-4108-8547-59058c1c901f
<?php declare(strict_types = 1);
use Ds\Set;
class MyTest {
/**
* @return Set<mixed>
*/
public function provideSets1(): Set
{
return new Set(['a']);
}
/**
* @return Set<string>
*/
public function provideSets2(): Set
{
return new Set(['a']);
}
/**
* @return Set<string|int>
*/
public function provideSets3(): Set
{
return new Set(['a']);
}
}
The errors:
Line | Error
---- | --
11 | Method MyTest::provideSets1() should return Ds\Set<mixed> but returns Ds\Set<string>.
27 | Method MyTest::provideSets3() should return Ds\Set<int\|string> but returns Ds\Set<string>.
I opened a duplicate issue in Psalm's repo: https://github.com/vimeo/psalm/issues/8482
Hack solves this with the solution I give there, but I'm hesitant to copy that implementation because the type hole is a pretty small edge-case.
Hack solves this with the solution I give there, but I'm hesitant to copy that implementation because the type hole is a pretty small edge-case
We've several cases like the one I've shared in a single application and dozens throughout the codebase. I don't think it's uncommon at all.
The edge-case I was talking about is the false-negative I mention in the linked issue. This issue also mentions a number of false-positives in PHPStan (some of which also appear in Psalm), but I wasn't talking about those. In a tool that has few of either, I see false-negatives as inherently more dangerous than false-positives.
because the type hole is a pretty small edge-case
I consider the implementation suggestion above with Collection<unresolved>
etc. to be a huge deal, because once implemented, it will allow us to get rid of generic types generalization (where 1
becomes an int
). More and more issues are being reported connected to that problem.
Closing a small type hole is just a small part of that...
After this we'll also be able to do https://github.com/phpstan/phpstan-src/pull/2110.
@marcospassos After the latest push in 2.0.x, PHPStan now reports different result with your code snippet:
@@ @@
+PHP 8.0 – 8.1 (1 error)
+==========
+
+23: Parameter #1 $collection of method Example<T>::addAll() expects Collection<T>, Collection<mixed> given.
+
+PHP 7.1 – 7.4 (3 errors)
+==========
+
+11: PHPDoc tag @param for parameter $value with type T is not subtype of native type mixed.
+11: Parameter $value of method Collection::add() has invalid type mixed.
23: Parameter #1 $collection of method Example<T>::addAll() expects Collection<T>, Collection<mixed> given.
@michelv After the latest push in 2.0.x, PHPStan now reports different result with your code snippet:
@@ @@
-11: Method MyTest::provideSets1() should return Ds\Set<mixed> but returns Ds\Set<string>.
-27: Method MyTest::provideSets3() should return Ds\Set<int|string> but returns Ds\Set<string>.
+11: Method MyTest::provideSets1() should return Ds\Set<mixed> but returns Ds\Set<'a'>.
+19: Method MyTest::provideSets2() should return Ds\Set<string> but returns Ds\Set<'a'>.
+27: Method MyTest::provideSets3() should return Ds\Set<int|string> but returns Ds\Set<'a'>.
@arnaud-lb After the latest push in 2.0.x, PHPStan now reports different result with your code snippet:
@@ @@
-37: Dumped type: Collection<mixed>
+36: Parameter #1 $c of function takeCollection expects Collection<int>, Collection<mixed> given.
+37: Dumped type: Collection<mixed>
+38: Function makeCollection() should return Collection<string> but returns Collection<mixed>.
Feature request
Currently an assignment of the form
$this->attribute = $value
narrows the type of$this->attribute
to the same type as$value
for the remainder of the method. In most cases, this makes sense.For example, the following code is statically sound. Iterables don't always have a
forAll
method, but there's no possible way$this->stuff
can be of a type other thanArrayCollection
when we attempt to call theforAll
method.However, a problem arises if the type of
$value
is in some way wider than the type of$this->attribute
. This is only possible when generic types are used:$this->attribute
might have narrower type parameters than$value
does, although$value
might have a more specific class type. Since the assignment only affects the inferred type of$this->attribute
, it actually widens those type parameters.I demonstrated an example of this problem a few minutes ago, which I'll reproduce here with
dumpType
information included:As you can see, PHPStan "forgets" that
$this->ints
and$this->strings
have different type parameters. This leads to an admittedly-minor hole in the type system, since this mechanism may be used to statically cast from any type to any other, without a corresponding runtime conversion occurring.In a situation like this, I believe that PHPStan should effectively unify the types of
$this->attribute
and$value
, in the logic programming sense. The inferred types of both variables ought to be narrowed by the assignment expression, preserving type compatibility for both the outer class type and its type parameters. In other words, this ought to happen:This narrowing step would cause the subsequent assignment,
$this->strings = $array
, to fail type-checking: aCollection<int, int>
cannot be assigned to a variable of typeCollection<int, string>
. The hole in the type system is sealed.I do think this is an unlikely situation, since one would typically write
$this->ints = new ArrayCollection()
in the first place rather than using a temporary variable. Additionally, Psalm has the exact same type system hole at time of writing. Still, this is an issue worth being aware of. Also, narrowing bidirectionally would allow the constructor to go ahead and populate$this->ints
in a properly typesafe way, which currently doesn't occur.Did PHPStan help you today? Did it make you happy in any way?
Yes! I've been experimenting with API Platform at work, since we're hoping to adopt it for newly planned projects, and API Platform's very structured design has turned out to mesh wonderfully with PHPStan's type checking. As a Haskell fan I generally love powerful type systems, and PHPStan has been giving me those warm fuzzy TypeScript feels despite working in PHP. 🐱