-
-
Notifications
You must be signed in to change notification settings - Fork 152
Improve type annotations #290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0c20c9d
c80fcc7
4520494
dd9b21f
072c303
686a1d5
509e515
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's it good for?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If variable is of type list it will always return true. Informs user that check is effectively useless. But it should probably be
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand what it does, I ask what good it does :-) I just need an example.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it is.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mabar @enumag Why do you think the return type should be 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xificurk Because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried a few things and yeah you're right, false is enough there. PHPStan does treat unknown array as a possible list.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For completeness, here is a good example for it: https://phpstan.org/r/8f789bd9-8fbe-477c-87e1-e7c2cad3d6f2 |
||
| */ | ||
| public static function isList(mixed $value): bool | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,8 +90,8 @@ | |
| * @method void stringUp($font, $x, $y, string $s, $col) | ||
| * @method void trueColorToPalette(bool $dither, $ncolors) | ||
| * @method array ttfText($size, $angle, $x, $y, $color, string $fontfile, string $text) | ||
| * @property-read int $width | ||
| * @property-read int $height | ||
| * @property-read positive-int $width | ||
| * @property-read positive-int $height | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's it good for?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly for compatibility with other classes that already expect width and height to be >= 0. e.g. for database entity.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.:
|
||
| * @property-read \GdImage $imageResource | ||
| */ | ||
| class Image | ||
|
|
@@ -220,6 +220,8 @@ private static function invokeSafe(string $func, string $arg, string $message, s | |
|
|
||
| /** | ||
| * Creates a new true color image of the given dimensions. The default color is black. | ||
| * @param positive-int $width | ||
| * @param positive-int $height | ||
| * @throws Nette\NotSupportedException if gd extension is not loaded | ||
| */ | ||
| public static function fromBlank(int $width, int $height, ?array $color = null): static | ||
|
|
@@ -247,6 +249,7 @@ public static function fromBlank(int $width, int $height, ?array $color = null): | |
|
|
||
| /** | ||
| * Returns the type of image from file. | ||
| * @return ImageType::*|null | ||
| */ | ||
| public static function detectTypeFromFile(string $file, &$width = null, &$height = null): ?int | ||
| { | ||
|
|
@@ -257,6 +260,7 @@ public static function detectTypeFromFile(string $file, &$width = null, &$height | |
|
|
||
| /** | ||
| * Returns the type of image from string. | ||
| * @return ImageType::*|null | ||
| */ | ||
| public static function detectTypeFromString(string $s, &$width = null, &$height = null): ?int | ||
| { | ||
|
|
@@ -267,6 +271,8 @@ public static function detectTypeFromString(string $s, &$width = null, &$height | |
|
|
||
| /** | ||
| * Returns the file extension for the given `ImageType::XXX` constant. | ||
| * @param ImageType::* $type | ||
| * @return value-of<self::Formats> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, as a return annotation, this should probably be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because it returns one of the string values contained in |
||
| */ | ||
| public static function typeToExtension(int $type): string | ||
| { | ||
|
|
@@ -280,6 +286,7 @@ public static function typeToExtension(int $type): string | |
|
|
||
| /** | ||
| * Returns the `ImageType::XXX` constant for given file extension. | ||
| * @return ImageType::* | ||
| */ | ||
| public static function extensionToType(string $extension): int | ||
| { | ||
|
|
@@ -295,6 +302,7 @@ public static function extensionToType(string $extension): int | |
|
|
||
| /** | ||
| * Returns the mime type for the given `ImageType::XXX` constant. | ||
| * @param ImageType::* $type | ||
| */ | ||
| public static function typeToMimeType(int $type): string | ||
| { | ||
|
|
@@ -314,6 +322,7 @@ public function __construct(\GdImage $image) | |
|
|
||
| /** | ||
| * Returns image width. | ||
| * @return positive-int | ||
| */ | ||
| public function getWidth(): int | ||
| { | ||
|
|
@@ -323,6 +332,7 @@ public function getWidth(): int | |
|
|
||
| /** | ||
| * Returns image height. | ||
| * @return positive-int | ||
| */ | ||
| public function getHeight(): int | ||
| { | ||
|
|
@@ -351,7 +361,7 @@ public function getImageResource(): \GdImage | |
|
|
||
| /** | ||
| * Scales an image. Width and height accept pixels or percent. | ||
| * @param self::OrSmaller|self::OrBigger|self::Stretch|self::Cover|self::ShrinkOnly $mode | ||
| * @param int-mask-of<self::OrSmaller|self::OrBigger|self::Stretch|self::Cover|self::ShrinkOnly> $mode | ||
| */ | ||
| public function resize(int|string|null $width, int|string|null $height, int $mode = self::OrSmaller): static | ||
| { | ||
|
|
@@ -388,7 +398,7 @@ public function resize(int|string|null $width, int|string|null $height, int $mod | |
|
|
||
| /** | ||
| * Calculates dimensions of resized image. Width and height accept pixels or percent. | ||
| * @param self::OrSmaller|self::OrBigger|self::Stretch|self::Cover|self::ShrinkOnly $mode | ||
| * @param int-mask-of<self::OrSmaller|self::OrBigger|self::Stretch|self::Cover|self::ShrinkOnly> $mode | ||
| */ | ||
| public static function calculateSize( | ||
| int $srcWidth, | ||
|
|
@@ -534,7 +544,7 @@ public function sharpen(): static | |
|
|
||
| /** | ||
| * Puts another image into this image. Left and top accepts pixels or percent. | ||
| * @param int $opacity 0..100 | ||
| * @param int<0, 100> $opacity 0..100 | ||
| */ | ||
| public function place(self $image, int|string $left = 0, int|string $top = 0, int $opacity = 100): static | ||
| { | ||
|
|
@@ -596,6 +606,7 @@ public function place(self $image, int|string $left = 0, int|string $top = 0, in | |
|
|
||
| /** | ||
| * Saves image to the file. Quality is in the range 0..100 for JPEG (default 85), WEBP (default 80) and AVIF (default 30) and 0..9 for PNG (default 9). | ||
| * @param ImageType::*|null $type | ||
| * @throws ImageException | ||
| */ | ||
| public function save(string $file, ?int $quality = null, ?int $type = null): void | ||
|
|
@@ -607,6 +618,7 @@ public function save(string $file, ?int $quality = null, ?int $type = null): voi | |
|
|
||
| /** | ||
| * Outputs image to string. Quality is in the range 0..100 for JPEG (default 85), WEBP (default 80) and AVIF (default 30) and 0..9 for PNG (default 9). | ||
| * @param ImageType::* $type | ||
| */ | ||
| public function toString(int $type = ImageType::JPEG, ?int $quality = null): string | ||
| { | ||
|
|
@@ -627,6 +639,7 @@ public function __toString(): string | |
|
|
||
| /** | ||
| * Outputs image to browser. Quality is in the range 0..100 for JPEG (default 85), WEBP (default 80) and AVIF (default 30) and 0..9 for PNG (default 9). | ||
| * @param ImageType::* $type | ||
| * @throws ImageException | ||
| */ | ||
| public function send(int $type = ImageType::JPEG, ?int $quality = null): void | ||
|
|
@@ -639,6 +652,7 @@ public function send(int $type = ImageType::JPEG, ?int $quality = null): void | |
| /** | ||
| * Outputs image to browser or file. | ||
| * @throws ImageException | ||
| * @param ImageType::* $type | ||
| */ | ||
| private function output(int $type, ?int $quality, ?string $file = null): void | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that duplicate information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. Can I ask @xificurk to remove the annotation from
getIterator()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit more complicated...
Latest PHPStan can infer key/value types even without the
@implementsannotation. Actually, the correct annotations on methods likegetIterator()are the key to get this working.The
@implementsannotations on class come into play only when checking compatibility between e.g.ArrayList<Foo>andTraversable<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