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
WIP: psalm-specific annotations for restricting post-assertion value types #102
Conversation
@Nyholm sorry for not being around the conference venue anymore: I'm falling asleep, so I had to call it a day :| |
@@ -18,8 +18,8 @@ | |||
"symfony/polyfill-ctype": "^1.8" | |||
}, | |||
"require-dev": { | |||
"phpunit/phpunit": "^4.6", | |||
"sebastian/version": "^1.0.1" | |||
"phpunit/phpunit": "^8.1.6", |
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.
Likely a problem, since the package still supports PHP 5.x.
A bump in a minor release is highly endorsed, but that's up to the maintainer.
I can also use a different installation method if we don't want to use "require-dev"
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.
I'd prefer to keep allowing 5.3 for this library, though looking at it on the PHP website, it has been end of life for almost 5 years.
Personally i'd like to use another installation method, so we don't have to bump the mimimum version to 7.1
.
What do you think @Nyholm ?
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.
Tbh, anything < 7.2 is dead and defunct, especially when looking at newer releases. That said, modernisation of the library is out of scope in this particular patch: I'd be fine with composer require vimeo/psalm
in a temporary path as part of the .travis.yml
setup.
</ignoreFiles> | ||
</projectFiles> | ||
|
||
<!-- <issueHandlers>--> |
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.
To be removed: by default, the tool uses "error"
for all of these
/** | ||
* @param mixed $value | ||
* | ||
* @return array|\Countable |
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.
@muglug I found that @psalm-assert
does not work with union types: expected? Should I report this?
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.
As of 10 minutes ago, you can now use @psalm-assert countable
- https://psalm.dev/r/e9a2641c58
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.
Noice 👍
@@ -0,0 +1,17 @@ | |||
<?php |
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.
TODO: re-enable this or remove the type annotation
/** | ||
* @param mixed $value | ||
* | ||
* @psalm-return A|class-string<A> |
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.
TODO: re-enable this or remove the type annotation
/** | ||
* @param mixed $value | ||
* | ||
* @return array|\ArrayAccess |
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.
TODO: re-enable this or remove the type annotation
/** | ||
* @param mixed $value | ||
* | ||
* @return array|\Countable |
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.
TODO: re-enable this or remove the type annotation
/** | ||
* @param mixed $value | ||
* | ||
* @return A|B |
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.
TODO: re-enable this or remove the type annotation
*/ | ||
function consume($value) | ||
{ | ||
Assert::isInstanceOfAny($value, [A::class, B::class]); |
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.
Tricky one: I'm not sure if @param array<class-string<T>> $param
can be inferred to @param array<class-string<A>|class-string<B>> $param
here
/** | ||
* @param mixed $value | ||
* | ||
* @return array<int, mixed> |
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.
Not sure if psalm can restrict an array<mixed>
to an array<int, mixed>
, or array<Foo>
to array<int, Foo>
here
/** | ||
* @param mixed $value | ||
* | ||
* @return array<string, mixed> |
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.
/** | ||
* @param mixed $value | ||
* | ||
* @return array|\Countable |
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.
Similar to the above: does @psalm-assert
work with union types?
/** | ||
* @param mixed $value | ||
* | ||
* @return array|\Countable |
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.
Similar to the above: does @psalm-assert
work with union types?
/** | ||
* @param mixed $value | ||
* | ||
* @return A|B |
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.
Similar to the above: does @psalm-assert
work with union types?
Very similar to https://github.com/webmozart/assert/pull/102/files#r291697765
/** | ||
* @param mixed $value | ||
* | ||
* @psalm-return A|class-string<A> |
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.
Very similar to https://github.com/webmozart/assert/pull/102/files#r291697765
use Webmozart\Assert\Assert; | ||
|
||
/** | ||
* @psalm-return callable() : never-returns |
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.
Doesn't seem to work: the idea is that we want to be sure that this is a zero-argument never-returns closure (equivalent to a thrown exception)
Note: I'll have to turn all |
@@ -466,6 +549,9 @@ public static function false($value, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert string $value | |||
*/ | |||
public static function ip($value, $message = '') | |||
{ | |||
if (false === filter_var($value, FILTER_VALIDATE_IP)) { |
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.
Value is not guaranteed to be a string after this point. The documentation on filter_var
mentions the following:
Value to filter. Note that scalar values are converted to string internally before they are filtered.
for example: https://3v4l.org/2mpMA
@@ -476,6 +562,9 @@ public static function ip($value, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert string $value | |||
*/ | |||
public static function ipv4($value, $message = '') | |||
{ | |||
if (false === filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { |
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.
See other comment about filter_var
@@ -486,6 +575,9 @@ public static function ipv4($value, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert string $value | |||
*/ | |||
public static function ipv6($value, $message = '') | |||
{ | |||
if (false === filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) { |
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.
See the other comment about filter_var
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.
ok I can either add these slightly custom assertions outlined in my comments or add a custom parser for @psalm-assert
assertions and allow unions. Which do you think makes more sense?
@@ -350,6 +395,9 @@ public static function isArrayAccessible($value, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert (array|Countable) $value |
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.
Change to @psalm-assert countable
(needs a Psalm release)
/** | ||
* @psalm-template ExpectedType of object | ||
* @psalm-param class-string<ExpectedType> $class | ||
* @psalm-assert ExpectedType|class-string<ExpectedType> $value |
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 isn't supported yet - there'll need to be an annotation like object-or-class-string<ExpectedType>
/** | ||
* @psalm-template ExpectedType of object | ||
* @psalm-param class-string<ExpectedType> $class | ||
* @psalm-assert ExpectedType|class-string<ExpectedType> $value |
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 isn't supported yet - there'll need to be an annotation like object-or-class-string<ExpectedType>
@@ -996,6 +1120,9 @@ public static function keyNotExists($array, $key, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert array|\Countable $array |
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.
@psalm-assert countable $array
@@ -1005,6 +1132,9 @@ public static function count($array, $number, $message = '') | |||
); | |||
} | |||
|
|||
/** | |||
* @psalm-assert array|\Countable $array |
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.
@psalm-assert countable $array
@@ -1016,6 +1146,9 @@ public static function minCount($array, $min, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert array|\Countable $array |
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.
@psalm-assert countable $array
@@ -1027,6 +1160,9 @@ public static function maxCount($array, $max, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert array|\Countable $array |
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.
@psalm-assert countable $array
@@ -1041,6 +1177,9 @@ public static function countBetween($array, $min, $max, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert array<int, mixed>&!empty $array |
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.
@psalm-assert array<int, mixed> $array
@psalm-assert non-empty-countable $array
should work
@@ -1050,6 +1189,9 @@ public static function isList($array, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert array<string, mixed>&!empty $array |
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.
@psalm-assert array<string, mixed> $array
@psalm-assert non-empty-countable $array
@@ -340,6 +382,9 @@ public static function isTraversable($value, $message = '') | |||
} | |||
} | |||
|
|||
/** | |||
* @psalm-assert (array|ArrayAccess) $value |
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.
I can introduce a @psalm-assert array-accessible $value
assertion here
Let me know when you are ready for final review |
I didn't yet have time to work on this, due to IRL work issues, sorry :-\ |
I rebased this feature branch to keep up with the current development https://github.com/zerkms/assert/tree/feature/type-assertions
Is there really no way to stay with |
So here is the idea, what if
Thoughts? |
Note that the current $null = null;
$string = 'foo';
$strings = array($string, $string);
Assert::string($string);
Assert::nullOrString($null);
Assert::nullOrString($string);
Assert::allString($strings);
Assert::nullOrAllString($null);
Assert::nullOrAllString($strings);
// "right-associative": all (null or string)
Assert::allNullOrString(array(null, $string));
Assert::allAllString(array($strings, $strings));
// "right-associative": all (null or (all string))
Assert::allNullOrAllString(array(null, $strings));
// ... Cf #97 |
If that is officially supported - then I guess it would be impossible to psalm-ise it. If it's not officially supported - then it's possible and it should be fixed. |
It looks like psalm does not support nullable types (it's a bug I suppose) or union types, so asserts for I filed a bug report vimeo/psalm#1897 |
@Ocramius , thanx for working on this! Any help needed to finish this PR? |
I have implemented the code generator - that generates the |
Btw, I implemented a generator (really quick and really dirty yet, POC), but it's capable of generating https://github.com/zerkms/assert/blob/feature/type-assertions-zerkms-wip/src/Extra.php In the final implementation current |
Thanks everyone for their work on this, this has been merged as part of #118, where some annotations that weren't working yet were removed, and a few have been changed to match their correct behaviour. @Ocramius, a big thank you for your work on this. @zerkms, feel free to open a PR so we can take a good look at the generator. |
@BackEndTea the generator is in https://github.com/zerkms/assert/blob/feature/type-assertions-zerkms-wip/generate.php in my branch. But it does not matter how you generate, I'm seeking feedback for what was generated. |
Btw, @BackEndTea thank you too, with these changes our code would become even clearer! |
Thanks for dragging this forward, @BackEndTea! Sorry if I ended up not getting to it any more :S |
As discussed with @Nyholm, this is a WIP patch containing happy-path scenarios (some of which are marked as
.disabled
, will talk to @muglug about those) that ensure the added type declarations work as expected.The idea is that after a
@psalm-assert int $value
, the caller ofAssert::integer($value)
can assume$value
to beint
.This is equivalent to the behavior of https://github.com/phpstan/phpstan-webmozart-assert, but the declarations are now closer to the source, and that makes for easier usage from the ecosystem (support by phpstan and phpstorm may come soon ;-) )
As for the prefixed annotation format: that is required to not trip over tools like
doctrine/annotations
.Other similar patches, for reference (since this is quite new stuff):