WIP: psalm-specific annotations for restricting post-assertion value types#102
WIP: psalm-specific annotations for restricting post-assertion value types#102Ocramius wants to merge 2 commits intowebmozarts:masterfrom Ocramius:feature/type-assertions
Conversation
|
@Nyholm sorry for not being around the conference venue anymore: I'm falling asleep, so I had to call it a day :| |
| "require-dev": { | ||
| "phpunit/phpunit": "^4.6", | ||
| "sebastian/version": "^1.0.1" | ||
| "phpunit/phpunit": "^8.1.6", |
There was a problem hiding this comment.
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.
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.
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.
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.
@muglug I found that @psalm-assert does not work with union types: expected? Should I report this?
There was a problem hiding this comment.
As of 10 minutes ago, you can now use @psalm-assert countable - https://psalm.dev/r/e9a2641c58
| @@ -0,0 +1,17 @@ | |||
| <?php | |||
There was a problem hiding this comment.
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.
TODO: re-enable this or remove the type annotation
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return array|\ArrayAccess |
There was a problem hiding this comment.
TODO: re-enable this or remove the type annotation
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return array|\Countable |
There was a problem hiding this comment.
TODO: re-enable this or remove the type annotation
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return A|B |
There was a problem hiding this comment.
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.
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.
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.
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return array|\Countable |
There was a problem hiding this comment.
Similar to the above: does @psalm-assert work with union types?
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return array|\Countable |
There was a problem hiding this comment.
Similar to the above: does @psalm-assert work with union types?
| /** | ||
| * @param mixed $value | ||
| * | ||
| * @return A|B |
There was a problem hiding this comment.
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.
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.
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 |
| */ | ||
| public static function ip($value, $message = '') | ||
| { | ||
| if (false === filter_var($value, FILTER_VALIDATE_IP)) { |
There was a problem hiding this comment.
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
| */ | ||
| public static function ipv4($value, $message = '') | ||
| { | ||
| if (false === filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { |
There was a problem hiding this comment.
See other comment about filter_var
| */ | ||
| public static function ipv6($value, $message = '') | ||
| { | ||
| if (false === filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) { |
There was a problem hiding this comment.
See the other comment about filter_var
muglug
left a comment
muglug
left a comment
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| /** | ||
| * @psalm-assert (array|Countable) $value |
There was a problem hiding this comment.
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.
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.
this isn't supported yet - there'll need to be an annotation like object-or-class-string<ExpectedType>
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array|\Countable $array |
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array|\Countable $array |
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array|\Countable $array |
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array|\Countable $array |
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array<int, mixed>&!empty $array |
There was a problem hiding this comment.
@psalm-assert array<int, mixed> $array
@psalm-assert non-empty-countable $array
should work
| } | ||
|
|
||
| /** | ||
| * @psalm-assert array<string, mixed>&!empty $array |
There was a problem hiding this comment.
@psalm-assert array<string, mixed> $array
@psalm-assert non-empty-countable $array
| } | ||
|
|
||
| /** | ||
| * @psalm-assert (array|ArrayAccess) $value |
There was a problem hiding this comment.
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 |
7 participants
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$valueto 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):