Improve type annotations #290

xificurk · 2023-02-07T20:35:46Z

new feature
BC break? no
doc PR: not needed

Add more specific type annotations to improve static analysis support.

src/Utils/Image.php

dg · 2023-02-08T03:53:30Z

src/Utils/ArrayList.php

@@ -15,6 +15,8 @@
 /**
 * Provides the base class for a generic list (items can be accessed by index).
 * @template T
+ * @implements \IteratorAggregate<int, T>


Isn't that duplicate information?

It is, but just for getIterator(). Implementing IteratorAggregate also passes generic type to Traversable. Without it Traversable type is mixed. Not sure if this specific case is problem, but generally speaking, generic interface should be always implemented.

Generic type from getIterator() should be removed, as it is already defined by IteratorAggregate

Thanks for the explanation. Can I ask @xificurk to remove the annotation from getIterator()?

This is a bit more complicated...

Latest PHPStan can infer key/value types even without the @implements annotation. Actually, the correct annotations on methods like getIterator() are the key to get this working.

The @implements annotations on class come into play only when checking compatibility between e.g. ArrayList<Foo> and Traversable<string>.

Example with method annotations only:
https://phpstan.org/r/0ecafc0e-1f6a-422b-8121-601992d34b25

Example with class annotation only:
https://phpstan.org/r/ec2e29f0-53a0-45c8-9030-dcfb5cb3ab57

dg · 2023-02-08T03:54:15Z

src/Utils/Arrays.php

@@ -225,6 +225,7 @@ public static function flatten(array $array, bool $preserveKeys = false): array

 	/**
 	 * Checks if the array is indexed in ascending order of numeric keys from zero, a.k.a list.
+	 * @return ($value is list ? true : false)


What's it good for?

If variable is of type list it will always return true. Informs user that check is effectively useless.

But it should probably be @return ($value is list ? true : bool)

I understand what it does, I ask what good it does :-) I just need an example.

Here it is. If condition is always true.
https://phpstan.org/r/37d792a0-1fe7-488a-8818-15048810f6c0

Sadly it is only half of the solution and it seems phpstan does not support assert for list. Even after isList() is given value array
https://phpstan.org/r/c26d6864-a46a-4d81-9fad-54dbc75d7e83

@mabar @enumag Why do you think the return type should be @return ($value is list ? true : **bool**)? Can you give me example of value type which is not list, but the method will return true?

This change was actually the reason, why the examples you've posted seemingly did not work. The correct conditional return annotation is enough for phpstan to correctly detect always true/false conditions & perform type narrowing.

https://phpstan.org/r/c3f4cdc0-3417-42d8-9e5f-24f1c722b01c

@xificurk Because $value can be a list without PHPStan being aware of it.

@enumag Can you create example in phpstan playground?

btw, isn't it exactly the case of the last if-else block in my example? PHPStan can't know if the value is list or not, but if it is, the isList() method will always return true, otherwise false.

I tried a few things and yeah you're right, false is enough there. PHPStan does treat unknown array as a possible list.

For completeness, here is a good example for it: https://phpstan.org/r/8f789bd9-8fbe-477c-87e1-e7c2cad3d6f2

dg · 2023-02-08T03:54:29Z

src/Utils/Image.php

- * @property-read int $width
- * @property-read int $height
+ * @property-read positive-int $width
+ * @property-read positive-int $height


What's it good for?

Mostly for compatibility with other classes that already expect width and height to be >= 0. e.g. for database entity.

As any type (narrowing) hint, it's good to make code more readable and eliminate useless checks.

If you rely on static analysis and narrow down the types enough, checks like this become useless and can be safely eliminated.

Dimensions are often used in various calculations. Knowing that those are actually positive integers, implies e.g.:

their ratio is well defined and also positive number

the area (number of pixels in image) is positive integer

dg · 2023-02-08T03:56:00Z

src/Utils/Image.php

@@ -267,6 +271,8 @@ public static function detectTypeFromString(string $s, &$width = null, &$height

 	/**
 	 * Returns the file extension for the given `Image::XXX` constant.
+	 * @param self::JPEG|self::PNG|self::GIF|self::WEBP|self::AVIF|self::BMP $type
+	 * @return value-of<self::Formats>


What's @return annotation good for?

For me the most important thing out of this is specifying that the output extension is non-empty string, which is useful when you want to use the extension further. I guess @return non-empty-string could cover most of the use-cases, but why shouldn't we make the return type as narrow as possible?

Now, as a return annotation, this should probably be ImageType::*, right?

No, because it returns one of the string values contained in self::Formats, not ImageType::* value.

src/Utils/Image.php

dg · 2023-03-19T19:31:39Z

I would like to release version 4.0.1 soon, we can finish this somehow @xificurk ?

xificurk · 2023-03-20T06:55:11Z

@dg I'll take a look today/tomorrow.

xificurk · 2023-03-20T20:00:33Z

@dg rebased & squashed, I think it's ready from my point of view.

dg · 2023-03-28T15:53:02Z

Thanks!

mabar reviewed Feb 7, 2023

View reviewed changes

src/Utils/Image.php Outdated Show resolved Hide resolved

xificurk force-pushed the phpstan-types branch 2 times, most recently from 21c3a34 to d82f3ca Compare February 7, 2023 20:45

dg reviewed Feb 8, 2023

View reviewed changes

src/Utils/Image.php Outdated Show resolved Hide resolved

xificurk marked this pull request as draft February 8, 2023 07:43

dg force-pushed the master branch 2 times, most recently from 55488d5 to 3678b20 Compare February 8, 2023 10:09

xificurk force-pushed the phpstan-types branch from d82f3ca to f0569a6 Compare February 20, 2023 20:36

dg added 3 commits March 9, 2023 01:23

added enum ImageType

0c20c9d

Finder: default mask is '*'

c80fcc7

Finder: improved exception when directory is missing

4520494

dg force-pushed the master branch from 3678b20 to b1f7377 Compare March 9, 2023 00:24

Finder: collect() returns list

dd9b21f

dg force-pushed the master branch from b1f7377 to dd9b21f Compare March 19, 2023 19:30

Finder: removed slashes normalization

072c303

xificurk force-pushed the phpstan-types branch from 1578fdc to eda8330 Compare March 20, 2023 07:01

Improve type annotations

686a1d5

xificurk force-pushed the phpstan-types branch from eda8330 to 686a1d5 Compare March 20, 2023 19:25

xificurk marked this pull request as ready for review March 20, 2023 19:29

dg force-pushed the master branch from b2001c9 to 2d60daf Compare March 28, 2023 15:33

Merge branch 'master' into phpstan-types

509e515

dg merged commit 6ab8350 into nette:master Mar 28, 2023

dg pushed a commit that referenced this pull request Mar 28, 2023

improved type annotations (#290)

d6168fe

xificurk deleted the phpstan-types branch March 28, 2023 20:41

dg pushed a commit that referenced this pull request Jul 30, 2023

improved type annotations (#290)

835d6a3

dg pushed a commit that referenced this pull request Jul 30, 2023

improved type annotations (#290)

a4858af

dg pushed a commit that referenced this pull request Jul 30, 2023

improved type annotations (#290)

d42d358

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve type annotations #290

Improve type annotations #290

xificurk commented Feb 7, 2023

dg Feb 8, 2023

mabar Feb 8, 2023

dg Feb 8, 2023

xificurk Mar 20, 2023

dg Feb 8, 2023

mabar Feb 8, 2023

dg Feb 8, 2023

mabar Feb 8, 2023

mabar Feb 8, 2023

xificurk Mar 20, 2023

enumag Mar 20, 2023 •

edited

xificurk Mar 20, 2023

enumag Mar 20, 2023

xificurk Mar 20, 2023

dg Feb 8, 2023

mabar Feb 8, 2023

xificurk Feb 20, 2023

dg Feb 8, 2023

xificurk Feb 20, 2023

dg Mar 19, 2023

xificurk Mar 20, 2023

dg commented Mar 19, 2023

xificurk commented Mar 20, 2023

xificurk commented Mar 20, 2023

dg commented Mar 28, 2023

Improve type annotations #290

Improve type annotations #290

Conversation

xificurk commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enumag Mar 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg commented Mar 19, 2023

xificurk commented Mar 20, 2023

xificurk commented Mar 20, 2023

dg commented Mar 28, 2023

enumag Mar 20, 2023 •

edited